-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
buffer: added Buffer methods to r/w 24 bits integers. #169
Conversation
Added readInt24LE() readInt24BE(), readUInt24LE(), readUInt24BE(). Added tests.
I have seen readUIntLE/readUIntBE/readIntUE.. methods, but I think it could be simpler (also faster) to have direct methods to read/write at least 24 bits / 3 bytes integers. @trevnorris @bnoordhuis if the PR sounds good, I can do another PR with updated docs. |
Performance difference is on the order of sub 10 ns. I don't think adding additional methods to Buffer is really necessary. |
Hi @trevnorris, Thanks for you reply. However, generally I deal with big Buffers ( > 1MB), you can see that exists a real gain (not ns), here a gist to run; see results, when the Buffer is small, improvements are not tangible, I agree, but if you deal a lot with bigger Buffers you can see a real difference in the long run. I see a gain up to 40% (then seconds). P.S. The real difference is in writing Buffers, Sorry, I forget to mention write methods in the commit message! |
@trevnorris , As a last resort, can I appeal to Ecology? |
@rootslab what protocol do you implement by using 3 bytes |
Hi @yorkie, thanks for your interest, simply, I read/write big chunks of values through Buffers. |
@yorkie one thing that comes to mind is 24-bit audio. but then i'm not sure how people typically organise that kind of information in memory and on disk. |
@rootslab I appreciate the insight, and I think there's probably a fast-path that can be managed in the general implementation for 24-bit. I mean, honestly, is there really a use case for 40/48 bit reads/writes? I just did it because it could be done. :P That being said, I'll look into how to speed up the generalized case. Thanks for your PR, and more so for validation that 24 bit reads/writes are actually used. |
Hi @trevnorris, the only way that comes to my mind, to improve speed for generalized code, is to use an array to cache multipliers values from 1 to 6 bytes, but I think that you wouldn't like to use an array inside Buffer code, right? |
@rootslab Also can do something as simplistic as: if (byteLength === 3)
return ((this[offset]) |
(this[offset + 1] << 8) |
(this[offset + 2] << 16)); |
@trevnorris, ok, now I understand what you mean, you propose a test condition only for 24 bits in the generalized code, I agree with you ;) |
As per spec `Object.getOwnPropertyNames()` and `Object.getPropertyNames()` should return an array of strings. However there is no standardization of what should happen if these methods are called natively (through NaN for example). For these 2 APIs, v8 returns an array containing mix of numbers for numeric properties and strings for normal string properties. This breaks [grpc](https://github.com/grpc/grpc) module because it [expects](https://github.com/grpc/grpc/blob/master/src/node/ext/call.cc#L673-L683) returned values to be numeric. Fix: Once we get the result from, iterate over the result and call `parseInt` to save numeric value of the key. PR-URL: nodejs/node#169 Reviewed-By: Jianchun Xu <[email protected]>
Added readInt24LE() readInt24BE(), readUInt24LE(), readUInt24BE(), writeInt24LE() writeInt24BE(), writeUInt24LE(), writeUInt24BE().
Added tests.