stream: remove {writeableState/readableState}.length

[calvinmetcalf]

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

[mscdex]

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

[Trott]

Should documentation be added for the new property?

[jasnell]

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]

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

[mcollina]

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

[calvinmetcalf]

@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]

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

[calvinmetcalf]

ok I'll rebase

[jasnell]

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

[mscdex]

Any benchmark results for this yet?

[mcollina]

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

[mcollina]

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]

ok let me fix the nits and rebase

[calvinmetcalf]

ok done

[mscdex]

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]

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]

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

[mcollina]

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

[mcollina]

@mscdex are you still -1 on this?

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

[mscdex]

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

[addaleax]

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

[mcollina]

I can do this in a week, or if someone is willing to help please go ahead and land. Il giorno mar 28 nov 2017 alle 08:02 Anna Henningsen < notifications@github.com> ha scritto:

…

@calvinmetcalf <https://github.com/calvinmetcalf> Do you think you could rebase this? Otherwise I guess one of us could do that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12857 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADL46E03AzAdukGrmEWyFKEqE79BJ5Bks5s62KrgaJpZM4NR4Cu> .

[mcollina]

CI before landing:

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

[mcollina]

Landed as 36ffa21.