stream: augment BufferList.prototype

[lpinca]

For SoC/readability and to facilitate refactoring if the BufferList underlying data structure is changed.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

stream

[lpinca]

[luigi@fedora node]$ cat compare.csv | Rscript benchmark/compare.R improvement confidence p.value
streams/readable-bigread.js n=1000 -0.18 % 5.909677e-01
streams/readable-bigunevenread.js n=1000 1.19 % *** 5.431321e-04
streams/readable-boundaryread.js type="buffer" n=2000 -0.20 % 5.733284e-01
streams/readable-boundaryread.js type="string" n=2000 4.16 % *** 2.807485e-08
streams/readable-readall.js n=5000 0.35 % 7.158884e-01
streams/readable-unevenread.js n=1000 1.34 % *** 1.184393e-07
streams/transform-creation.js n=1000000 1.32 % 1.789189e-01
streams/writable-manywrites.js n=2000000 4.14 % *** 3.774978e-09

Not sure where the 4% improvement of streams/writable-manywrites.js comes from since this doesn't touch writable.

[apapirovski]

LGTM but can we either hide _getString & _getBuffer behind a symbol or make them helper functions (that accept the class instance as first argument)? I don't want to accidentally end up in a universe where someone starts using those methods and we have to support them.

(I would almost lean towards them being helper functions since there's only one place they're used.)

[lpinca]

@apapirovski I didn't use symbols because that is an internal module.

[apapirovski]

@lpinca The problem is that we can access BufferList instances via stream internals which are exposed, right? (I'm not saying it's smart to do so but I would just prefer to even not have a chance of ending up in that situation. That said, if no one else cares then I'm fine with this staying as is – hence my approval.)

[lpinca]

Yes but by that logic all BufferList methods should be helper functions as the user should not use them. I mean the same argument is also valid for shift, clear and direct access to BufferList properties.

[apapirovski]

@lpinca true enough. 👍 It was probably the _ in the name that made me start thinking along this path.

(To be clear: I'm fine with this remaining as is. You're completely right.)

[BridgeAR]

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

[BridgeAR]

Landed in ccf64e5

[MylesBorins]

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.