-
Notifications
You must be signed in to change notification settings - Fork 29k
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
net,stream: skip chunk check on incoming data #19559
Conversation
lib/_stream_readable.js
Outdated
@@ -192,9 +192,8 @@ Readable.prototype._destroy = function(err, cb) { | |||
// This returns true if the highWaterMark has not been hit yet, | |||
// similar to how Writable.write() returns true if you should | |||
// write() some more. | |||
Readable.prototype.push = function(chunk, encoding) { | |||
Readable.prototype.push = function(chunk, encoding, skipChunkCheck) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is acceptable, but I have no better ideas. If it's ok, should we document it or keep it private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is to use a special encoding
value to specify that chunk
is a buffer and should not be checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of this approach. What would likely be better is a new internal only Readable.prototype[kPush] = function(...)
that Readable.prototype.push(...)
can defer to and that our internal code can call directly.. then keep the type check in the existing push()
and skip it in the internal one.
Whatever happens here, tho, the @nodejs/streams wg needs to take a look :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The private push
would basically be just an helper for
Line 225 in f2e0288
function readableAddChunk(stream, chunk, encoding, addToFront, skipChunkCheck) { |
Can we get benchmark (at least |
No noticeable difference on Btw I've opened this only because |
lib/_stream_readable.js
Outdated
@@ -192,9 +192,8 @@ Readable.prototype._destroy = function(err, cb) { | |||
// This returns true if the highWaterMark has not been hit yet, | |||
// similar to how Writable.write() returns true if you should | |||
// write() some more. | |||
Readable.prototype.push = function(chunk, encoding) { | |||
Readable.prototype.push = function(chunk, encoding, skipChunkCheck) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of this approach. What would likely be better is a new internal only Readable.prototype[kPush] = function(...)
that Readable.prototype.push(...)
can defer to and that our internal code can call directly.. then keep the type check in the existing push()
and skip it in the internal one.
Whatever happens here, tho, the @nodejs/streams wg needs to take a look :-)
'use strict';
const common = require('../common');
const { Readable } = require('stream');
const bench = common.createBenchmark(main, {
n: [1e6]
});
function main({ n }) {
const buf = Buffer.alloc(64);
const readable = new Readable({
read() {}
});
readable.on('end', function() {
bench.end(n);
});
readable.on('resume', function() {
bench.start();
for (var i = 0; i < n; i++)
this.push(buf, undefined, true);
this.push(null);
});
readable.resume();
}
so it probably does not worth the effort. I should try on v8.x and v9.x but I'm too lazy to recompile now. |
Same benchmark on v9.x-staging:
which explains why Edit: same results on on v8.x-staging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m very conflicted by this PR. It’d be worthy running benchmarks and applying this also for HTTP (post) and TLS.
I would prefer having this as an option at creation time and documenting it.
This seems like a feature we should support and offet to our users as well.
Also, some of the benefits might come from calling push with the right number of arguments and avoid the trampoline.
I've run
Agreed but why having it as an instance option instead of a per push option?
Not sure I understand care to elaborate? |
If you define a function with 2 arguments, but you invoke it with only one, V8 will place a trampoline between the two to fill in the missing arguments. For maximum performance, we should always call
Either your code is "safe", or it's not. And also I prefer if we do not add a third parameter to |
Hey! Just for context, one of my motivations for #18537 ( I wouldn’t expect many people to call that for network sockets, but for file streams or HTTP/2 streams this seems a lot more likely, so I’ve been waiting for progress on #19060 (merging handling for I don’t know whether that conflicts with this patch, but in case it does, I just wanted to mention it. :) |
Yes, but it doesn't matter in this case, the benchmark always run with 3 arguments and the 30% improvement is consistent. |
6f7aa29
to
07477b7
Compare
@mcollina does it better align with your idea now? |
There are cases where there is no need to validate the `chunk` argument of `Readable.prototype.push()` because it is known beforehand that `chunk` is valid. Make `Readable` constructor accept a `skipChunkCheck` option to skip `chunk` validation.
07477b7
to
fe4513b
Compare
Benchmark results on v9.x-staging:
|
Do not validate data chunks read from the socket handle as they are guaranteed to be buffers and validation is costly.
fe4513b
to
359b1ae
Compare
I mean, in net it's currently called with 1 and the trampoline will be used. I'm not really concerned about the benchmarks within stream, but rather something involving net or http. The benefits should be prominent everywhere. |
@mcollina, I've just rerun Please take a look at this: https://foo-pcsvujxlnp.now.sh/ |
Can you run http simple as well? I can't see a definite benefit on all my checks. |
I think there is no point, if Feel free to close if you think there is no value. |
What it is puzzling me is why there is no impact of this in real life scenarios. Truly a 30% improvement in streams should be noticeable. |
My guess is that the gain is masked by other, heavier things.
Yes I think so but I've also tried to write an ad hoc test case where a very fast TCP client writes many small chunks as fast as possible with no luck. The chunks are coalesced into a single big TCP packet making the test useless.
By running and profiling an echo server benchmark in |
@lpinca in that case, what was the performance improvement of this change? |
@mcollina still insignificant but I think the reason why it doesn't make a difference is that only a chunk is pushed per tick? It might make sense to test with tens of thousands connected sockets, but I'm not passionate enough to do that, sorry. |
Closing as per #19559 (comment). It's easier to upgrade Node.js than adding an option that is only useful on Node.js < 10 and that we will have to support forever. |
Do not validate data chunks read from the socket handle as they are
guaranteed to be buffers and validation is costly.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes