-
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
FAST HTTP YEAH #2355
FAST HTTP YEAH #2355
Conversation
could we keep backward compat by checking for |
Yeah, this might break my |
@rvagg I think we can do it. Though, we will need to do it by replacing In fact it does start emitting |
@mscdex even more, it will break node-spdy as well :) Let's figure out how to do it with least possible consequences |
Tagging as major for now, remove if we can verify it isn't. :) |
Here is the results of benchmarks (number of runs=10):
|
Looks pretty mixed to me. @trevnorris may I ask you to help me nail down the bottleneck of -40% benchmarks? I'm quite sure that it might be related to the lazy-allocation of buffer, but still not exactly sure why it takes so much time. |
Nah, I think the was some problem with benchmarks. Didn't see this -40% oddballs locally, I assume it should be visible even on my machine. I can only blame DigitalOcean for this. |
0931623
to
f2efe5c
Compare
I think this PR is finished at the moment, PTAL |
ping @trevnorris @bnoordhuis @nodejs/crypto @nodejs/collaborators |
skipping the data events makes me a bit nervous as we do not have a reliable way of knowing all the parts of the ecosystem this could impact but in general this SGTM and I don't see anything in the commits that raise any red flags. We should make sure to extensively sniff test this tho. |
f6ebf5f
to
c8af6d4
Compare
private: | ||
static void Consume(const FunctionCallbackInfo<Value>& args) { | ||
Parser* parser = Unwrap<Parser>(args.Holder()); | ||
Local<External> stream_obj = args[0].As<External>(); |
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.
Checking if the object is external from JS isn't possible. I'd feel more comfortable with a CHECK(args[0]->IsExternal());
just above.
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.
@indutny Did anything need to happen here? Just asking to be sure nothing was overlooked.
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.
Argh, I did it below... and forgot this place. Looks like it will happen sometime later.
Don't see anything else. If CI is happy then LGTM. (as you noted, the logic here is crazy to follow. sorry for all the unnecessary questions) |
5b98382
to
cb3306a
Compare
Looks like CI is not exactly happy with this yet. Failing tests:
Other failures does not seem to be related. |
Consume StreamBase instance and operate on incoming data directly without allocating Buffer instances. Improves performance. PR-URL: nodejs#2355 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
cb3306a
to
59b91f1
Compare
Fixed. |
Is pipeline-flood generally considered to be flaky? I was running it in the loop for 30 minutes and it never hanged. |
Depends on: #2351
I just want to kick off the discussion of where this path may lead us. With this change http Server will consume all data from the net.Socket, skipping its
data
events, and the server will parse this data directly without going to JS-land and without allocating Buffer instances.From my local benchmarks (simple server, GET requests):
I'm yet to run our benchmarks on some server (or my laptop) to see what general effects this patch has on everything, but I think it should be mostly an improvement, not degradation.
The downside of this thing is absence of
data
events on thenet.Socket
. We will also need to handle the cases where the socket is not anet.Socket
instance after all, which might appear in user-land modules.Please leave your feedback here.
cc @bnoordhuis @trevnorris @nodejs/collaborators