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

[Java] Configurable buffer size limit #1950

Closed
theigl opened this issue Nov 20, 2024 · 2 comments
Closed

[Java] Configurable buffer size limit #1950

theigl opened this issue Nov 20, 2024 · 2 comments
Labels
question Further information is requested

Comments

@theigl
Copy link
Contributor

theigl commented Nov 20, 2024

Question

Since 01f1b66 Fury's internal MemoryBuffer is reset to 128k after every use.

The change makes sense because it reclaims memory that could otherwise sit unused, but it will cause massive allocations if most of the object graphs you serialize are >= 128k. In this case, a new buffer is allocated for every serialization.

In my application, most object graphs are 64k-512k leading to very frequent reallocation of the built-in buffer.

Currently, my only option is to manage a pool of MemoryBuffers myself and pass them to Fury instead of relying on the built-in buffer, but this makes using Fury significantly more complex.

Would it make sense to make the buffer size limit configurable? In my case, a 1MB limit would be ideal.

@theigl theigl added the question Further information is requested label Nov 20, 2024
@theigl theigl changed the title [Question] [Java]: Configurable buffer size limit [Java]: Configurable buffer size limit Nov 20, 2024
@theigl theigl changed the title [Java]: Configurable buffer size limit [Java] Configurable buffer size limit Nov 20, 2024
@chaokunyang
Copy link
Collaborator

chaokunyang commented Nov 21, 2024

Make sense, would you like to submit a PR to add a new field at FuryBuilder/Config and use that value for Buffer reset?

@theigl
Copy link
Contributor Author

theigl commented Dec 5, 2024

@chaokunyang: I pushed a PR for this, but I'm not sure if it is the right approach. Do you have any feedback?

chaokunyang pushed a commit that referenced this issue Dec 5, 2024
## What does this PR do?

This PR introduces a new configuration option `bufferSizeLimitBytes`
that replaces the hard-coded default of 128kb.

## Related issues

#1950

## Does this PR introduce any user-facing change?

The PR introduces a new configuration option `bufferSizeLimitBytes`.

- [x] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?

## Discussion

This PR solves my problem, but I'm not sure if it is the right way to
move forward. This is quite a low-level configuration option, but a
potentially very important one. Every user whose average payload size is
>=128kb, will need to increase this value for maximum performance. Maybe
the default limit should be increased to something less conservative
like 1MB, so fewer users will need to adjust this setting?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants