[PERF] OnyxUtils keyChanged cached collection retrieval optimisation #577
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
kacper-mikolajczak
force-pushed
the
perf/keyChanged
branch
from
332e4ca
to
cb15a80
Compare
kacper-mikolajczak
changed the title
There was a problem hiding this comment.
Comjng from Slack, this seems quite straightforward. Can you create App issue for this with details and link it? Also update the cla and checklist, thank you!
@hannojg @blazejkustra do you want to give it a quick review too
| @@ -804,6 +804,9 @@ function keyChanged<TKey extends OnyxKey>( | |||
| return; | |||
| } | |||
| } | |||
|
|
|||
| const cachedCollections: Record<string, ReturnType<typeof getCachedCollection>> = {}; | |||
There was a problem hiding this comment.
cachedCollections stores all collections all in one object? Let's verify that we have enough memory for that 😅
There was a problem hiding this comment.
Haha :D
On serious note, it won't store all the collections, only those from subscriber.key. Also the reference is hold for the keyChange lifespan and the object size is the same as we've got from getCachedCollection.
Furthermore, it will hold a reference to only one of such collection object, whereas multiple calls to getCachedCollection spawn many of those.
Let me know if my understanding is correct here, thanks!
|
Changes look good to me as well. Just wanted to check with y'all if you think we can solve this more holistically - discussion here |
@mountiny Done ✅ ! |
Thanks for your input @hannojg ❤️ I think it's definitely worth looking into how the cache retrieval process works! For now, let's go ahead with the current improvement. It should still have a positive impact, even when we consider the changes you've suggested. What do you think? |
|
Continuation of Slack thread where Hanno wrote:
// OnyxCache.ts (pseudo code)
const collectionKeyMap = new Map<string, string>();
set(key: OnyxKey, value: OnyxValue<OnyxKey>): OnyxValue<OnyxKey> {
if (isCollectionKey(key) {
collectionKeyMap[key] = splitCollectionMemberKey(key)[1]
}
}
getCachedCollection(key: CollectionKey) {
const result = {}
Object.values(collectionKeyMap[key]).forEach(...)
return result
}
Options B seems more practical but I am not sure of what would be the memory impact of storing such collections once again just to serve as a middleman between in-memory cache and end-user. Let's think of how we can improve the idea even further. Having that said @mountiny, what are the next steps if there is a positive response from reviewers? |
Absolutely, lets do that 😊 |
Maybe, instead of having an extra copy just for collections, we could store collections and regular keys separately in the cache (so the data is only there once) |
@hannojg Though I agree with this but if we do this it would require us adding some if condition in I was thinking if we can do something like this: diff --git a/lib/OnyxCache.ts b/lib/OnyxCache.ts
index 530f8e6..69da4f6 100644
--- a/lib/OnyxCache.ts
+++ b/lib/OnyxCache.ts
@@ -9,7 +9,7 @@ import type {OnyxKey, OnyxValue} from './types';
*/
class OnyxCache {
/** Cache of all the storage keys available in persistent storage */
- private storageKeys: Set<OnyxKey>;
+ private storageKeys: Map<string, Set<OnyxKey>>;
/** A list of keys where a nullish value has been fetched from storage before, but the key still exists in cache */
private nullishStorageKeys: Set<OnyxKey>;
@@ -30,7 +30,7 @@ class OnyxCache {
private maxRecentKeysSize = 0;
constructor() {
- this.storageKeys = new Set();
+ this.storageKeys = new Map();
this.nullishStorageKeys = new Set();
this.recentKeys = new Set();
this.storageMap = {};
@@ -59,8 +59,8 @@ class OnyxCache {
}
/** Get all the storage keys */
- getAllKeys(): Set<OnyxKey> {
- return this.storageKeys;
+ getAllKeys(key: string): Set<OnyxKey> {
+ return this.storageKeys.get(key) || new Set();
}
/**
@@ -74,15 +74,15 @@ class OnyxCache {
*
* @param keys - an array of keys
*/
- setAllKeys(keys: OnyxKey[]) {
- this.storageKeys = new Set(keys);
+ setAllKeys(key: string, keys: OnyxKey[]) {
+ this.storageKeys.set(key, new Set(keys));
}
/** Saves a key in the storage keys list
* Serves to keep the result of `getAllKeys` up to date
*/
addKey(key: OnyxKey): void {
- this.storageKeys.add(key);
+ this.storageKeys.set(key, this.storageKeys.get(key)?.add(key) || new Set([key]));
}
/** Used to set keys that are null/undefined in storage without adding null to the storage map */
|
|
I have read the CLA Document and I hereby sign the CLA |
|
All yours @MariaHCD |
|
🚀Published to npm in v2.0.62 |
Details
PR introduces an improvement to how
getCachedCollectionis called inkeyChanged.After investigating this trace, we've noticed that when looping through subscribers list in
keyChanged, thegetCachedCollectionis called for every iteration with the same key.To do:
getCachedCollectionresults to reuse them between iterations (similar to what we have inkeysChanged)I could not reproduce a case from the trace where
getCachedCollectionis called on each iteration to compare the performance gains. By comparing PR perf to main there was no regression seen and from static analysis we should help when the aforementioned edge case happens.Related Issues
Expensify/App#46863
Automated Tests
Manual Tests
Author Checklist
### Related Issuessection aboveTestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop