[fix/feat] recache values in higher layers for composed cache adapter #182
Conversation
Codecov Report
@@ Coverage Diff @@
## main #182 +/- ##
==========================================
+ Coverage 96.23% 96.27% +0.04%
==========================================
Files 80 80
Lines 1991 2015 +24
Branches 239 244 +5
==========================================
+ Hits 1916 1940 +24
Misses 75 75
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| } | ||
| } | ||
| unfulfilledFieldValues = newUnfulfilledFieldValues; | ||
| } | ||
|
|
||
| // Recache values from higher layers that were not found in lower layers | ||
| for (let i = 0; i < this.cacheAdapters.length; i++) { |
There was a problem hiding this comment.
we should be able to run a Promise.all here instead of awaiting within a for loop since the re-cache operations are logically independent
There was a problem hiding this comment.
looking at the other code, we've made a point to await on caches closest to the database, then move onto the ones closer to the application. I think this was to avoid bugs where the cache closest to the db was stale, but the ones closer to the application was more up-to-date.
I think the most correct approach would be to iterate backwards (ie) for (let i = this.cacheAdapters.length - 1; i >= 0; i--) { to keep this behaviour consistent
|
|
||
| let unfulfilledFieldValues = fieldValues; | ||
| for (const cacheAdapter of this.cacheAdapters) { | ||
| for (let i = 0; i < this.cacheAdapters.length; i++) { |
There was a problem hiding this comment.
We await within this loop to fetch less objects each time, though might it actually be faster to just fetch all from all caches in parallel as well and then merge them? Might be worth profiling. If we do stick with the for loop we should probably have an early break when unfulfilledFieldValues is empty.
There was a problem hiding this comment.
Performance here will depend on the type of caches that are in the ComposedEntityCacheAdapter. In our case where we've got a local memory and redis cache, I would strongly prefer to keep the for-loop.
If we fetched everything in parallel, we will be waiting for the slowest cache every single time (redis). In the case we have a popular query and the caches are warm, we'll be waiting on the redis cache even though all the values are available in local memory. This would be unfortunate because the redis cache is magnitudes slower than the local memory cache.
The for-loop will optimize for the case when the cache is warm (which will be the majority of requests for a popular query), since we will only be waiting on the local memory cache
There was a problem hiding this comment.
Good call. Yeah that makes sense.
We should still be able to get away with an early break out of the loop if unfulfilledFieldValues is empty though I think.
Co-authored-by: Will Schurman <wschurman@gmail.com>
quinlanj
changed the title
Co-authored-by: Will Schurman <wschurman@gmail.com>
Why
This PR recaches the values of lower cache layers missing from the higher layers of a ComposedCacheAdapter.
Consider a ComposedCacheAdapter with a local memory cache and a redis cache. In www, values typically stay in Redis much longer than they stay in Local Memory (capped to 5 seconds). In the scenario that a value is already cached in Redis but no longer cached in Local Memory (either it expired or was not the machine that originally cached to Redis), we will consider this a cache hit. Because the only time we cache values is when we fetch from DB, we will continually fetch from Redis until the value is evicted, at which point we will cache back into all the layers.
A better system would be to recache the higher layers with values found in the lower layers so we aren't loading from Redis most of the time.
How
recache higher layers in
loadManyAsyncTest Plan