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

Sys prop configuration to leverage SSL heap buffer pooling #45185

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

franz1981
Copy link
Contributor

@franz1981 franz1981 commented Dec 18, 2024

This fix the weird performance problem observed at #41880 (comment)

if the users run the quarkus application with
-Dquarkus.http.server.ssl.jdk.bufferPooling=true -Dvertx.reuseNettyAllocators=true (latter, see eclipse-vertx/vert.x#5292)

It is still missing:

  • Reactive SQL Client SSL
  • gRPC SSL
  • vertx http client-side (if we expose it)

I'm not aware in which other Quarkus extensions (.e.g OTel?) we are using SSL/TLS from Vertx: if we do, we should provide a similar fix there.

@franz1981
Copy link
Contributor Author

@geoand I'm opened to suggestions on how to better let users to consume it.

To be fair -Dvertx.reuseNettyAllocators=true should always be set, it's low risk and good impact.
Whilst for quarkus.http.server.ssl.jdk.bufferPooling I have tried to make it "hidden" enough, since, with Vertx 5 it will be the default behaviour (both, actually) - and I don't think we want to deprecate or else....

IDK if it's ok the way I've implemented it because HttpServerOptionsUtils is consumed by VertxHttpRecorder

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Dec 18, 2024

This is more for @cescoffier to decide

@cescoffier cescoffier added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 18, 2024
@franz1981
Copy link
Contributor Author

@mabartos can you please check the checklist in description if TLS/SSL is used in any of the missing changes?

Copy link

quarkus-bot bot commented Dec 18, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 49ee0bc.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@geoand
Copy link
Contributor

geoand commented Dec 19, 2024

I'm not aware in which other Quarkus extensions (.e.g OTel?) we are using SSL/TLS from Vertx: if we do, we should provide a similar fix there.

Correct

@geoand geoand merged commit 55f2a7b into quarkusio:main Dec 19, 2024
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Dec 19, 2024
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 19, 2024
@franz1981
Copy link
Contributor Author

I've quickly verified, by running a debug session vs getting-started (dev branch) - that the 2 args work as intended.
@mabartos need to add them in order to benefit of these changes, but please check if you need to have the gRPC fix and the reactive SQL one

@mabartos
Copy link
Contributor

mabartos commented Dec 19, 2024

@franz1981 Thanks for these changes! We will probably run some more comprehensive analysis after Xmas.

For Keycloak purposes, we don't use gRPC SSL, or reactive SQL SSL. AFAIK, not even Vert.x HTTP client.

@franz1981
Copy link
Contributor Author

franz1981 commented Dec 19, 2024

Lovely, I would like to ask more people in the community if they can give it a shot, maybe @vsevel - but it's approaching Christmas so, don't expect the world to be reactive eheh

@vsevel
Copy link
Contributor

vsevel commented Dec 19, 2024

I am making a note of it. how should this be tested? where can I expect the improvements?

@franz1981
Copy link
Contributor Author

Thanks @vsevel for the quick response: it should benefit when http (1.1 and 2) TLS/SSL is involved.
If you have other "secured" cases which include gRPC, Reactive SQL, http client side...let me know so I can provide a fix for these too 🙏

The benefits are both CPU usage and heap allocation pressure, since, without these fixes, on SSL/TLS every decoding/encoding of data just allocate a giant unpooled heap buffer.
These changes uses heap pooled buffers, instead, which despite increases the application idle memory footprint (in terms of heap old gen occupancy) it should reduce the demand under load.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants