src: refactor stream callbacks and ownership#18334
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or 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
nodejs-github-bot added c++ 
MemberAuthor
addaleax commented Jan 24, 2018
Member
jasnell commented Jan 24, 2018
Generally +1 on this but it's going to take a bit to review. ping @mcollina |
MemberAuthor
addaleax commented Jan 24, 2018
CITGM blew up with |
MemberAuthor
addaleax commented Jan 24, 2018
Pushed another commit that simplifies the passing of handles to/from child processes quite a bit. I realize that this PR might take a while to review, but I’d like to invite any @nodejs/collaborators who want to to do that and watch out for bits that aren’t immediately clear here, because a main goal of this PR is increasing readability & adding documentation to the C++ code parts. |
jasnell approved these changes Jan 24, 2018
jasnell reviewed Jan 25, 2018
src/stream_base.h Outdated
Contributor
apapirovski commented Jan 25, 2018
I've started reviewing and very much +1 on this change but won't get a chance to finish until later today or tomorrow morning. |
MemberAuthor
addaleax commented Jan 25, 2018
@apapirovski I definitely won’t land this before next week, there’s no rush. :) |
Member
jasnell commented Jan 25, 2018
The changes look very good overall (thus the sign off). I would like to see some benchmark runs. I don't anticipate much impact, but given how much code this touches, it would be worthwhile. |
Member
jasnell commented Jan 25, 2018
ha! nevermind! I missed the collapsed section in the original post :-) |
addaleaxforce-pushed the stream-listener branch from bc23a25 to 2ecac89Compare
Member
gibfahn commented Jan 25, 2018
@addaleax CitGM should be better now. |
addaleaxforce-pushed the stream-listener branch 2 times, most recently from bbd7913 to 39fb222Compare
mcollina approved these changes Jan 30, 2018
Member
mcollina left a comment
mcollina left a comment There was a problem hiding this comment.
LGTM, this is very solid work.
addaleaxforce-pushed the stream-listener branch from 39fb222 to 2bcc28dCompare
Instead of setting individual callbacks on streams and tracking stream ownership through a boolean `consume_` flag, always have one specific listener object in charge of a stream, and call methods on that object rather than generic C-style callbacks. Benchmark results show no significant changes: $ ./node benchmark/compare.js --runs 5 --new ./node --old ./node-master net | Rscript benchmark/compare.R [00:43:05|% 100| 8/8 files | 10/10 runs | 6/6 configs]: Done improvement confidence p.value net/net-c2s-cork.js dur=5 type="buf" len=1024 -0.80 % 0.720985414 net/net-c2s-cork.js dur=5 type="buf" len=128 -3.50 % 0.278786279 net/net-c2s-cork.js dur=5 type="buf" len=16 -4.44 % * 0.010458284 net/net-c2s-cork.js dur=5 type="buf" len=32 -0.51 % 0.445313528 net/net-c2s-cork.js dur=5 type="buf" len=4 -1.57 % 0.074816557 net/net-c2s-cork.js dur=5 type="buf" len=512 -0.25 % 0.926451422 net/net-c2s-cork.js dur=5 type="buf" len=64 1.66 % * 0.020469582 net/net-c2s-cork.js dur=5 type="buf" len=8 -0.18 % 0.739524856 net/net-c2s.js dur=5 type="asc" len=102400 -0.22 % 0.904819514 net/net-c2s.js dur=5 type="asc" len=16777216 0.34 % 0.862222556 net/net-c2s.js dur=5 type="buf" len=102400 -0.45 % 0.755593966 net/net-c2s.js dur=5 type="buf" len=16777216 1.87 % 0.477896886 net/net-c2s.js dur=5 type="utf" len=102400 -0.30 % 0.572739665 net/net-c2s.js dur=5 type="utf" len=16777216 1.18 % 0.369268245 net/net-pipe.js dur=5 type="asc" len=102400 1.18 % 0.368102481 net/net-pipe.js dur=5 type="asc" len=16777216 0.41 % 0.659646192 net/net-pipe.js dur=5 type="buf" len=102400 1.65 % 0.148484290 net/net-pipe.js dur=5 type="buf" len=16777216 0.05 % 0.949649889 net/net-pipe.js dur=5 type="utf" len=102400 0.65 % 0.463140117 net/net-pipe.js dur=5 type="utf" len=16777216 0.57 % 0.531757174 net/net-s2c.js dur=5 type="asc" len=102400 0.01 % 0.994663657 net/net-s2c.js dur=5 type="asc" len=16777216 0.55 % 0.690648594 net/net-s2c.js dur=5 type="buf" len=102400 1.06 % 0.162661878 net/net-s2c.js dur=5 type="buf" len=16777216 2.21 % 0.458328732 net/net-s2c.js dur=5 type="utf" len=102400 0.47 % 0.346382821 net/net-s2c.js dur=5 type="utf" len=16777216 -1.19 % 0.075676276 net/net-wrap-js-stream-passthrough.js dur=5 type="asc" len=102400 -5.01 % 0.566507367 net/net-wrap-js-stream-passthrough.js dur=5 type="asc" len=16777216 1.81 % 0.382296906 net/net-wrap-js-stream-passthrough.js dur=5 type="buf" len=102400 -4.32 % 0.543143575 net/net-wrap-js-stream-passthrough.js dur=5 type="buf" len=16777216 0.12 % 0.774690856 net/net-wrap-js-stream-passthrough.js dur=5 type="utf" len=102400 2.33 % 0.152586683 net/net-wrap-js-stream-passthrough.js dur=5 type="utf" len=16777216 0.50 % 0.687525683 net/tcp-raw-c2s.js dur=5 type="asc" len=102400 0.05 % 0.917082371 net/tcp-raw-c2s.js dur=5 type="asc" len=16777216 4.17 % ** 0.005564067 net/tcp-raw-c2s.js dur=5 type="buf" len=102400 0.56 % * 0.037673166 net/tcp-raw-c2s.js dur=5 type="buf" len=16777216 0.77 % ** 0.006890503 net/tcp-raw-c2s.js dur=5 type="utf" len=102400 -0.50 % 0.397862701 net/tcp-raw-c2s.js dur=5 type="utf" len=16777216 1.00 % 0.300638263 net/tcp-raw-pipe.js dur=5 type="asc" len=102400 0.82 % 0.722353484 net/tcp-raw-pipe.js dur=5 type="asc" len=16777216 15.00 % 0.070918075 net/tcp-raw-pipe.js dur=5 type="buf" len=102400 -1.03 % 0.819639125 net/tcp-raw-pipe.js dur=5 type="buf" len=16777216 18.35 % 0.329069149 net/tcp-raw-pipe.js dur=5 type="utf" len=102400 -0.27 % 0.984576346 net/tcp-raw-pipe.js dur=5 type="utf" len=16777216 2.78 % 0.362840470 net/tcp-raw-s2c.js dur=5 type="asc" len=102400 -0.15 % 0.820491736 net/tcp-raw-s2c.js dur=5 type="asc" len=16777216 -0.42 % 0.813160796 net/tcp-raw-s2c.js dur=5 type="buf" len=102400 0.26 % 0.615102013 net/tcp-raw-s2c.js dur=5 type="buf" len=16777216 -2.16 % 0.464289164 net/tcp-raw-s2c.js dur=5 type="utf" len=102400 -0.33 % 0.383964275 net/tcp-raw-s2c.js dur=5 type="utf" len=16777216 1.08 % 0.224603980 PR-URL: nodejs#18334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Instead of passing along the handle object, just set it as a property on the stream handle object and let the read handler grab it from there. PR-URL: nodejs#18334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
addaleaxforce-pushed the stream-listener branch from 2bcc28d to 69f3831Compareaddaleax added the author ready MemberAuthor
addaleax commented Jan 31, 2018
Fresh CI: https://ci.nodejs.org/job/node-test-commit/15821/ I’d like to land this once these come back good. |
Member
BridgeAR commented Feb 1, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 1, 2018
Instead of setting individual callbacks on streams and tracking stream ownership through a boolean `consume_` flag, always have one specific listener object in charge of a stream, and call methods on that object rather than generic C-style callbacks. PR-URL: nodejs#18334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 1, 2018
Instead of passing along the handle object, just set it as a property on the stream handle object and let the read handler grab it from there. PR-URL: nodejs#18334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
addaleax removed the author ready 3 tasks
Contributor
MylesBorins commented Feb 20, 2018
Should this be backported to |
addaleax added a commit to addaleax/node that referenced this pull request Feb 26, 2018
Instead of setting individual callbacks on streams and tracking stream ownership through a boolean `consume_` flag, always have one specific listener object in charge of a stream, and call methods on that object rather than generic C-style callbacks. PR-URL: nodejs#18334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Feb 26, 2018
Instead of passing along the handle object, just set it as a property on the stream handle object and let the read handler grab it from there. PR-URL: nodejs#18334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
addaleax added a commit that referenced this pull request Feb 26, 2018
Instead of setting individual callbacks on streams and tracking stream ownership through a boolean `consume_` flag, always have one specific listener object in charge of a stream, and call methods on that object rather than generic C-style callbacks. PR-URL: #18334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
addaleax added a commit that referenced this pull request Feb 26, 2018
Instead of passing along the handle object, just set it as a property on the stream handle object and let the read handler grab it from there. PR-URL: #18334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
addaleax added a commit that referenced this pull request Feb 26, 2018
Instead of setting individual callbacks on streams and tracking stream ownership through a boolean `consume_` flag, always have one specific listener object in charge of a stream, and call methods on that object rather than generic C-style callbacks. PR-URL: #18334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
addaleax added a commit that referenced this pull request Feb 26, 2018
Instead of passing along the handle object, just set it as a property on the stream handle object and let the read handler grab it from there. PR-URL: #18334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
Instead of setting individual callbacks on streams and tracking stream ownership through a boolean `consume_` flag, always have one specific listener object in charge of a stream, and call methods on that object rather than generic C-style callbacks. PR-URL: #18334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
Instead of passing along the handle object, just set it as a property on the stream handle object and let the read handler grab it from there. PR-URL: #18334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Closed
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Instead of setting individual callbacks on streams and tracking stream ownership through a boolean `consume_` flag, always have one specific listener object in charge of a stream, and call methods on that object rather than generic C-style callbacks. PR-URL: nodejs#18334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Instead of passing along the handle object, just set it as a property on the stream handle object and let the read handler grab it from there. PR-URL: nodejs#18334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
8 participants
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.
Instead of setting individual callbacks on streams and tracking
stream ownership through a boolean
consume_flag, always haveone specific listener object in charge of a stream, and call
methods on that object rather than generic C-style callbacks.
Benchmark results show no significant changes:
As a heads up, after this I’d like to also refactor the writable side a bit more by removing
WriteWrapandShutdownWrapas explicit classes; I have some WIP work but haven’t gotten that to pass all TLS tests yet…Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
src