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 Publisher.fromInputStream(InputStream, ByteArrayMapper) #2989

Merged
merged 3 commits into from
Jun 28, 2024

Conversation

idelpivnitskiy
Copy link
Member

@idelpivnitskiy idelpivnitskiy commented Jun 26, 2024

Motivation:

Existing Publisher.fromInputStream(InputStream) contract has a side effect of an extra allocation in case the pre-allocated byte-array was not completely full of data.

Modifications:

  • Add Publisher.fromInputStream(InputStream, ByteArrayMapper) that allows users to decide how to use the buffer region with data;
  • Deprecate pre-existing Publisher.fromInputStream(InputStream) and Publisher.fromInputStream(InputStream, int) overloads;
  • Update all existing use-cases to use BufferAllocator.wrap as a method reference as a ByteArrayMapper;

Result:

No impact on throughput and latency for BlockingStreamingHttpClient requests with InputStream payload (8Kb, 16Kb, 24Kb), but significant reduction in memory allocations and number of GC runs per benchmark:

Allocations: ~2Gb/s -> ~1.4Gb/s
Young Collection GC Count: 29 -> 23
Alloc Outside TLABs: 7.33 GiB -> 5.04 GiB

Motivation:

Existing `Publisher.fromInputStream(InputStream)` contract has a side
effect of an extra allocation in case the pre-allocated byte-array was
not completely full of data.

Modifications:

- Add `Publisher.fromInputStream(InputStream, ByteArrayMapper)` that
allows users to decide how to use the buffer region with data;
- Deprecate pre-existing `Publisher.fromInputStream(InputStream)` and
`Publisher.fromInputStream(InputStream, int)` overloads;
- Update all existing use-cases to use `BufferAllocator.wrap` as a
method reference as a `ByteArrayMapper`;

Result:

No impact on throughput and latency for `BlockingStreamingHttpClient`
requests with `InputStream` payload, but significant reduction in memory
allocations and number of GC runs per benchmark:

Allocations: ~2Gb/s -> ~1.4Gb/s
Young Collection GC Count: 29 -> 23
Alloc Outside TLABs: 7.33 GiB -> 5.04 GiB
* @param <T> Type of the result of this mapper
*/
@FunctionalInterface
public interface ByteArrayMapper<T> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this interface bcz concurrent-api doesn't have a dependency on buffer-api module.
Eventually, I think it's worth it. We can consolidate configuration parameters for how to read data from InputStream as default methods on this interface instead of adding more and more overloads for Publisher.fromInputStream. Publisher is already quite a lengthy class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

* @param <T> Type of the result of this mapper
*/
@FunctionalInterface
public interface ByteArrayMapper<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@idelpivnitskiy idelpivnitskiy merged commit 434da83 into apple:main Jun 28, 2024
11 checks passed
@idelpivnitskiy idelpivnitskiy deleted the ByteArrayMapper branch June 28, 2024 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants