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

Add readableAmount getter to ReadableByteStream #279

Closed
wants to merge 1 commit into from

Conversation

tyoshino
Copy link
Member

No description provided.

@tyoshino
Copy link
Member Author

I'm making a PR to update the ReadableByteStream to address #266. I'll make ref impl change once it's done.

@tyoshino
Copy link
Member Author

#280 is the PR to update the ReadableByteStream to address #266.

@domenic
Copy link
Member

domenic commented Feb 10, 2015

I wonder if this should be a getter or a method. Right now it is a method on the underlying source but a getter on the stream.

I think either:

  • We should make it a getter on both, with the assumption that the underlying source implementer will respect the vague idea that "getters should not do very much"
  • Or we should make it a method on both, so that we don't have to assume anything.

Alternately we could come up with more complicated schemes like making it a method on the underlying source that we only call at certain times and then cache the result of, and then a getter on the stream returns the cached result.

@domenic
Copy link
Member

domenic commented Feb 10, 2015

Oops. I accidentally merged this as 127df9e. Anyway let's continue the discussion (here I guess?). I'll leave it as a getter externally/method on underlying source for now.

@acolwell
Copy link

FWIW if the solution for the MSE case is being moved to only being addressed by ReadableByteStream, then I don't think readableAmount is needed. If I understand the ReadableByteStream correctly, I think MSE should simply be able to call readInto() with the size it needs and everything would be fine. It looks like readInto() essentially acts like POSIX read() so that would be sufficient for MSE's needs.

I suppose this is a long way of saying, don't add readableAmount just for MSE. :)

@tyoshino
Copy link
Member Author

Even in the new async read() byte stream Domenic is working on at #296, you can call reader.read(view) with view whose size equals to the number of bytes you want to get to read only what you want.

Kindly hold!

@tyoshino
Copy link
Member Author

readableAmount getter was proposed for users of hypothetical underlying sources which can tell how many bytes can be read synchronously to pass a single contiguous buffer of sufficient enough size to readInto() call while keeping it able to consume only part of the synchronously readable data not to replenish back pressure too much.

But when I revisited the issue, I think that it doesn't make much sense.

  • If the total size to read into a single contiguous buffer is known, they can just allocate a buffer of the size and repeat invoking readInto() until it's filled.
  • If the total size is unknown, we may still need to repeat readInto() calls if parts of data arrive little by little.

If reading into a single contiguous buffer doesn't really matter, we can just read into some small buffer and process bytes in it repeatedly. It just works.

readableAmount getter just reduces the number of readInto() call. That's not big benefit. Closing for now. Can be opened if necessary.

@tyoshino tyoshino closed this Mar 16, 2015
@domenic domenic deleted the readableAmount branch March 17, 2015 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants