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.

Sign up for GitHub

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

Jump to bottom

Use RetainableByteBuffer and write a new pool for it #6332

Merged
merged 1 commit into from Jul 24, 2021
Merged

Use RetainableByteBuffer and write a new pool for it #6332

merged 1 commit into from Jul 24, 2021

Conversation

lorban
Copy link
Contributor

@lorban lorban commented May 28, 2021 •
edited by gregw

Fix #6322
HttpConnection can be modified to make use of RetainableByteBuffer with little effort, which indeed allows the use of a much more efficient pool type.

With the above change in place, we can make use of o.e.j.u.Pool to create a RetainableByteBuffer pool for all input buffer needs as that design does bring a significant perf improvement.

@lorban lorban force-pushed the jetty-10.0.x-6322-UseRetainableByteBuffer branch from 996def2 to 6119f2f Compare June 1, 2021 09:23
@lorban
Copy link
Contributor Author

lorban commented Jun 1, 2021

I've also modified SslConnection because its encrypted input buffer also appears as a hotspot when SSL is enabled, as show in the following flame graph: profile.4.zip

@lorban lorban force-pushed the jetty-10.0.x-6322-UseRetainableByteBuffer branch 2 times, most recently from e7e3ad0 to 5fc4f51 Compare June 10, 2021 14:31
@lorban lorban changed the title use RetainableByteBuffer use RetainableByteBuffer and write a new pool for them Jun 11, 2021
@lorban lorban requested review from gregw and sbordet June 11, 2021 09:02
gregw
gregw requested changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Just some initial feedback. More review needed.

jetty-io/src/main/java/org/eclipse/jetty/io/AdapterMemoryPool.java Outdated Show resolved Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/MemoryPool.java Outdated Show resolved Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java Show resolved Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBufferPool.java Outdated Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java Outdated Show resolved Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/AdapterMemoryPool.java Outdated Show resolved Hide resolved
@lorban lorban requested a review from gregw June 17, 2021 09:52
@lorban lorban requested a review from gregw June 21, 2021 15:57
@lorban lorban marked this pull request as ready for review June 21, 2021 16:42
@gregw gregw added this to In progress in Jetty 10.0.7/11.0.7 FROZEN via automation Jun 21, 2021
@lorban lorban requested a review from joakime June 22, 2021 13:29
gregw
gregw approved these changes Jul 7, 2021
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Let's get this in early during the next release cycle!

Jetty 10.0.7/11.0.7 FROZEN automation moved this from In progress to Reviewer approved Jul 7, 2021
@lorban
Copy link
Contributor Author

lorban commented Jul 7, 2021

Looking back, I just noticed that DefaultRetainableByteBufferPool does not have an eviction mechanism to avoid the pool to accumulate an infinite amount of buffers. Duh!

I'm going to work on this right away to make sure this PR is not removing any existing functionality.

gregw
gregw requested changes Jul 7, 2021
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

fix what ludo said

Jetty 10.0.7/11.0.7 FROZEN automation moved this from Reviewer approved to Review in progress Jul 7, 2021
@lorban lorban requested a review from gregw July 7, 2021 15:20
sbordet
sbordet previously approved these changes Jul 7, 2021
View reviewed changes
jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java Show resolved Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/DefaultRetainableByteBufferPool.java Outdated Show resolved Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBufferPool.java Outdated Show resolved Hide resolved
jetty-io/src/test/java/org/eclipse/jetty/io/DefaultRetainableByteBufferPoolTest.java Outdated Show resolved Hide resolved
jetty-io/src/test/java/org/eclipse/jetty/io/DefaultRetainableByteBufferPoolTest.java Outdated Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java Outdated Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java Outdated Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java Show resolved Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java Outdated Show resolved Hide resolved
@lorban lorban requested a review from sbordet July 8, 2021 10:43
@lorban lorban linked an issue Jul 8, 2021 that may be closed by this pull request
Use RetainableByteBuffer in HttpConnection #6322
Closed
@lorban lorban requested a review from sbordet July 8, 2021 17:09
sbordet
sbordet previously approved these changes Jul 8, 2021
View reviewed changes
@lorban lorban requested review from gregw and sbordet July 9, 2021 10:14
sbordet
sbordet previously approved these changes Jul 9, 2021
View reviewed changes
gregw
gregw approved these changes Jul 10, 2021
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

This looks good.
However, I'd still like to see an alternate version (or perhaps an extension) where the bucket sizes are non-linear. Instead of buckets for 1k, 2k, 3k, 4k....64k I think log buckets could be lot more efficient at avoiding lots of unused buckets. So the bucket sizes should be 1k, 2k, 4k, 8k,16k,32k,64k.

Jetty 10.0.7/11.0.7 FROZEN automation moved this from Review in progress to Reviewer approved Jul 10, 2021
gregw
gregw requested changes Jul 12, 2021
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Consider change from #6513

Jetty 10.0.7/11.0.7 FROZEN automation moved this from Reviewer approved to Review in progress Jul 12, 2021
@gregw gregw mentioned this pull request Jul 12, 2021
Closed
@lorban lorban changed the title use RetainableByteBuffer and write a new pool for them Use RetainableByteBuffer and write a new pool for it Jul 12, 2021
@lorban
Copy link
Contributor Author

lorban commented Jul 12, 2021

This PR still needs to be rebased and have its history rewritten.

gregw
gregw previously approved these changes Jul 12, 2021
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

My PR isn't yet ready, so let's not hold this one up.

Jetty 10.0.7/11.0.7 FROZEN automation moved this from Review in progress to Reviewer approved Jul 12, 2021
@lorban
#6322 Use RetainableByteBuffer and write a new pool for it
cfc0a9b 
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban lorban dismissed stale reviews from gregw and sbordet via cfc0a9b July 12, 2021 11:57
@lorban lorban force-pushed the jetty-10.0.x-6322-UseRetainableByteBuffer branch from 410d402 to cfc0a9b Compare July 12, 2021 11:57
Jetty 10.0.7/11.0.7 FROZEN automation moved this from Reviewer approved to Review in progress Jul 12, 2021
Jetty 10.0.7/11.0.7 FROZEN automation moved this from Review in progress to Reviewer approved Jul 12, 2021
@gregw
Copy link
Contributor

gregw commented Jul 19, 2021

don't wait for my PR to merge

@joakime
Copy link
Contributor

joakime commented Jul 23, 2021

@lorban I think you should merge this soon.
Both @gregw and @sbordet have approved it.

@lorban lorban merged commit c9a5d8d into jetty-10.0.x Jul 24, 2021
Jetty 10.0.7/11.0.7 FROZEN automation moved this from Reviewer approved to Done Jul 24, 2021
@lorban lorban deleted the jetty-10.0.x-6322-UseRetainableByteBuffer branch July 24, 2021 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbordet sbordet sbordet approved these changes

@gregw gregw gregw left review comments

@joakime joakime Awaiting requested review from joakime

Assignees

@lorban lorban

Labels
Enhancement
Projects
No open projects
Jetty 10.0.7/11.0.7 FROZEN
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use RetainableByteBuffer in HttpConnection
4 participants
@lorban @gregw @joakime @sbordet