Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port precise flow control to writable streams #318

Closed
domenic opened this issue Apr 7, 2015 · 11 comments
Closed

Port precise flow control to writable streams #318

domenic opened this issue Apr 7, 2015 · 11 comments

Comments

@domenic
Copy link
Member

domenic commented Apr 7, 2015

After #317, readable streams will have precise flow control. We should add something similar to writable streams. However, the shape of it will likely depend on the resolution of #316.

@domenic
Copy link
Member Author

domenic commented Aug 20, 2015

Questions:

What should we expose?

I see two choices:

  • queuedSize = total size in the queue.
  • desiredSize = high water mark minus total size in the queue. Can be negative or zero. Represents how much you should write right now.

How should the ready signal work?

A few possibilities:

  1. The current ready signal, which fulfills when queuedSize dips below the high water mark (or desiredSize goes above zero).
  2. A different signal, call it drained, which fulfills when queuedSize reaches zero (or desiredSize reaches highWaterMark).
  3. A generic queuedSizeChanged signal, which fulfills both when you write to the stream and when the underlying sink finishes consuming a chunk.
  4. A more monotonic queueSizeDecreased signal (probably there is a better name), which fulfills when the underlying sink finishes consuming a chunk. The semantics here are a bit unclear to me honestly.
  5. A customizable .waitForQueueSize(n) which waits for the queue size to dip below n, or .waitForDesiredSize(n) which waits for the desired size to go above n.

I am leaning toward the simplicity of (1) and/or (2). But (5) also works. I don't really like (3) or (4).

Who should determine the high water mark?

We could do a few different choices:

  1. Get rid of the high water mark entirely. Producers need to monitor queuedSize and adjust to their comfort level. Writable streams don't get to signal what they want.
  2. As-is now, make it part of the queuing strategy, so that it is set at writable stream creation time by the stream creator.
  3. Make it determined by getWriter({ highWaterMark }). This is essentially saying that high water mark is a way to save the producer some arithmetic over option (1). The producer would pass in highWaterMark = x and get back desiredSize, instead of getting back queuedSize = y and calculating desiredSize = x - y. See also Move high water mark to readers? #375.

I am a bit uncomfortable with (1) and (3) because I think many writable streams will have at the very least a desired chunk size that they would want to signal.

@domenic domenic modified the milestone: Week of 2015-08-24 Aug 20, 2015
@tyoshino
Copy link
Member

What should we expose?

I think desiredSize is the right thing to expose. The queue size is a part of the internal state/details of the stream we don't have to expose. I'd just return what the producer need to know, which is desiredSize.

@tyoshino
Copy link
Member

How should the ready signal work?

waitForDesiredSize(n) is useful when the producer know that the stream will have a space >= n. Given a stream but without high water mark value, a producer cannot determine what n would work (don't wait forever) for the stream. I think we need to make n optional and then waitForDesiredSize(undefined) work as waitForDesiredSizeChange/Increase(). I.e. equiv of (3), (4). I think it would work like the following:

  • when .waitForDesiredSize(undefined) is called, the writer remembers current space available for write
  • when it changes/increases compared to the saved value, resolve the promise.
  • and then, the producer checks desiredSize and produce appropriate size of data and write it.
  • seems "change" should be "change to different value than the saved && the new value is positive", and "increase" should be "increase to a value more than the saved && positive". But not sure. A producer may want to target some value more than the stream expects for some reason. I.e. it may want to get notified even when the desiredSize is not yet positive.

@domenic
Copy link
Member Author

domenic commented Aug 25, 2015

I think desiredSize is the right thing to expose.

OK, this makes sense. I am still kind of unclear whether the producer or the stream should be setting the high water mark, but if we assume the stream, then this seems correct.

waitForDesiredSize(n) is useful when the producer know that the stream will have a space >= n. Given a stream but without high water mark value, a producer cannot determine what n would work (don't wait forever) for the stream.

This seems like a potentially big problem. I think it would mean that unless the producer controls the high water mark, then waitForDesiredSize(n) is never very useful. Because you never know if you might end up waiting forever.

Maybe one fix is that if n > highWaterMark, we notify you anyway, when desiredSize = highWaterMark.

I think we need to make n optional and then waitForDesiredSize(undefined) work as waitForDesiredSizeChange/Increase(). I.e. equiv of (3), (4). I think it would work like the following:

Hmm, what do you think of making it work like (1) or (2)?

Also, if we make these require positive, then for HWM = 0 writable streams they all reduce to the same thing. Probably that is OK.

@domenic
Copy link
Member Author

domenic commented Aug 25, 2015

I think desiredSize is the right thing to expose.

I have one other worry about desiredSize. Do you see it working in practice for UA created streams? E.g. let's take some examples (no comment on whether these will actually be implemented):

  • HTTP upload writable stream (even though we decided against this)
  • Write-to-IndexedDB stream
  • TextEncoder transform stream writable side
  • Streaming HTML parser writable stream

Do you think it will be reasonable for UAs to come up with high water marks for all of these, in order to govern desiredSize? Maybe in some cases the HWM will just be zero---is that OK as a default?

@tyoshino
Copy link
Member

Maybe one fix is that if n > highWaterMark, we notify you anyway, when desiredSize = highWaterMark.

Ah, yeah. That works. Notification threshold would be set to min(hightWaterMark, n)

Hmm, what do you think of making it work like (1) or (2)?

(1) and (2) would work but changed signal is more flexible (yeah, HWM=0 would behave as them). What's the reason you preferred them than changed signal? Simplicity? I understand that the name "ready" isn't good name for "changed" signal, though.

UA created streams

Yeah. When HWM of 0 is used, desiredSize would be 0 or negative. The problem is that pipeTo() should be designed to (possibly by having some mode to) be able to push data to such a writable stream.

"Push only when non-negative" would work?

@tyoshino
Copy link
Member

Current pipeTo() is: read() and wait for dest.ready. When data is read and dest is ready, write the read data to dest.

For this kind of HWM=0 writable streams, pipeTo() should read() anyway when desiredSize becomes non-negative. I.e. non-negative desiredSize is treated as "ready". The actual value of desiredSize would be used to update precise pulling on readable stream to control amount to pull, but read() invocation happens anyway.

Maybe this works?

@domenic
Copy link
Member Author

domenic commented Sep 3, 2015

(1) and (2) would work but changed signal is more flexible (yeah, HWM=0 would behave as them). What's the reason you preferred them than changed signal? Simplicity? I understand that the name "ready" isn't good name for "changed" signal, though.

Yeah, basically simplicity. I cannot figure out the use case for the generic changed signal. The generic changed signal seems to not be very aligned with the high water mark idea. That is, I think two valid default strategies are either you should always be writing when desiredSize > 0 (1), or you should wait until the stream is drained and desiredSize = HWM (2). Anything more customizable can be taken care of by specific calls to waitForDesiredSize(n), with a parameter. (Remember, we are just trying to decide the default for waitForDesiredSize(), or whatever it is called.)

In fact, maybe it does not matter too much. I think you can get (4) with writer.waitForDesiredSize(writer.desiredSize + 1). (Well, assuming your sizes are integers. If we really wanted to fix that we could do .waitForDesiredSize(n, { comparison: ">" }) or similar.)

I also noticed when reviewing Node streams that their 'drain' event is (2), not (1).

Given this I propose: writer.waitForDesiredSize(n = high water mark), i.e. default to (2). This will fulfill when desiredSize becomes >= min(highWaterMark, n).

Yeah. When HWM of 0 is used, desiredSize would be 0 or negative. The problem is that pipeTo() should be designed to (possibly by having some mode to) be able to push data to such a writable stream.

"Push only when non-negative" would work?

... Maybe this works?

I think this works if we say waitForDesiredSize waits for >= min(highWaterMark, n). So in this case it will be equivalent to waitForDesiredSize(0), which fulfills as soon as the queue drains.

@tyoshino
Copy link
Member

I was supposing cases where:

  • We're piping data from ReadableStream R to WritableStream W
  • There's a producer P behind R
  • There's a consumer C behind W
  • R has a queue RQ
  • W has a queue WQ
  • When P sees new space available in RQ, starts producing more data to fill it. But it takes some time to produce it.

When the amount of data in WQ decreases a little, i.e. desiredSize increases, we should immediately tell that to RQ so that P can start producing more data.

This argument supports (4).

(5) is useful when:

  • a producer code is directly using the WritableStream. the producer can tell the size of the next item
  • we want to strictly limit the size of the queue in the WritableStream

(3) and (4) work but we can reduce number of notifications by (5).

(Remember, we are just trying to decide the default for waitForDesiredSize(), or whatever it is called.)

Oh, OK. Then I slightly prefer using (1) for .ready and waitForDesiredSize(undefined) for the same reason as above.

But as you wondered in the second paragraph for different topic, if our sizes are non-integer, the behavior cannot be realized by waitForDesiredSize(n). It strikes a little weird to me though it's not a big deal. n = Number.EPSILON? Hmm.

I think you can get (4) with writer.waitForDesiredSize(writer.desiredSize + 1)

Yeah, right.

Given this I propose: writer.waitForDesiredSize(n = high water mark), i.e. default to (2). This will fulfill when desiredSize becomes >= min(highWaterMark, n).

Ah, ... I think I'm fine with this, too.

@bmeck
Copy link

bmeck commented Dec 15, 2015

Just a reminder here, the ready signal always fires the .then even after the desired size / queue is no longer at that size. That seems a bit odd for a signal to stay active even when the condition is no longer true.

@tyoshino
Copy link
Member

tyoshino commented Aug 4, 2016

Partially fixed by PR #488.

See #493 for whether and how we should change writer.ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants