Change default readableStrategy HWM to 0?

[ricea]

Currently a TransformStream with the default strategies adds two chunks of buffering to any pipe. I'd like to reduce that as much as possible. We can't reduce the HWM of the writableStrategy to 0 because it would have permanent backpressure preventing the pipe from working. However, reducing the HWM of the readableStrategy to 0 will work.

Advantages:

• Reduces the amount of "bufferbloat" induced by adding an extra transform to a pipe, making backpressure propagate faster.
• Makes transform() be read-triggered by default, reducing unnecessary work. However, as soon as you pipe the readable to something transforms will start happening, so it doesn't actually make much difference.

Disadvantages:

• Breaks a surprising number of tests. I haven't looked at it in detail, but I don't think any of the breakage is fundamental. The plus side is that this will identify a set of tests whose behaviour changes with backpressure, and then we can split them into with- and without- backpressure versions.
• TransformStream ends up with a different default from ReadableStream, which may cause some surprise.

I've thought of three variations for how to treat the third argument of TransformStream:

1. undefined gets converted to {highWaterMark: 0}. Anything else is passed to the ReadableStream constructor unchanged.
2. undefined and empty objects get converted to {highWaterMark: 0}. Anything else is unchanged.
3. undefined gets converted to {highWaterMark: 0}. Anything else that doesn't already have a highWaterMark property gets one added with value 0.

I think I prefer the first one.

[domenic]

+1 to changing this. The disadvantages seem fine. I don't think we need the same defaults for readable vs. transform since we already know the readable strategy is very nich for transform streams.

The mechanics of how to do this are harder, hmm. I'm no longer convinced we're doing the right thing for non-transform streams, actually :(. With fresh eyes, I think I would have preferred that only undefined for the whole argument triggers defaults, and if it's not undefined, you have to specify both size and highWaterMark, otherwise you get errors.

Right now instead, providing undefined is the same as providing {}, and we have piece-wise defaults for size and highWaterMark. Oh well. So, how do we be consistent with that?

I think we do { size, highWaterMark } = {}, and if highWaterMark is undefined, we change it to 0 before passing it to ReadableStream. So we don't necessarily pass the exact object through to the readable stream.

Practically I think this looks like your 3.

[ricea]

I like your suggestion better so that is what I am working on.

I have run into an interesting problem with this test:

promise_test(() => {
  const ts = new TransformStream();

  const writer = ts.writable.getWriter();
  writer.close();

  return Promise.all([writer.closed, ts.readable.getReader().closed]);
}, 'TransformStream: by default, closing the writable closes the readable (when there are no queued writes)');

This test never completes with HWM 0 because the promise returned by TransformWriterDefaultSink's start() method never resolves. I think this is pretty clearly bad behaviour.

Strictly speaking, this has always been broken, but making the readable HWM default to 0 makes it a lot more obvious.

I am looking at revising the state machine so that we don't need to delay returning from start().

[ricea]

PR #786 fixes the test in my last comment.