stream: implement WritableBase without buffering

[ronag]

This separates the Writable class into two classes a WritableBase class which implements all Writable logic except for anything related to buffering, and a Writable class which inherits WritableBase and adds the buffering logic.

This is in order to be able to consistently and in a performant way implement Writable streams that want to implement their own buffering or are simply wrapping existing streams.

WritableBase expects 2 methods passed through options:

• write: which is basically the writeOrBuffer implementation from today.
• flush: which is basically end() + wait for completion from today.

Note, that this is for now only for internal core usage not meant for public API.

 streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='no' n=1000000                  -0.94 %       ±6.14% ±8.16% ±10.63%
 streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='yes' n=1000000                  5.76 %       ±5.92% ±7.88% ±10.26%
 streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='no' n=1000000                  2.47 %       ±7.08% ±9.42% ±12.27%
 streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='yes' n=1000000                 4.11 %       ±6.17% ±8.21% ±10.69%

This will help with making Writablelike implementations where we want to avoid buffering or sequential writes more streams compliant, e.g. OutgoingMessage.

It will also help with some optimization e.g. avoid buffering both input and output of Transform stream by implementing it in terms of Readable + WritableBase instead of Readable + Writable.

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

[ronag]

This is very much WIP but I would like some initial feedback whether this is worth further engagement.

[ronag]

@nodejs/streams

[ronag]

extends WritableBase.WritableState

[ronag]

Might also be useful for the QUIC work.

[mcollina]

I really like the direction this is going, however I would prefer to land this together with the changes in the other places it is targeting.

I suspect you want to try making http.OutgoingMessage and Transform inheriting from WritableBase. I would prefer to see if those changes are feasible before landing this.

[ronag]

I really like the direction this is going, however I would prefer to land this together with the changes in the other places it is targeting.

Good. I just wanted to make sure this is worth spending more time on. I'll start with Transform and #33397.

[mcollina]

(Keep them on separate commits, so that we can easily split it up if we need to for ease of backporting).

[nodejs-github-bot]

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

[ronag]

@mcollina: This kind of works now... Still has some details I would like to improve upon. However, when trying to use it to implement Transform or OutgoingMessage it quickly comes to a point where it is significantly breaking.

• Transform wants to buffer pre transform (since it has cork/uncork) but it wants to invoke the callback post transform. Having both at the same time kind of requires double buffering. Unless we are prepared to remove the writable buffering API from Transform (writableLength, cork/uncork, writableBuffer), i.e. only buffer on readable side.

• OutgoingMessage is still too far from a "real" stream to make it practical, e.g. http: align OutgoingMessage.writable with streams #33655 and much more. ClientRequest further complicates things. Would maybe make sense to totally remove OutgoingMessage from ClientRequest and reimplement as a first step. However, that's a lot of work and I would probably prefer to add a new and better client api.

Maybe @jasnell would have use of this for quic... not sure where to take it from here. Maybe put on ice for now?

[mcollina]

Given all the above, I would put on ice. Unless @jasnell (or somebody else) has a good use for this.