buffer: remove noAssert and improve performance #18395
Conversation
BridgeAR
added
buffer
BridgeAR
requested review from
apapirovski,
mcollina,
mscdex,
bnoordhuis,
trevnorris,
jasnell,
addaleax,
TimothyGu and
cjihrig
nodejs-github-bot
added
buffer
BridgeAR
added
semver-major
There was a problem hiding this comment.
This looks very good. I am a bit concerned by the perf regressions in a handful of the tests but given the overall change, I think those are reasonable and this is definitely something to get landed. As I mentioned on the phone with you, I'd like a version of this that we can backport without the semver-major changes but I'm definitely +1 on getting this PR landed. Great work.
test/parallel/test-buffer-fakes.js
Outdated
| assert.throws(function() { | ||
| fb.writeFloatLE(0); | ||
| }, TypeError); | ||
|
|
There was a problem hiding this comment.
Note: this does not throw anymore and works the same as any other regular buffer method from now on. This was only important for the c++ code that now does not exist anymore.
|
Please take a look nevertheless! |
|
About the performance regressions: This is very simple math and often the operations needed are very little. So any minimal check added will therefore regress the performance. That should not be important though as long as we are sure we have the best overall implementation in place for the mathematical operations on their own. Most functions will see a significant performance boost and only a few functions work a tiny bit slower. To outline what I mean: const res = new Array(2)
function simpleMath(v) {
v = +v
res[0] = v
res[1] = v << 8
return res
}That function is so small and there is so little to do that adding a check like |
BridgeAR
added
the
notable-change
|
The endian issue is fixed. |
|
I had another look at the code and found a elegant way to improve the performance in all cases significantly more. Now all I updated the description above accordingly including the benchmarks. |
lib/buffer.js
Outdated
| checkOffset(offset, byteLength, this.length); | ||
| } | ||
| Buffer.prototype.readUIntLE = function readUIntLE(offset, byteLength) { | ||
| if (byteLength === 6) |
There was a problem hiding this comment.
This should be better as a switch statement, no?
There was a problem hiding this comment.
I personally prefer this style as it is a bit less to read. The performance is the same. If you have a strong feeling about it, I'll change it, otherwise I would rather keep it.
lib/buffer.js
Outdated
| while (byteLength > 0 && (mul *= 0x100)) | ||
| val += this[offset + --byteLength] * mul; | ||
| Buffer.prototype.readUIntBE = function readUIntBE(offset, byteLength) { | ||
| if (byteLength === 6) |
There was a problem hiding this comment.
Same as above, re: switch. More of these down below, won't mention every single one :)
Support for the `noAssert` argument dropped in the upcoming Node.js v.10. This removes the argument to make sure everything works as it should. Refs: nodejs/node#18395
|
Ping @ChALkeR @addaleax about #18395 (comment) |
Support for the `noAssert` argument dropped in the upcoming Node.js v.10. This removes the argument to make sure everything works as it should. Refs: nodejs/node#18395
Support for the `noAssert` argument dropped in the upcoming Node.js v.10. This removes the argument to make sure everything works as it should. Refs: nodejs/node#18395
|
@BridgeAR Yes, we need to add back support for omitting the offset (or revert this PR). I’m okay with removing coercion support for other value types like strings, though. |
Support for the `noAssert` argument dropped in the upcoming Node.js v.10. This removes the argument to make sure everything works as it should. Refs: nodejs/node#18395
If none is provided, use zero as a default offset for all read/write operations on the buffer. PR-URL: nodejs#19749 Refs: nodejs#18395 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
If none is provided, use zero as a default offset for all read/write operations on the buffer. PR-URL: #19749 Refs: #18395 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
The FIXME is obsolete as it was meant about a indentation issue that got fixed a long time ago. PR-URL: nodejs#18395 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
This ports the Buffer#write(Double|Float)(B|L)E functions to JS. This fixes a security issue concerning type confusion and fixes another possible crash in combination with `noAssert`. In addition to that it will also significantly improve the write performance. Fixes: nodejs#12179 Fixes: nodejs#8724 PR-URL: nodejs#18395 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
PR-URL: nodejs#18395 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
There are a lot of changes in this commit: 1) Remove the `noAssert` argument from all read and write functions. 2) Improve the performance of all read floating point functions significantly. This is done by switching to TypedArrays as the write floating point write functions. 3) No implicit type coercion for offset and byteLength anymore. 4) Adds a lot of tests. 5) Moves the read and write functions to the internal buffer file to split the files in smaller chunks. 6) Reworked a lot of existing tests. 7) Improve the performane of all all read write functions by using a faster input validation and by improving function logic. 8) Significantly improved the performance of all read int functions. This is done by using a implementation without a loop. 9) Improved error handling. 10) Rename test file to use the correct subsystem. PR-URL: nodejs#18395 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
This removes the `noAssert` argument and also adds some more tests. PR-URL: nodejs#18395 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
PR-URL: nodejs#18395 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
|
|
||
| function boundsError(value, length, type) { | ||
| if (Math.floor(value) !== value) { | ||
| checkNumberType(value, type); |
There was a problem hiding this comment.
That's a little confused. In other constructions where type is used offset is default value.
Meanwhile checkNumberType (currently validateNumber) will throw AssertionError if type is undefined, because ERR_INVALID_ARG_TYPE have assert which check that type should be a string.
Should it be changed to: function boundsError(value, length, type = 'offset') { ?
@BridgeAR
There was a problem hiding this comment.
The default value is set in checkNumberType in this PR. Seems like it regressed in #24815. I'll open a PR to fix this. Thanks for reporting it!
There was a problem hiding this comment.
I just checked this again and all call sites seem fine, even with the PR I mentioned. Do you have any case that actually throws an AssertionError? I can't come up with any case.
There was a problem hiding this comment.
There is no way to receive AssertionError right now. It's only about readability, when I checked code and found that type passed as undefined I asked myself "is it possible that some error will be thrown?", then I found assert and start looked for real case when error can be thrown. But then I realized that validateNumber will never throw error here because always called before.
If default value will be exists probably this will be easier for reading.
There was a problem hiding this comment.
What about such change? Or will be reject because not fix anything?
diff --git a/lib/internal/buffer.js b/lib/internal/buffer.js
index f3bc0e48d1..5a6dddf9f8 100644
--- a/lib/internal/buffer.js
+++ b/lib/internal/buffer.js
@@ -60,17 +60,18 @@ function checkInt(value, min, max, buf, offset, byteLength) {
checkBounds(buf, offset, byteLength);
}
-function boundsError(value, length, type) {
+function boundsError(value, length, type = 'offset') {
if (Math.floor(value) !== value) {
validateNumber(value, type);
- throw new ERR_OUT_OF_RANGE(type || 'offset', 'an integer', value);
+ throw new ERR_OUT_OF_RANGE(type, 'an integer', value);
}
if (length < 0)
throw new ERR_BUFFER_OUT_OF_BOUNDS();
- throw new ERR_OUT_OF_RANGE(type || 'offset',
- `>= ${type ? 1 : 0} and <= ${length}`,
+ const lowerBound = type === 'offset' ? 0 : 1;
+ throw new ERR_OUT_OF_RANGE(type,
+ `>= ${lowerBound} and <= ${length}`,
value);
}Support for the `noAssert` argument dropped in the upcoming Node.js v.10. This removes the argument to make sure everything works as it should. Refs: nodejs/node#18395
Support for the `noAssert` argument dropped in the upcoming Node.js v.10. This removes the argument to make sure everything works as it should. Refs: nodejs/node#18395
Support for the `noAssert` argument dropped in the upcoming Node.js v.10. This removes the argument to make sure everything works as it should. Refs: nodejs/node#18395
Support for the `noAssert` argument dropped in the upcoming Node.js v.10. This removes the argument to make sure everything works as it should. Refs: nodejs/node#18395
* Use Buffer.alloc/Buffer.from instead of "new Buffer" * Remove the 'noAssert' parameter from the write calls This got removed with nodejs/node#18395 Fixes pierrec#91
* Use Buffer.alloc/Buffer.from instead of "new Buffer" * Remove the 'noAssert' parameter from the write calls This got removed with nodejs/node#18395 Fixes pierrec#91
* Use Buffer.alloc/Buffer.from instead of "new Buffer" * Remove the 'noAssert' parameter from the write calls This got removed with nodejs/node#18395 Fixes #91
This PR contains the following main changes:
noAssertargument from all buffer functions.Update: There is no performance penalty in the regular case anymore. Instead there is a constant performance gain! Only while using
noAssert = truesome cases exist that got a minor performance penalty. This mainly applies to allBuffer#(write|read)U?Int\d\d?(B|L)Efunctions.Now it will not be possible to insert any non integer numbers as offset or byteLength. Any other type than number of floats will throw.
Benchmarks:
Write performance (noAssert = false)
Write performance (noAssert = true)
Read performance (noAssert = false)
Read performance (noAssert = true)
Read performance floats (noAssert = false)
Read performance floats (noAssert = true)
Read performance U?Int(B|L)E (noAssert = false)
Read performance U?Int(B|L)E (noAssert = true)
Fixes #18115
Fixes #12179
Fixes #8724
CI: https://ci.nodejs.org/job/node-test-pull-request/12751/CI https://ci.nodejs.org/job/node-test-pull-request/12757/
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
buffer, benchmark, test