FAST HTTP YEAH #2355
Closed
FAST HTTP YEAH #2355
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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 |
Fishrock123
added
the
semver-major
|
Tagging as major for now, remove if we can verify it isn't. :) |
brendanashworth
added
the
c++
|
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. |
indutny
force-pushed
the
feature/http-stream
branch
2 times, most recently
from
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. |
indutny
force-pushed
the
feature/http-stream
branch
from
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.
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.
@indutny Did anything need to happen here? Just asking to be sure nothing was overlooked.
There was a problem hiding this comment.
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) |
indutny
force-pushed
the
feature/http-stream
branch
from
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>
indutny
force-pushed
the
feature/http-stream
branch
from
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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
dataevents, 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
dataevents on thenet.Socket. We will also need to handle the cases where the socket is not anet.Socketinstance after all, which might appear in user-land modules.Please leave your feedback here.
cc @bnoordhuis @trevnorris @nodejs/collaborators