src: refactor stream callbacks and ownership by addaleax · Pull Request #18334 · nodejs/node

addaleax · 2018-01-24T00:16:10Z

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

As a heads up, after this I’d like to also refactor the writable side a bit more by removing WriteWrap and ShutdownWrap as explicit classes; I have some WIP work but haven’t gotten that to pass all TLS tests yet…

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

src

addaleax · 2018-01-24T00:17:36Z

CI: https://ci.nodejs.org/job/node-test-commit/15636/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1222/

jasnell · 2018-01-24T00:37:00Z

Generally +1 on this but it's going to take a bit to review.

ping @mcollina

addaleax · 2018-01-24T19:15:58Z

CITGM blew up with ENOSPC on AIX but other than that I think we’re good. (Will run CITGM again at a later point to make sure).

addaleax · 2018-01-24T23:51:06Z

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 · 2018-01-25T00:00:36Z

extreme nit: s/Node's/Node.js'

apapirovski · 2018-01-25T00:04:39Z

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.

addaleax · 2018-01-25T00:05:40Z

@apapirovski I definitely won’t land this before next week, there’s no rush. :)

jasnell · 2018-01-25T00:13:49Z

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.

jasnell · 2018-01-25T00:15:24Z

ha! nevermind! I missed the collapsed section in the original post :-)

gibfahn · 2018-01-25T07:36:50Z

@addaleax CitGM should be better now.

apapirovski

LGTM 👍

mcollina

LGTM, this is very solid work.

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 <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

addaleax · 2018-01-31T14:34:19Z

Fresh CI: https://ci.nodejs.org/job/node-test-commit/15821/
Fresh CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1246/

I’d like to land this once these come back good.

BridgeAR · 2018-02-01T09:54:44Z

Landed in 7c4b09b, 5898dc3

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 <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins · 2018-02-20T23:20:19Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

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 <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

addaleax requested a review from apapirovski January 24, 2018 00:16

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 24, 2018

jasnell approved these changes Jan 24, 2018
View reviewed changes

jasnell reviewed Jan 25, 2018
View reviewed changes

Comment threadsrc/stream_base.h Outdated
jasnellJan 25, 2018
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extreme nit: s/Node's/Node.js'

addaleax force-pushed the stream-listener branch from bc23a25 to 2ecac89Compare January 25, 2018 02:43

maclover7 force-pushed the master branch from bb5575a to 993b716Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952Compare January 26, 2018 22:36

addaleax force-pushed the stream-listener branch 2 times, most recently from bbd7913 to 39fb222Compare January 29, 2018 02:09

apapirovski approved these changes Jan 29, 2018
View reviewed changes

mcollina approved these changes Jan 30, 2018
View reviewed changes

addaleax force-pushed the stream-listener branch from 39fb222 to 2bcc28dCompare January 31, 2018 14:25

addaleax added 2 commits January 31, 2018 15:26

addaleax force-pushed the stream-listener branch from 2bcc28d to 69f3831Compare January 31, 2018 14:28

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 31, 2018

BridgeAR closed this Feb 1, 2018

addaleax deleted the stream-listener branch February 1, 2018 17:12

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2018

addaleax mentioned this pull request Feb 10, 2018
src: refactor WriteWrap and ShutdownWrap #18676
Closed
3 tasks

MylesBorins added the backport-requested-v9.x label Feb 20, 2018

addaleax removed the backport-requested-v9.x label Feb 26, 2018

addaleax mentioned this pull request Feb 27, 2018
v9.7.0 proposal #19040
Closed

addaleax added dont-land-on-v6.x labels Mar 28, 2018

Uh oh!

Uh oh!

Conversation

addaleax commented Jan 24, 2018

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Jan 24, 2018

Uh oh!

jasnell commented Jan 24, 2018

Uh oh!

addaleax commented Jan 24, 2018

Uh oh!

addaleax commented Jan 24, 2018

Uh oh!

jasnellJan 25, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Jan 25, 2018

Uh oh!

addaleax commented Jan 25, 2018

Uh oh!

jasnell commented Jan 25, 2018

Uh oh!

jasnell commented Jan 25, 2018

Uh oh!

gibfahn commented Jan 25, 2018

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jan 31, 2018

Uh oh!

BridgeAR commented Feb 1, 2018

Uh oh!

MylesBorins commented Feb 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants