Perf: Lru cache #93

kidroca · 2021-08-03T21:15:59Z

Details

PR4. Of the changes planned here: #88 (comment)

Keep a list of recently accessed keys in OnyxCache - recentKeys
This serves to clean "Least Recently Used" keys from cache

Keys are added to recentKeys for any cache access operation - get/set/merge

Added a configurable limit of 150 keys in cache
This looks like a sane value from observations during chat browsing

Note on print/benchmark updates:

This helps with extracting information to excel in a consistent matter

Add an optional methods list parameter
When such a list is provided only the methods from this list will be printed

Removed any sorting
Tables order varies and makes it harder to deal with in Excel

Related Issues

Expensify/App#2667

Automated Tests

Added test covering that recent keys are available in cache
and that older keys past the LRU limit are cleared from cache when safe eviction connection happens

Linked PRs

This helps with extracting information to excel in a consistent matter Add an optional `methods` list parameter When such a list is provided only the methods from this list will be printed Removed any sorting Tables order varies and makes it harder to deal with in Excel

Keep a list of recently accessed keys in `OnyxCache` - `recentKeys` This serves to clean "Least Recently Used" keys from cache Keys are added to `recentKeys` for any cache access operation - get/set/merge Added a configurable limit of 150 keys in cache This looks like a sane value from observations during chat browsing

marcaaron

Just leaving some comments as this is still in draft.

marcaaron · 2021-08-03T21:27:22Z

lib/Onyx.js

@@ -742,13 +689,16 @@ function mergeCollection(collectionKey, collection) {
 * @param {function} registerStorageEventListener a callback when a storage event happens.
 * This applies to web platforms where the local storage emits storage events
 * across all open tabs and allows Onyx to stay in sync across all open tabs.
+ * @param {Number} [options.maxCachedKeysCount=150] Sets how many recent keys should we try to keep in cache


Not a blocker, would maybe remove the =150 here since it's possible this could go out of date if someone changes the default at the top of the file.

lib/OnyxCache.js

lib/Onyx.js

Having aliases with no data captured prints empty methods/tables

We shouldn't be in a hurry to clean up cache, we can clean it up periodically every 10, 30 or 60sec. or on another event that happens less frequently

kidroca · 2021-08-04T16:34:04Z

lib/Onyx.js

+        setInterval(() => {
+            // Try to free anything dated from cache
+            cache.removeLeastRecentUsedKeys(maxCachedKeysCount);
+        }, 10 * 1000);


Created this interval based cleanup because trying to free cache after each disconnect is too often
For example when I switch to a new chat - 10 old connections would disconnect at the same time and cause 10 cache cleans, where only 1 would be enough

Instead of interval based solution perhaps we can hook to something that happens less frequently
I think it would be OK to clean cache every minute or even less frequently as long as we clean it

I feel maybe the interval based solution is too aggressive since most of the time we don't really need to clean the cache at all.

What if we hook the cache clean into the connection of a safe eviction key like the reportActions_*. If we are connecting to another large object that seems like a good opportunity to clean the cache in preparation.

Thanks for the idea, that sounds much better, will update

lib/OnyxCache.js

lib/decorateWithMetrics.js

Removed tests covering the old behavior wehere a connection would keep data in cache Added tests that cover periodic cache cleanup and the new LRU functionality

…irst usage" This reverts commit d34cbb6. The functionality is actually needed to prevent duplicate decorations

Remove the setInterval cache cleanup solution Instead try to free cache when a new connection to safe eviction key happens This shouldn't happen very often and as such is ideal for cleaning cache

kidroca · 2021-08-05T22:27:05Z

Updated
Replaced the set interval cache clean trigger with cache clean triggered on safe eviction keys connections

…ethods

kidroca · 2021-08-06T12:17:49Z

I've moved to test the changes here on the latest Expensify
But after capturing some metrics I see there's no much difference in before/after

The cache already contains everything from storage (For the Before run)

But my chat list is small - I've got only 10

Here's why the cache already contains everything from storage

This connection loads all chat actions and is kept for the duration of the app - the old cache logic does not remove items that are connected: https://github.com/Expensify/App/blob/e88b7f984ab4e89e33844260c3a334622bf23023/src/libs/actions/ReportActions.js#L20-L22
General report data used in LHN connects to all report items (and other keys)
Recent optimizations keep some keys in cache forever
- The OnxyProvider related keys (network, personalDetails, report action drafts)
- The initial keys (sesson, account, network, IOU, is sidebar loaded)

The changes in this PR set a default limit of 55 recent keys in cache so they should help reduce the cache footprint for users that have more than 10 Chats (With 10 chats I have 58 unique keys in storage)

I've explained why we don't need to keep items in cache just because they have an active connection here: #93 (comment)

In short if something is "so old" yet still connected after we've cycled 100-150 keys and it didn't re-appear at least once to revive itself - it's not something that will benefit from the "cache while connected" strategy - no other connection/component wants to use it anyway

As a summary the changes here don't seem to provider any performance benefit over the existing strategy, but they are simplifying the code and can help with freeing more cache

marcaaron

Thanks for the breakdown. I don't mind merging these changes anyway since they are an improvement over the previous logic.

Should we get these changes into Expensify/App?

kidroca · 2021-08-06T20:47:02Z

Should we get these changes into Expensify/App?

I can open a PR and test all platforms on Monday

kidroca · 2021-08-09T17:16:41Z

@marcaaron I've Opened this PR: Expensify/App#4509 to update Onyx version in Expensify/App

kidroca added 2 commits August 3, 2021 22:23

marcaaron reviewed Aug 3, 2021

View reviewed changes

kidroca added 4 commits August 4, 2021 19:02

Don't initialize stats[alias] during decoration but only on first usage

d34cbb6

Having aliases with no data captured prints empty methods/tables

Refactor OnyxCache.removeLeastRecentUsedKeys

d01c698

Don't clean cache on disconnect as it happens too often

7ec795b

We shouldn't be in a hurry to clean up cache, we can clean it up periodically every 10, 30 or 60sec. or on another event that happens less frequently

Reduce default cache LRU limit to 55

dd20646

kidroca commented Aug 4, 2021

View reviewed changes

kidroca added 3 commits August 4, 2021 20:36

Update tests to cover cache changes

eca0698

Removed tests covering the old behavior wehere a connection would keep data in cache Added tests that cover periodic cache cleanup and the new LRU functionality

Revert "Don't initialize stats[alias] during decoration but only on f…

00ba943

…irst usage" This reverts commit d34cbb6. The functionality is actually needed to prevent duplicate decorations

Trigger cache clean on safe eviction connections

fbce605

Remove the setInterval cache cleanup solution Instead try to free cache when a new connection to safe eviction key happens This shouldn't happen very often and as such is ideal for cleaning cache

kidroca requested a review from marcaaron August 5, 2021 22:30

kidroca marked this pull request as ready for review August 5, 2021 22:30

kidroca requested a review from a team as a code owner August 5, 2021 22:30

MelvinBot requested review from tylerkaraszewski and removed request for a team August 5, 2021 22:31

kidroca added 2 commits August 6, 2021 14:44

Filter out empty methods, so they don't cause print errors

4d74e39

Add removeLeastRecentlyUsedKeys and setRecentKeysLimit to bound m…

aa7a23a

…ethods

marcaaron approved these changes Aug 6, 2021

View reviewed changes

marcaaron merged commit d7553b9 into Expensify:master Aug 6, 2021

This was referenced Aug 6, 2021

[Hold for payment 2021-08-17] [Performance] Optimize AsyncStorage on Android Expensify/App#2667

Closed

Update Onyx hash (LRU cache) Expensify/App#4509

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perf: Lru cache #93

Perf: Lru cache #93

kidroca commented Aug 3, 2021 •

edited

Loading

marcaaron left a comment

marcaaron Aug 3, 2021

kidroca Aug 4, 2021

kidroca Aug 4, 2021

marcaaron Aug 4, 2021

kidroca Aug 4, 2021

kidroca commented Aug 5, 2021

kidroca commented Aug 6, 2021 •

edited

Loading

marcaaron left a comment

kidroca commented Aug 6, 2021

kidroca commented Aug 9, 2021 •

edited

Loading

Perf: Lru cache #93

Perf: Lru cache #93

Conversation

kidroca commented Aug 3, 2021 • edited Loading

Details

Note on print/benchmark updates:

Related Issues

Automated Tests

Linked PRs

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Aug 3, 2021

Choose a reason for hiding this comment

kidroca Aug 4, 2021

Choose a reason for hiding this comment

kidroca Aug 4, 2021

Choose a reason for hiding this comment

marcaaron Aug 4, 2021

Choose a reason for hiding this comment

kidroca Aug 4, 2021

Choose a reason for hiding this comment

kidroca commented Aug 5, 2021

kidroca commented Aug 6, 2021 • edited Loading

marcaaron left a comment

Choose a reason for hiding this comment

kidroca commented Aug 6, 2021

kidroca commented Aug 9, 2021 • edited Loading

kidroca commented Aug 3, 2021 •

edited

Loading

kidroca commented Aug 6, 2021 •

edited

Loading

kidroca commented Aug 9, 2021 •

edited

Loading