-
Notifications
You must be signed in to change notification settings - Fork 30k
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
http2: Abstracting stream and session-related logic #33928
Conversation
e145c8a
to
aba2d82
Compare
@nodejs/http2 |
The changes didn't have much of an impact on performance.
|
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.
lgtm
The node/src/node_native_module.cc Lines 91 to 99 in 56967af
|
Not sure if this PR is the culprit but the benchmark above failed (the prev benchmark run for this PR failed with the same error). 19:04:44 ++ ./node-master benchmark/compare.js --old ./node-master --new ./node-pr -- http2
19:04:44 ++ tee output190620-170444.csv
19:04:44 "binary", "filename", "configuration", "rate", "time"
19:04:49 "old", "http2/compat.js", "duration=5 benchmarker='test-double-http2' clients=2 streams=1 requests=100", 3522, 5.25998366
19:04:55 "old", "http2/compat.js", "duration=5 benchmarker='test-double-http2' clients=2 streams=10 requests=100", 2186, 5.219020456
19:05:01 "old", "http2/compat.js", "duration=5 benchmarker='test-double-http2' clients=2 streams=20 requests=100", 2112, 5.223423342
19:05:06 "old", "http2/compat.js", "duration=5 benchmarker='test-double-http2' clients=2 streams=40 requests=100", 2515, 5.221362605
19:05:12 "old", "http2/compat.js", "duration=5 benchmarker='test-double-http2' clients=2 streams=100 requests=100", 2914, 5.244409724
19:05:17 "old", "http2/compat.js", "duration=5 benchmarker='test-double-http2' clients=2 streams=200 requests=100", 3372, 5.197587316
19:05:23 "old", "http2/compat.js", "duration=5 benchmarker='test-double-http2' clients=2 streams=1 requests=1000", 2310, 5.234376489
19:05:28 "old", "http2/compat.js", "duration=5 benchmarker='test-double-http2' clients=2 streams=10 requests=1000", 2298, 5.228407984
19:05:34 "old", "http2/compat.js", "duration=5 benchmarker='test-double-http2' clients=2 streams=20 requests=1000", 2122, 5.208801167
19:05:39 "old", "http2/compat.js", "duration=5 benchmarker='test-double-http2' clients=2 streams=40 requests=1000", 2128, 5.223415788
19:05:44 /w/bnch-comp/node/benchmark/_test-double-benchmarker.js:40
19:05:44 client.on('error', (e) => { throw e; });
19:05:44 ^
19:05:44
19:05:44 Error: connect EADDRNOTAVAIL 127.0.0.1:12346 - Local (127.0.0.1:0)
19:05:44 at internalConnect (net.js:904:16)
19:05:44 at defaultTriggerAsyncIdScope (internal/async_hooks.js:416:12)
19:05:44 at net.js:992:9
19:05:44 at processTicksAndRejections (internal/process/task_queues.js:79:11) {
19:05:44 errno: -99,
19:05:44 code: 'EADDRNOTAVAIL',
19:05:44 syscall: 'connect',
19:05:44 address: '127.0.0.1',
19:05:44 port: 12346
19:05:44 }
19:05:44 Error: test-double-http2 failed with 1.
19:05:44 at ChildProcess.<anonymous> (/w/bnch-comp/node/benchmark/_http-benchmarkers.js:238:16)
19:05:44 at Object.onceWrapper (events.js:421:26)
19:05:44 at ChildProcess.emit (events.js:314:20)
19:05:44 at maybeClose (internal/child_process.js:1051:16)
19:05:44 at Socket.<anonymous> (internal/child_process.js:442:11)
19:05:44 at Socket.emit (events.js:314:20)
19:05:44 at Pipe.<anonymous> (net.js:655:12)
19:05:44 ++ cat output190620-170444.csv
19:05:44 ++ Rscript benchmark/compare.R
19:05:45 confidence improvement accuracy (*) (**) (***)
19:05:45 http2/compat.js duration=5 benchmarker='test-double-http2' clients=2 streams=100 requests=100 NA NaN % NA NA NA
19:05:45 http2/compat.js duration=5 benchmarker='test-double-http2' clients=2 streams=10 requests=100 NA NaN % NA NA NA
19:05:45 http2/compat.js duration=5 benchmarker='test-double-http2' clients=2 streams=10 requests=1000 NA NaN % NA NA NA
19:05:45 http2/compat.js duration=5 benchmarker='test-double-http2' clients=2 streams=1 requests=100 NA NaN % NA NA NA
19:05:45 http2/compat.js duration=5 benchmarker='test-double-http2' clients=2 streams=1 requests=1000 NA NaN % NA NA NA
19:05:45 http2/compat.js duration=5 benchmarker='test-double-http2' clients=2 streams=200 requests=100 NA NaN % NA NA NA
19:05:45 http2/compat.js duration=5 benchmarker='test-double-http2' clients=2 streams=20 requests=100 NA NaN % NA NA NA
19:05:45 http2/compat.js duration=5 benchmarker='test-double-http2' clients=2 streams=20 requests=1000 NA NaN % NA NA NA
19:05:45 http2/compat.js duration=5 benchmarker='test-double-http2' clients=2 streams=40 requests=100 NA NaN % NA NA NA
19:05:45 http2/compat.js duration=5 benchmarker='test-double-http2' clients=2 streams=40 requests=1000 NA NaN % NA NA NA |
@lundibundi Maybe we can run the benchmark again, I've run it locally (OSX) multiple times with no errors. |
Pulling the author ready label off until the possible issues are explored |
@lundibundi There seems to be a problem with the benchmark on master, I only run the latest master benchmark ci and it gives me this error ./node benchmark/compare.js --old ./node-master --new ./node-master -- http2 |
ping @jasnell Do we need to wait for the benchmark CI to be fixed before we can land this PR? |
ping @jasnell |
@lundibundi It looks like the master problem is fixed. Can you please start the benchmark CI again? |
Very strange, I run normally on macOS, I find a Linux to try. |
e558800
to
241d587
Compare
4a344c3
to
02c8610
Compare
This PR is at an impasse, waiting for #34598 |
This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open. |
Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions. |
Take session and strems-related logic out of core.js for easier coding (every time I try to make changes to the logic for http2 I see that the file is too big and I can't do anything about it).
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes