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

Fix InboundBuffer::pause & write performance #5144

Merged
merged 2 commits into from May 16, 2024

Conversation

franz1981
Copy link
Contributor

This patch improve Quarkus techempower plaintext throughput of ~5% in our lab; nothing fancy but seems related to the excessive synchronization which, by using instance field to synchronize with, doesn't allow the JVM to perform the https://shipilev.net/jvm/anatomy-quarks/1-lock-coarsening-for-loops/ (lock coarsening) optimization.

I've applied an optimization to onEnd which benefit vertx as well, which often observe non-paused requests
i.e. reducing the synchronization over the connection while completing the request.

@franz1981
Copy link
Contributor Author

@vietj This change here is not in conflict with #5143 given that it works on different synchronization(s) happening in a inefficient way, so, if you agree on the changes, could be safely merged.

Just for reference, it improves by ~5% few benchmarks using plaintext, so, not a game changer; but given that fixing the presence of pausing in Quarkus is NOT possible nor is possible to improve the inbound buffer to not create any array q, in case we end a request while still paused, this is the less destructive and simpler change to improve things I could think about.

Said that, sadly profilers can consistently lie and, in an experiment, I've tried removing InboundBuffer::write code just to check if it improves performance, and it seems not (!); instead its caller io/vertx/core/http/impl/Http1xServerConnection.handleMessage has been blame, which will be improved by #5143

@franz1981
Copy link
Contributor Author

@vietj ping

@franz1981
Copy link
Contributor Author

@vietj how this look bud?

@vietj
Copy link
Member

vietj commented Mar 27, 2024

this looks good, I need to review it.

can you have a quick look at https://github.com/eclipse-vertx/vert.x/pull/5164/files ?

@vietj
Copy link
Member

vietj commented Mar 27, 2024

@franz1981 before proceeding with this, I think we can review and merge https://github.com/eclipse-vertx/vert.x/pull/5143/files which does remove synchronization use in Http1xServerConnection

@franz1981
Copy link
Contributor Author

@vietj I've addressed your comments and added some tests for the additional unsynchronized methods.

@franz1981
Copy link
Contributor Author

@vietj this is fine than? So we can bring it in the next quarkus release

@franz1981
Copy link
Contributor Author

Ping @vietj ? This is ok to merge? So I can verify on the next techempower round if helps?

@vietj vietj merged commit f8244af into eclipse-vertx:4.x May 16, 2024
7 checks passed
@vietj vietj added this to the 4.5.8 milestone May 16, 2024
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.

None yet

2 participants