stream: avoid possible slow paths w UInt8Array #13956
Conversation
nodejs-github-bot
added
the
stream
|
Have you ran benchmarks to check the effects of this? I would think non-object mode streams are more common than object mode streams and this could negatively non-object mode streams because the |
This is mostly related to readable-stream, after the whole transpiling process. In Node core, things should be equivalent. I discovered some of those while hunting for nodejs/readable-stream#304.
All conditions are at |
lib/_stream_readable.js
Outdated
| chunk !== undefined && | ||
| !state.objectMode) { | ||
| !state.objectMode && | ||
| !Stream._isUint8Array(chunk)) { |
There was a problem hiding this comment.
@mscdex are you referring to this one?
There was a problem hiding this comment.
You are right about this, I will need to check if this does not negatively effect benchmarks.
| @@ -248,7 +248,7 @@ function validChunk(stream, state, chunk, cb) { | |||
| Writable.prototype.write = function(chunk, encoding, cb) { | |||
| var state = this._writableState; | |||
| var ret = false; | |||
| var isBuf = Stream._isUint8Array(chunk) && !state.objectMode; | |||
| var isBuf = !state.objectMode && Stream._isUint8Array(chunk); | |||
There was a problem hiding this comment.
@mscdex this is equivalent.
There was a problem hiding this comment.
In my mind this is surely going to be be faster when !state.objectMode.
A chunk validity checks verifie if a chunk is a UInt8Array. We should defer it as it might be very expensive in older Node.js platforms.
mcollina
force-pushed
the
change-boolean-checks
branch
from
4f6fa5c to
e66fe47
Compare
|
@mscdex I've updated the problematic part, and just left with the change on |
|
LGTM now. |
|
Landed as 0279602 |
A chunk validity checks verifie if a chunk is a UInt8Array. We should defer it as it might be very expensive in older Node.js platforms. PR-URL: #13956 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
A chunk validity checks verifie if a chunk is a UInt8Array. We should defer it as it might be very expensive in older Node.js platforms. PR-URL: nodejs#13956 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
A chunk validity checks verifie if a chunk is a UInt8Array. We should defer it as it might be very expensive in older Node.js platforms. PR-URL: #13956 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
A chunk validity checks verifie if a chunk is a UInt8Array. We should defer it as it might be very expensive in older Node.js platforms. PR-URL: #13956 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
|
@mcollina I'm pretty sure this doesn't apply to v6.x the check is way different. var isBuf = (chunk instanceof Buffer);Can you add |
|
@MylesBorins this should not be backported. |
A chunk validity checks verifie if a chunk is a UInt8Array.
We should defer it as it might be very expensive in older Node.js
platforms.
It avoid the UInt8Array checks if the streams are in
objectMode: true.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
stream