-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add ng-http-caching request debouncing and caching #2171
Conversation
Size Change: +311 B (+0.01%) Total Size: 3.59 MB
ℹ️ View Unchanged
|
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.
Minor changes.
Most importantly, I can't see what the default TTL is. Remember this is a performance optimisation task, not a caching task. We don't need to reinvent the browser's cache, so we should be using a short TTL.
Measure the performance of this change:
- 2 pages
- 4 TTLs: 0, 200ms, 10 seconds, 60 seocnds
- 10 samples each
See if average load times change.
// TODO: we should do targeting cache invalidation and invalidate all cache | ||
// keys that contain the item being deleted from cache |
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 don't understand what this means
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.
It was supposed to add a todo stating "instead of clearing the entire cache when a single item is updated, we should just delete that one item from the cache. However, when an model is updated/deleted, it may be used as an association to another model, meaning that we will have to invalidate the changed model, and any other models that reference the newly invalidated model".
However, since we have lowered the cache lifetime to one second and our caching is designed to reduce the impact of associations and N+1 queries, this is no longer a problem we have to worry about.
MethodologyResults were gathered using a docker container to replicate the production environment as close as possible. The "Cooloola" "Annotation Search" page was used in all tests. Before each testing round, the Cooloola annotation search page was loaded without recording the results so that all audio files would be recalled, and would minimise deviations in the tests due to server caching. A new browser tab was used between each test to prevent potential v8 caching. Network caching was disabled. The starting page for the tests was the "Cooloola" project details page. The "Cooloola" project details page would be hard-reloaded, and the "Search Annotations" menu item would be clicked. Performance would be measured from the start of the menu's "click" event to the end of the annotation search page load. Performance test resultsResults are recorded in seconds
|
From discussion of the results we can see that the actual cache part of this library has little to no effect on performance. TTL does not seem to matter either. The main win is the request de-duplication. so the understanding here is:
We're going to trial 10 second TTL with the hope that for anyone doing any kind of quickly successive actions, they'll see a benefit from the cache (like flicking through pages of results). Also at the same time, anyone doing anything on a page is likely to take more than 10 seconds, stale cache results should basically be a non-issue. We'll do some more testing when #2160 is implemented. #2170 should also help. |
Add ng-http-caching request debouncing and caching
Changes
Issues
Fixes: #2159
Final Checklist
npm run lint
)npm run test:all
)