-
-
Notifications
You must be signed in to change notification settings - Fork 359
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: add caching to koanf config to fix performance regression #1042
fix: add caching to koanf config to fix performance regression #1042
Conversation
9e2ece5
to
cc501c9
Compare
cc501c9
to
75a7089
Compare
Codecov Report
@@ Coverage Diff @@
## master #1042 +/- ##
==========================================
+ Coverage 77.74% 78.20% +0.46%
==========================================
Files 83 83
Lines 3841 4006 +165
==========================================
+ Hits 2986 3133 +147
- Misses 578 594 +16
- Partials 277 279 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
5a25bba
to
4ce1fe6
Compare
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.
Thank you very much! I think there's an improvement to this that addresses the problem in a more targeted way and without sideffects :)
@@ -306,7 +346,11 @@ func (v *KoanfProvider) PipelineConfig(prefix, id string, override json.RawMessa | |||
return errors.WithStack(err) | |||
} |
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.
The culprit of the high CPU usage is in this method. My recommendation would be to cache the result of the validation (only when successful) and the cache key sha256(prefix + "|" + id + "|" + marshalled)
. That way we will respect hot reloading (because we use the config values) and still save a lot of CPU cycles!
49075d7
to
d251a7d
Compare
Hey @aeneasr I pushed edits to the PR. |
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.
Awesome, looking much better! just a commend on how to improve the ristretto config
d251a7d
to
77d5b4e
Compare
Thanks for the configuration @aeneasr 👍 |
Fix #1033 of a performance regression in v0.40.0.
It seems linked to a perpetual config reloading, leading to a lot of JSON marshalling being CPU intensive.
Adding back in-memory caching, as we used to have for Viper configuration.
Related issue(s)
Fixes #1033
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further Comments