FromInputStreamPublisher: avoid extra allocation of a buffer #2965
Conversation
| new byte[]{31, 32, 33, 34, 35}, | ||
| new byte[]{36}, | ||
| // available < readChunkSize | ||
| new byte[]{0, 1, 2}, |
There was a problem hiding this comment.
Changes for this test are made in the first commit to demonstrate that they only enhance test coverage and are not driven by the actual change. Consider reviewing this PR by every commit.
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
force-pushed
the
readNoAvailable
branch
from
bf71ddb to
70383fa
Compare
| @@ -176,14 +173,27 @@ public void cancel() { | |||
| private void readAndDeliver(final Subscriber<? super byte[]> subscriber) { | |||
| try { | |||
| do { | |||
| int readByte = -1; | |||
There was a problem hiding this comment.
ffti
| int readByte = -1; | |
| int readByte = END_OF_FILE; |
There was a problem hiding this comment.
While technically it's the same value, it has a different meaning as "didn't read a byte" vs "read EOF". I would prefer to not confuse it, but ready to change it to Integer.MIN_VALUE instead. WDYT?
There was a problem hiding this comment.
Maybe an inline javadoc would help to clear up confusion and we can keep the -1?
There was a problem hiding this comment.
Inline comment, or maybe a new constant (could have same or different value) to signal the different meaning would be fine. tbh I'm also fine with just merging as is, thus the ffti.
| @@ -176,14 +173,27 @@ public void cancel() { | |||
| private void readAndDeliver(final Subscriber<? super byte[]> subscriber) { | |||
| try { | |||
| do { | |||
| int readByte = -1; | |||
There was a problem hiding this comment.
Maybe an inline javadoc would help to clear up confusion and we can keep the -1?
Motivation:
In #2949 we optimized a case when
available()is not implemented and always returns0. However, we de-optimized a use-case when it's implemented because the last call toavailable()always returns 0, but we still allocate a buffer of sizereadChunkSizethat won't be used.Modifications:
doNotFailOnInputStreamWithBrokenAvailableCall(...)test before any changes for better test coverage.byte[] bufferfrom a class variable. It can be a local variable because it's never reused in practice. Only the lastbufferwon't be nullified, but we don't need it after that.available()returns0, try reading a single byte and then recheck availability instead of always falling back toreadChunkSize.doNotFailOnInputStreamWithBrokenAvailableCall()test to account for the 2nd call toavailable();singleReadTriggersMoreAvailability()test to simulate when the 2nd call toavailable()returns positive value;Result:
bufferthat won't be used at the EOF.read().Benchmark results:
The effect is visible for 256B and 16Kb payloads (less than 1 readChunkSize) because for them an allocation of an extra
bufferis a bigger contributor.available()of 0 means it always returns0,true- it always returns a real value. Used in-memoryByteArrayInputStreamwith overriddenavailable()method.