Fallback to readChunkSize if InputStream.available() not implemented
#2949
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: `FromInputStreamPublisher` attempts to read only 1 byte when `InputStream.available()` is not implemented. As a result, users of blocking streaming API with payload body as `InputStream` may write and flush by 1 byte if their `InputStream` implementation always returns 0 available bytes (default). Modifications: - Consider `available()` as a best effort to avoid blocking. If it returns 0 bytes, attempt to read up to `readChunkSize` bytes; Result: Improved efficiency of writing `InputStream` data for outgoing requests.
bryce-anderson
approved these changes
Jun 4, 2024
| @@ -171,7 +171,7 @@ private void readAndDeliver(final Subscriber<? super byte[]> subscriber) { | |||
| int available = stream.available(); | |||
| if (available == 0) { | |||
| // Work around InputStreams that don't strictly honor the 0 == EOF contract. | |||
| available = buffer != null ? buffer.length : 1; | |||
| available = buffer != null ? buffer.length : readChunkSize; | |||
There was a problem hiding this comment.
As you mentioned offline, the default value is 64kB which could be rough on GC in adversarial cases. Do you plan to change that as well in a different PR, or are we fine trying it as-is?
There was a problem hiding this comment.
Yes, I already played with different values and wire-logging enabled, 64Kb is not a good default.
Still need to run some benchmarks and will update the value in a separate PR.
There was a problem hiding this comment.
As a data point, in AHC the InputStreamEntity uses OUTPUT_BUFFER_SIZE = 4096; as the chunk size for this. (that said, the change dates back to 2013 and there doesn't seem to be an explanation why 4k was chosen, so maybe we can come up with something better (larger?).
daschl
approved these changes
Jun 5, 2024
idelpivnitskiy
added a commit
to idelpivnitskiy/servicetalk
that referenced
this pull request
Jun 12, 2024
Motivation: In apple#2949 we optimized a case when `available()` is not implemented and always returns `0`. However, we de-optimized a use-case when it's implemented because after that change the last call to `available()` always returns 0, but we still allocate a buffer of size `readChunkSize` that won't be used at all. Modifications: - Enhance `doNotFailOnInputStreamWithBrokenAvailableCall(...)` test before any changes to have better test coverage. - Remove `byte[] buffer` from a class variable. It can be a local variable because it's never reused in practice. Only the last `buffer` won't be used nullified, but we don't need it after that. - When `available()` returns `0`, try reading a single byte and then check availability again instead of always falling back to `readChunkSize`. - Adjust `doNotFailOnInputStreamWithBrokenAvailableCall()` test to account for the 2nd call to `available()`; - Add `singleReadTriggersMoreAvailability()` test to simulate when the 2nd call to `available()` returns positive value; Result: 1. No allocation of a `buffer` that won't be used at the EOF. 2. Account for new availability if it appears after a `read()`.
idelpivnitskiy
added a commit
that referenced
this pull request
Jun 13, 2024
Motivation: In #2949 we optimized a case when `available()` is not implemented and always returns `0`. However, we de-optimized a use-case when it's implemented because after that change the last call to `available()` always returns 0, but we still allocate a buffer of size `readChunkSize` that won't be used at all. Modifications: - Enhance `doNotFailOnInputStreamWithBrokenAvailableCall(...)` test before any changes to have better test coverage. - Remove `byte[] buffer` from a class variable. It can be a local variable because it's never reused in practice. Only the last `buffer` won't be used nullified, but we don't need it after that. - When `available()` returns `0`, try reading a single byte and then check availability again instead of always falling back to `readChunkSize`. - Adjust `doNotFailOnInputStreamWithBrokenAvailableCall()` test to account for the 2nd call to `available()`; - Add `singleReadTriggersMoreAvailability()` test to simulate when the 2nd call to `available()` returns positive value; Result: 1. No allocation of a `buffer` that won't be used at the EOF. 2. Account for new availability if it appears after a `read()`.
idelpivnitskiy
added a commit
that referenced
this pull request
Jun 14, 2024
Motivation: In #2949 we optimized a case when `available()` is not implemented and always returns `0`. However, we de-optimized a use-case when it's implemented because the last call to `available()` always returns 0, but we still allocate a buffer of size `readChunkSize` that won't be used. Modifications: - Enhance `doNotFailOnInputStreamWithBrokenAvailableCall(...)` test before any changes for better test coverage. - Remove `byte[] buffer` from a class variable. It can be a local variable because it's never reused in practice. Only the last `buffer` won't be nullified, but we don't need it after that. - When `available()` returns `0`, try reading a single byte and then recheck availability instead of always falling back to `readChunkSize`. - Adjust `doNotFailOnInputStreamWithBrokenAvailableCall()` test to account for the 2nd call to `available()`; - Add `singleReadTriggersMoreAvailability()` test to simulate when the 2nd call to `available()` returns positive value; Result: 1. No allocation of a `buffer` that won't be used at the EOF. 2. Account for new availability if it appears after a `read()`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
FromInputStreamPublisherattempts to read only 1 byte whenInputStream.available()is not implemented. As a result, users of blocking streaming API with payload body asInputStreammay write and flush by 1 byte if theirInputStreamimplementation always returns 0 available bytes (default).Modifications:
available()as a best effort to avoid blocking. If it returns 0 bytes, attempt to read up toreadChunkSizebytes;Result:
Improved efficiency of writing
InputStreamdata for outgoing requests.