-
Notifications
You must be signed in to change notification settings - Fork 50
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
WIP: add caching using KV #163
base: main
Are you sure you want to change the base?
Conversation
220fa47
to
4c53cd8
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.
I found this a bit hard to follow, but I think you are in a bind - there is no real loader layer where you can inject caching behaviour, so you need to inject caching into existing large functions, and it ends up bringing lots of tactical considerations around things like time parsing right up into the cache and application layers.
Ideally it would be good to start unpicking bits of the structure, and adding traits and helper methods (e.g. if cached.has_expired()
) rather than operating directly on strings and serialisation types. But that would likely require a more significant refactor than you can fit in at this time. Something to think about for the longer term maybe...
4c53cd8
to
47c5d8e
Compare
@itowlson, I think I have addressed all the comments and I think this is ready for another round of review. |
Signed-off-by: Karthik Ganeshram <karthik.ganeshram@fermyon.com>
47c5d8e
to
17b2cf2
Compare
Has this been tested in the cloud? (Not sure if we can), but I'm curious where the performance benefits come from? Moving from generating things as the request comes in, to read something pre-generated from the KV Store? Is that main thing? |
We can't test it on the cloud yet. You are correct in the source of performance improvements, by keeping a copy of rendered stuff and using it until the cache is invalidated, the entire render cycle is skipped. |
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.
LGTM!
without caching in kv
with caching