Skip to content
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

stream: remove {writeableState/readableState}.length #12857

Closed
wants to merge 1 commit into from

Conversation

calvinmetcalf
Copy link
Contributor

@calvinmetcalf calvinmetcalf commented May 5, 2017

As part of the readableState/writableState mega issue #445, this
removes all of the references to .length on those properties and
replaces them with a readLength and writeLength getter.

cc @nodejs/streams

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

streams, tls, net

@calvinmetcalf calvinmetcalf added net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem. tls Issues and PRs related to the tls subsystem. labels May 5, 2017
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 5, 2017
@mscdex
Copy link
Contributor

mscdex commented May 5, 2017

As with #12860, is performance impacted by these changes?

@Trott
Copy link
Member

Trott commented May 5, 2017

Should documentation be added for the new property?

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual code change looks fine to me. I agree with @mscdex that a benchmark run would be good so we can see the perf impact, and @Trott is right that this needs docs added.

@calvinmetcalf
Copy link
Contributor Author

do we want to document these properties? would it make sense to underscore prefix them and not publicly document them?

@mcollina
Copy link
Member

I will run the benchmarks next week when I am back.

@calvinmetcalf
Copy link
Contributor Author

@mcollina great thanks! we don't have a rush on this one, it's been open for a while so we should fix it right.

@mcollina
Copy link
Member

@calvinmetcalf can you rebase? It's failing if I try to apply this.

@calvinmetcalf
Copy link
Contributor Author

ok I'll rebase

@@ -61,6 +61,13 @@ function Duplex(options) {
this.once('end', onend);
}


Object.defineProperty(Duplex.prototype, 'writeLength', {
get: function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: these could just be get() { ... }

@mscdex
Copy link
Contributor

mscdex commented Jun 13, 2017

Any benchmark results for this yet?

@mcollina
Copy link
Member

@mscdex my fault. I've been busy and I did not have time. I'll get things done next week.

@mcollina
Copy link
Member

I have pulled this on top of current master, and here are the result:

$ cat compare-pr-12857.csv | Rscript benchmark/compare.R
                                                   improvement confidence    p.value
 net/net-c2s-cork.js dur=5 type="buf" len=1024          0.05 %            0.92585453
 net/net-c2s-cork.js dur=5 type="buf" len=128           0.01 %            0.98394598
 net/net-c2s-cork.js dur=5 type="buf" len=16            0.11 %            0.64262178
 net/net-c2s-cork.js dur=5 type="buf" len=32           -0.63 %            0.49813748
 net/net-c2s-cork.js dur=5 type="buf" len=4             0.69 %            0.22906509
 net/net-c2s-cork.js dur=5 type="buf" len=512          -0.68 %            0.11614061
 net/net-c2s-cork.js dur=5 type="buf" len=64           -0.10 %            0.88165041
 net/net-c2s-cork.js dur=5 type="buf" len=8             0.22 %            0.49318844
 net/net-c2s.js dur=5 type="asc" len=102400             0.12 %            0.79652780
 net/net-c2s.js dur=5 type="asc" len=16777216           0.17 %            0.69953088
 net/net-c2s.js dur=5 type="buf" len=102400            -0.03 %            0.95501291
 net/net-c2s.js dur=5 type="buf" len=16777216          -0.18 %            0.60032466
 net/net-c2s.js dur=5 type="utf" len=102400            -0.71 %            0.30724140
 net/net-c2s.js dur=5 type="utf" len=16777216           0.13 %            0.71738184
 net/net-pipe.js dur=5 type="asc" len=102400           -0.38 %            0.47130806
 net/net-pipe.js dur=5 type="asc" len=16777216         -0.50 %            0.22687420
 net/net-pipe.js dur=5 type="buf" len=102400            0.15 %            0.73656293
 net/net-pipe.js dur=5 type="buf" len=16777216          0.34 %            0.34467836
 net/net-pipe.js dur=5 type="utf" len=102400           -0.13 %            0.76703831
 net/net-pipe.js dur=5 type="utf" len=16777216         -0.06 %            0.82737525
 net/net-s2c.js dur=5 type="asc" len=102400             0.37 %            0.34771981
 net/net-s2c.js dur=5 type="asc" len=16777216           1.08 %          * 0.01600552
 net/net-s2c.js dur=5 type="buf" len=102400            -0.10 %            0.76992876
 net/net-s2c.js dur=5 type="buf" len=16777216          -0.01 %            0.97334494
 net/net-s2c.js dur=5 type="utf" len=102400             0.44 %            0.53842239
 net/net-s2c.js dur=5 type="utf" len=16777216           0.16 %            0.63315209
 net/tcp-raw-c2s.js dur=5 type="asc" len=102400        -0.20 %            0.67051959
 net/tcp-raw-c2s.js dur=5 type="asc" len=16777216       0.21 %            0.30793474
 net/tcp-raw-c2s.js dur=5 type="buf" len=102400        -0.29 %            0.28911115
 net/tcp-raw-c2s.js dur=5 type="buf" len=16777216      -0.04 %            0.86124577
 net/tcp-raw-c2s.js dur=5 type="utf" len=102400        -0.30 %            0.57333080
 net/tcp-raw-c2s.js dur=5 type="utf" len=16777216      -0.18 %            0.60026095
 net/tcp-raw-pipe.js dur=5 type="asc" len=102400        1.06 %            0.77474299
 net/tcp-raw-pipe.js dur=5 type="asc" len=16777216      3.02 %            0.34301122
 net/tcp-raw-pipe.js dur=5 type="buf" len=102400       -6.20 %            0.11493755
 net/tcp-raw-pipe.js dur=5 type="buf" len=16777216      0.53 %            0.86680583
 net/tcp-raw-pipe.js dur=5 type="utf" len=102400        4.27 %            0.26626613
 net/tcp-raw-pipe.js dur=5 type="utf" len=16777216     -0.77 %            0.80117040
 net/tcp-raw-s2c.js dur=5 type="asc" len=102400         0.70 %            0.10070950
 net/tcp-raw-s2c.js dur=5 type="asc" len=16777216       0.36 %            0.06191144
 net/tcp-raw-s2c.js dur=5 type="buf" len=102400         0.05 %            0.91598591
 net/tcp-raw-s2c.js dur=5 type="buf" len=16777216       0.05 %            0.82678721
 net/tcp-raw-s2c.js dur=5 type="utf" len=102400         0.60 %            0.32760427
 net/tcp-raw-s2c.js dur=5 type="utf" len=16777216      -0.21 %            0.28876196

IMHO we can land this, there are no regressions.

@calvinmetcalf
Copy link
Contributor Author

ok let me fix the nits and rebase

@calvinmetcalf
Copy link
Contributor Author

ok done

@mscdex
Copy link
Contributor

mscdex commented Jun 20, 2017

Can we use 'readable' and 'writable' as the prefixes instead of just 'read' and 'write' respectively, to further reduce the likelihood of a clash with userland?

I'm still -1 on adding all of these individual properties to everyone's streams. I would rather see something like an object housing all of these...

@mcollina
Copy link
Member

Currently we are creating an accessory object (the state) for every Readable and Writable, and two for Duplex (and descendants). This is already a bottleneck, so I'm -1 on adding a new object for both Readable and Writable.

BTW, I'm fine with documenting the _ properties, and/or remapping those.

@jasnell
Copy link
Member

jasnell commented Jun 20, 2017

If we're going to document the existing ones, I'd prefer to get rid of the _ prefix.

@mcollina
Copy link
Member

@jasnell that is not feasible either, as everyone is using those _ properties.

@mcollina mcollina modified the milestone: 9.0.0 Jul 20, 2017
@mcollina
Copy link
Member

mcollina commented Aug 4, 2017

@mscdex are you still -1 on this?

This might require some more reviews/approvals.. @nodejs/ctc as we discussed in the last meeting.

@mscdex
Copy link
Contributor

mscdex commented Aug 4, 2017

@mcollina Yes, I am still -1 in general to these and the related changes from other PRs.

@addaleax
Copy link
Member

@calvinmetcalf Do you think you could rebase this? Otherwise I guess one of us could do that.

@mcollina
Copy link
Member

mcollina commented Nov 28, 2017 via email

As part of the readableState/writableState mega issue nodejs#445, this
removes all of the references to .length on those properties and
replaces them with a readableLength and writableLength getter.
@mcollina
Copy link
Member

@mcollina
Copy link
Member

Landed as 36ffa21.

@mcollina mcollina closed this Dec 18, 2017
@mcollina mcollina deleted the read-write-length branch December 18, 2017 13:43
mcollina pushed a commit that referenced this pull request Dec 18, 2017
As part of the readableState/writableState mega issue #445, this
removes all of the references to .length on those properties and
replaces them with a readableLength and writableLength getter.

See: #445
PR-URL: #12857
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
As part of the readableState/writableState mega issue #445, this
removes all of the references to .length on those properties and
replaces them with a readableLength and writableLength getter.

See: #445
PR-URL: #12857
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
As part of the readableState/writableState mega issue #445, this
removes all of the references to .length on those properties and
replaces them with a readableLength and writableLength getter.

See: #445
PR-URL: #12857
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
MylesBorins added a commit that referenced this pull request Jan 10, 2018
Notable change:

* async_hooks:
  - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
    API were documented. (Andreas Madsen)
    #16972
* deps:
  - update nghttp2 to 1.29.0 (James M Snell)
    #17908
  - upgrade npm to 5.6.0 (Kat Marchán)
    #17535
  - cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
    #16591
* events:
  - remove reaches into _events internals (Anatoli Papirovski)
    #17440
* http:
  - add rawPacket in err of `clientError` event (XadillaX)
    #17672
* http2:
  - implement maxSessionMemory (James M Snell)
    #17967
  - add initial support for originSet (James M Snell)
    #17935
  - add altsvc support (James M Snell)
    #17917
  - perf_hooks integration (James M Snell)
    #17906
* net:
  - remove Socket.prototype.write (Anna Henningsen)
    #17644
  - remove Socket.prototype.listen (Ruben Bridgewater)
    #13735
* repl:
  - show lexically scoped vars in tab completion (Michaël Zasso)
    #16591
* stream:
  - rm {writeable/readable}State.length (Calvin Metcalf)
    #12857
  - add flow and buffer properties to streams (Calvin Metcalf)
    #12855
* util:
  - allow wildcards in NODE_DEBUG variable (Tyler)
    #17609
* zlib:
  - add ArrayBuffer support (Jem Bezooyen)
    #16042
* Addedew collaborator**
  - [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
  - [danbev](https://github.com/danbev) Daniel Bevenius

PR-URL: #18069
MylesBorins added a commit that referenced this pull request Jan 10, 2018
Notable change:

* async_hooks:
  - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
    API were documented. (Andreas Madsen)
    #16972
* deps:
  - update nghttp2 to 1.29.0 (James M Snell)
    #17908
  - upgrade npm to 5.6.0 (Kat Marchán)
    #17535
  - cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
    #16591
* events:
  - remove reaches into _events internals (Anatoli Papirovski)
    #17440
* http:
  - add rawPacket in err of `clientError` event (XadillaX)
    #17672
* http2:
  - implement maxSessionMemory (James M Snell)
    #17967
  - add initial support for originSet (James M Snell)
    #17935
  - add altsvc support (James M Snell)
    #17917
  - perf_hooks integration (James M Snell)
    #17906
  - Refactoring and cleanup of Http2Session and Http2Stream destroy
    (James M Snell) #17406
* net:
  - remove Socket.prototype.write (Anna Henningsen)
    #17644
  - remove Socket.prototype.listen (Ruben Bridgewater)
    #13735
* repl:
  - show lexically scoped vars in tab completion (Michaël Zasso)
    #16591
* stream:
  - rm {writeable/readable}State.length (Calvin Metcalf)
    #12857
  - add flow and buffer properties to streams (Calvin Metcalf)
    #12855
* util:
  - allow wildcards in NODE_DEBUG variable (Tyler)
    #17609
* zlib:
  - add ArrayBuffer support (Jem Bezooyen)
    #16042
* Addedew collaborator**
  - [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
  - [danbev](https://github.com/danbev) Daniel Bevenius

PR-URL: #18069
MylesBorins added a commit that referenced this pull request Jan 10, 2018
Notable change:

* async_hooks:
  - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
    API were documented. (Andreas Madsen)
    #16972
* deps:
  - update nghttp2 to 1.29.0 (James M Snell)
    #17908
  - upgrade npm to 5.6.0 (Kat Marchán)
    #17535
  - cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
    #16591
* events:
  - remove reaches into _events internals (Anatoli Papirovski)
    #17440
* http:
  - add rawPacket in err of `clientError` event (XadillaX)
    #17672
* http2:
  - implement maxSessionMemory (James M Snell)
    #17967
  - add initial support for originSet (James M Snell)
    #17935
  - add altsvc support (James M Snell)
    #17917
  - perf_hooks integration (James M Snell)
    #17906
  - Refactoring and cleanup of Http2Session and Http2Stream destroy
    (James M Snell) #17406
* net:
  - remove Socket.prototype.write (Anna Henningsen)
    #17644
  - remove Socket.prototype.listen (Ruben Bridgewater)
    #13735
* repl:
  - show lexically scoped vars in tab completion (Michaël Zasso)
    #16591
* stream:
  - rm {writeable/readable}State.length (Calvin Metcalf)
    #12857
  - add flow and buffer properties to streams (Calvin Metcalf)
    #12855
* util:
  - allow wildcards in NODE_DEBUG variable (Tyler)
    #17609
* zlib:
  - add ArrayBuffer support (Jem Bezooyen)
    #16042
* Addedew collaborator**
  - [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
  - [danbev](https://github.com/danbev) Daniel Bevenius

PR-URL: #18069
boingoing pushed a commit to nodejs/node-chakracore that referenced this pull request Jan 18, 2018
As part of the readableState/writableState mega issue #445, this
removes all of the references to .length on those properties and
replaces them with a readableLength and writableLength getter.

See: nodejs/node#445
PR-URL: nodejs/node#12857
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. net Issues and PRs related to the net subsystem. semver-minor PRs that contain new features and should be released in the next minor version. stream Issues and PRs related to the stream subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.