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

Optimize request buffer release #12239 #12240

Merged
merged 12 commits into from
Sep 17, 2024

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Sep 5, 2024

Fix #12239 by releasing an empty request buffer when it is known there is no content to read.

sbordet and others added 8 commits September 4, 2024 21:30
Alternative to #12228.

In this PR, the responsibility to release the buffers is in 2 methods: onFillable() (called when network data is available, and to process the next request) and parseAndFillForContent() (called from Request.read()).

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Using the request executor to dispatch onFillable().

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Ensure there is a request before accessing components
Release buffer after consumeAvailable
…ary.

Fixed failing tests that were not completing the Handler Callback.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Release request buffer before handling when there is no content
@gregw gregw requested review from sbordet and lorban and removed request for sbordet September 5, 2024 22:53
Release request buffer before handling when there is no content
@sbordet
Copy link
Contributor

sbordet commented Sep 6, 2024

@gregw can you please rebase this PR on top of HEAD, now that you have merged #12237?
Will be easier to spot the changes introduced by this PR.

…fferRelease

# Conflicts:
#	jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java
@gregw
Copy link
Contributor Author

gregw commented Sep 6, 2024

@gregw can you please rebase this PR on top of HEAD, now that you have merged #12237? Will be easier to spot the changes introduced by this PR.

@sbordet Done.... but might be best to rebase on top of 12.1 ??

@lorban can you think about load testing this branch?

@gregw gregw marked this pull request as ready for review September 7, 2024 00:47
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

Changes look simple enough, I think we should apply them to 12.0.x.

@lorban
Copy link
Contributor

lorban commented Sep 10, 2024

@gregw this branch seems to reliably improve perf by about 5%.

Here is a profiling run of vanilla 12.1.x: https://jenkins.webtide.net/job/load_testing/job/jetty-profiler-12.1.x/10
and a pair of profiling runs of this branch: https://jenkins.webtide.net/job/load_testing/job/jetty-profiler-12.1.x/11 and https://jenkins.webtide.net/job/load_testing/job/jetty-profiler-12.1.x/12

@gregw gregw requested a review from sbordet September 12, 2024 07:13
@gregw
Copy link
Contributor Author

gregw commented Sep 16, 2024

@sbordet @lorban nudge

@gregw gregw merged commit dc43f3d into jetty-12.0.x Sep 17, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Optimize buffer release in HttpConnection
3 participants