FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: remove {writeableState/readableState}.length by calvinmetcalf · Pull Request #12857 · nodejs/node · GitHub

/ node Public

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

Closed
calvinmetcalf wants to merge 1 commit into
nodejs:masterfrom
calvinmetcalf:read-write-length
ConversationCommits (1)ChecksFiles changed
Closed

stream: remove {writeableState/readableState}.length#12857
calvinmetcalf wants to merge 1 commit into
nodejs:masterfrom
calvinmetcalf:read-write-length

Conversation

calvinmetcalf commented May 5, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 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 added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 5, 2017

mscdex commented May 5, 2017

Copy link
Copy Markdown
Contributor

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

Trott commented May 5, 2017

Copy link
Copy Markdown
Member

Should documentation be added for the new property?

jasnell requested changes May 7, 2017
View reviewed changes

jasnell left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented May 8, 2017

Copy link
Copy Markdown
Contributor Author

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

mcollina commented May 11, 2017

Copy link
Copy Markdown
Member

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

calvinmetcalf commented May 11, 2017

Copy link
Copy Markdown
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 commented May 17, 2017

Copy link
Copy Markdown
Member

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

calvinmetcalf commented May 17, 2017

Copy link
Copy Markdown
Contributor Author

ok I'll rebase

jasnell reviewed Jun 13, 2017
View reviewed changes
Comment thread lib/_stream_duplex.js Outdated

jasnell Jun 13, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mscdex commented Jun 13, 2017

Copy link
Copy Markdown
Contributor

Any benchmark results for this yet?

mcollina commented Jun 15, 2017

Copy link
Copy Markdown
Member

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

mcollina commented Jun 20, 2017

Copy link
Copy Markdown
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 commented Jun 20, 2017

Copy link
Copy Markdown
Contributor Author

ok let me fix the nits and rebase

calvinmetcalf commented Jun 20, 2017

Copy link
Copy Markdown
Contributor Author

ok done

mscdex commented Jun 20, 2017

Copy link
Copy Markdown
Contributor

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 commented Jun 20, 2017

Copy link
Copy Markdown
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 commented Jun 20, 2017

Copy link
Copy Markdown
Member

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

mcollina commented Jun 20, 2017

Copy link
Copy Markdown
Member

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

mcollina modified the milestone: 9.0.0 Jul 20, 2017

mcollina commented Aug 4, 2017

Copy link
Copy Markdown
Member

@mscdex are you still -1 on this?

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

mscdex commented Aug 4, 2017

Copy link
Copy Markdown
Contributor

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

addaleax removed their assignment Nov 28, 2017

addaleax commented Nov 28, 2017

Copy link
Copy Markdown
Member

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

mcollina commented Nov 28, 2017 via email

Copy link
Copy Markdown
Member

stream: rm {writeable/readable}State.length
915dab7 
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 commented Dec 18, 2017

Copy link
Copy Markdown
Member

CI before landing:

https://ci.nodejs.org/job/node-test-pull-request/12167/

mcollina commented Dec 18, 2017

Copy link
Copy Markdown
Member

Landed as 36ffa21.

mcollina closed this Dec 18, 2017
mcollina deleted the read-write-length branch December 18, 2017 13:43
mcollina pushed a commit that referenced this pull request Dec 18, 2017
stream: rm {writeable/readable}State.length
36ffa21 
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 <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
stream: rm {writeable/readable}State.length
ef56ea4 
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 <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
stream: rm {writeable/readable}State.length
c5c4a53 
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 <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins mentioned this pull request Jan 10, 2018
Merged
MylesBorins added a commit that referenced this pull request Jan 10, 2018
2019-01-10 Version 9.4.0 (Current)
074a2cb 
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
2019-01-10 Version 9.4.0 (Current)
7aefa40 
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
2019-01-10 Version 9.4.0 (Current)
e688175 
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
stream: rm {writeable/readable}State.length
85aad98 
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 <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
aliscco mentioned this pull request Apr 27, 2023
Open
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina left review comments
jasnell jasnell approved these changes
addaleax addaleax approved these changes
BridgeAR BridgeAR approved these changes
+1 more reviewer
Fishrock123 Fishrock123 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL