-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 memory usage on the TLS manager/store #10302
base: master
Are you sure you want to change the base?
Conversation
Hello @ddtmachado, Thank you for this PR, we are very interested in it but won't be able to test/review/merge it before releasing Traefik v3.0. On your side, could you rebase it on the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ddtmachado!
Nice to see you around and thanks for this contribution!
I took a look at the changes and I have a few comments.
Regarding the next steps of the review process, do you want to work and iterate on the changes, or you don't mind if we push some review commits?
Hey Romain, great to see you're on this!! Anyway, I'm running out of free time lately due to other demands and would not mind if you just push some commits on top, feel free as I completely agree with your comments and did not validade for those use cases. But in case you run out of time as well or get prioritized in something else let me know and I'll give it a shot in the coming weeks. |
ae8c9a0
to
ab81fd8
Compare
Hello @ddtmachado, I pushed review commits and also rebased the changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
What does this PR do?
Optimize the memory footprint of TLS certificate handling on the TLS Manager and Store, mainly:
I included a small bench test for the manager in the PR, it's goal is to switch TLS certificates in batches of 10 from a pool of 100. With it I observed a large decrease from +10k allocs/op average to 1.9k allocs/op average as follows:
Motivation
Since v2 Traefik received many reports with concerns and problems caused by the increased memory usage, specially while handling TLS certs.
I had some free time to investigate and found the code on the manager and store overly complex and probably trading memory for compatibility without strong justification, other than "I won't risk touching this code" maybe? :D
More
Additional Notes
I did not had time to properly test it in a real environment with real data. Needs way more testing.