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

ArrayRetainableByteBufferPool inefficiently calculates bucket indices #8810

Closed
lorban opened this issue Nov 1, 2022 · 5 comments · Fixed by #8882
Closed

ArrayRetainableByteBufferPool inefficiently calculates bucket indices #8810

lorban opened this issue Nov 1, 2022 · 5 comments · Fixed by #8882

Comments

@lorban
Copy link
Contributor

lorban commented Nov 1, 2022

Jetty version(s)
10, 11, 12

Enhancement Description
The ArrayRetainableByteBufferPool.bucketFor() method is on the fast path of every served request, and it delegates its index calculation to a Function<Integer, Integer> lambda. This implies boxing and unboxing are happening each time a bucket must be chosen, and this inefficiency started appearing in profiling reports.

This code should be modified so that no boxing/unboxing is done anymore. A simple way would be to replace the Function<Integer, Integer> lambda with a custom IntIntFunction one for instance.

@lorban lorban changed the title Performance: ArrayRetainableByteBufferPool.bucketFor() inefficiently calculates bucket indices ArrayRetainableByteBufferPool.bucketFor() inefficiently calculates bucket indices Nov 1, 2022
@lorban lorban changed the title ArrayRetainableByteBufferPool.bucketFor() inefficiently calculates bucket indices ArrayRetainableByteBufferPool inefficiently calculates bucket indices Nov 1, 2022
@lorban
Copy link
Contributor Author

lorban commented Nov 1, 2022

Here's an async profiler flame graph that shows ArrayRetainableByteBufferPool.bucketFor() is showing up almost 3% of the time: async-profiler-cpu.zip

@joakime
Copy link
Contributor

joakime commented Nov 10, 2022

@lorban do you want to do this for the upcoming Jetty 10 / Jetty 11 releases?
Or postpone this? (if so, move this to the next project)

@sbordet
Copy link
Contributor

sbordet commented Nov 10, 2022

@joakime @lorban I'll take this, there is java.util.function.IntUnaryOperator that has exactly the right signature and should have been used from the beginning.

@lorban
Copy link
Contributor Author

lorban commented Nov 10, 2022

@joakime It's not going to have much impact, no in terms of reliability (we're just replacing Integers with ints) nor in terms of performance (the AdaptiveExecutionStrategy bottleneck is hiding this one) so it doesn't matter too much.

But we did agree that it is too late for #8762 to be included in the upcoming release.

@joakime
Copy link
Contributor

joakime commented Nov 10, 2022

@lorban in that case, I've moved this from 10.0.13 to 10.0.14

sbordet added a commit that referenced this issue Nov 10, 2022
…bucket indices

Added constructor that uses IntUnaryOperator to avoid boxing.
Deprecated constructor that uses Function<Integer, Integer>.

Signed-off-by: Simone Bordet <[email protected]>
sbordet added a commit that referenced this issue Nov 10, 2022
…bucket indices (#8882)

Added constructor that uses IntUnaryOperator to avoid boxing.
Deprecated constructor that uses Function<Integer, Integer>.

Signed-off-by: Simone Bordet <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants