From 1b89bc83722eb838fb69abc1e3fc332a42bffd14 Mon Sep 17 00:00:00 2001 From: hurali97 Date: Tue, 5 Mar 2024 18:13:21 +0500 Subject: [PATCH 1/4] perf: avoid re-creating array each time getAllKeys is called --- lib/OnyxCache.ts | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/lib/OnyxCache.ts b/lib/OnyxCache.ts index 14908d46d..862dd8f7c 100644 --- a/lib/OnyxCache.ts +++ b/lib/OnyxCache.ts @@ -28,11 +28,25 @@ class OnyxCache { /** Maximum size of the keys store din cache */ private maxRecentKeysSize = 0; + /** + * Array to store the keys that are created from the storageKeys using + * `Array.from` method. This is used to prevent creating a new array + * every time `getAllKeys` is called. + */ + private storageKeysArray: Key[]; + + /** + * Flag to indicate if the keys have changed. + */ + private hasKeysChanged: boolean; + constructor() { this.storageKeys = new Set(); this.recentKeys = new Set(); this.storageMap = {}; this.pendingPromises = new Map(); + this.storageKeysArray = []; + this.hasKeysChanged = false; // bind all public methods to prevent problems with `this` bindAll( @@ -54,7 +68,11 @@ class OnyxCache { /** Get all the storage keys */ getAllKeys(): Key[] { - return Array.from(this.storageKeys); + if (this.hasKeysChanged) { + this.storageKeysArray = Array.from(this.storageKeys); + } + this.hasKeysChanged = false; + return this.storageKeysArray; } /** @@ -78,6 +96,7 @@ class OnyxCache { */ addKey(key: Key): void { this.storageKeys.add(key); + this.hasKeysChanged = true; } /** @@ -97,6 +116,7 @@ class OnyxCache { delete this.storageMap[key]; this.storageKeys.delete(key); this.recentKeys.delete(key); + this.hasKeysChanged = true; } /** @@ -110,6 +130,7 @@ class OnyxCache { this.storageMap = {...utils.fastMerge(this.storageMap, data, false)}; + this.hasKeysChanged = true; const storageKeys = this.getAllKeys(); const mergedKeys = Object.keys(data); this.storageKeys = new Set([...storageKeys, ...mergedKeys]); From 5a27fab765c7ec2b2a51c84485731c81e5d910ac Mon Sep 17 00:00:00 2001 From: hurali97 Date: Tue, 5 Mar 2024 19:11:52 +0500 Subject: [PATCH 2/4] fix: move the hasKeyChanged flag to bottom --- lib/OnyxCache.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/OnyxCache.ts b/lib/OnyxCache.ts index 862dd8f7c..1b0b2d99f 100644 --- a/lib/OnyxCache.ts +++ b/lib/OnyxCache.ts @@ -130,10 +130,13 @@ class OnyxCache { this.storageMap = {...utils.fastMerge(this.storageMap, data, false)}; - this.hasKeysChanged = true; const storageKeys = this.getAllKeys(); const mergedKeys = Object.keys(data); this.storageKeys = new Set([...storageKeys, ...mergedKeys]); + + // set the flag to indicate that the keys have changed + this.hasKeysChanged = true; + mergedKeys.forEach((key) => this.addToAccessedKeys(key)); } From d00b8cb8c777887a119188b7e71ec50a833f1e1f Mon Sep 17 00:00:00 2001 From: hurali97 Date: Tue, 12 Mar 2024 17:14:54 +0500 Subject: [PATCH 3/4] refactor: use Set instead of Array for getAllKeys method in OnyxCache --- lib/Onyx.js | 43 +++++++++++++++++++++++++++---------- lib/OnyxCache.ts | 27 ++--------------------- tests/unit/onyxCacheTest.js | 18 ++++++++-------- tests/unit/onyxTest.js | 6 +++--- 4 files changed, 46 insertions(+), 48 deletions(-) diff --git a/lib/Onyx.js b/lib/Onyx.js index 47cf0c166..bef5ab6cc 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -167,12 +167,12 @@ function get(key) { /** * Returns current key names stored in persisted storage * @private - * @returns {Promise} + * @returns {Promise>} */ function getAllKeys() { // When we've already read stored keys, resolve right away const storedKeys = cache.getAllKeys(); - if (storedKeys.length > 0) { + if (storedKeys.size > 0) { return Promise.resolve(storedKeys); } @@ -186,7 +186,8 @@ function getAllKeys() { // Otherwise retrieve the keys from storage and capture a promise to aid concurrent usages const promise = Storage.getAllKeys().then((keys) => { _.each(keys, (key) => cache.addKey(key)); - return keys; + // return the updated set of keys + return cache.getAllKeys(); }); return cache.captureTask(taskName, promise); @@ -254,10 +255,18 @@ function tryGetCachedValue(key, mapping = {}) { // It is possible we haven't loaded all keys yet so we do not know if the // collection actually exists. - if (allCacheKeys.length === 0) { + if (allCacheKeys.size === 0) { return; } - const matchingKeys = _.filter(allCacheKeys, (k) => k.startsWith(key)); + + const matchingKeys = []; + allCacheKeys.forEach((k) => { + if (!k.startsWith(key)) { + return; + } + matchingKeys.push(k); + }); + const values = _.reduce( matchingKeys, (finalObject, matchedKey) => { @@ -360,7 +369,7 @@ function addToEvictionBlockList(key, connectionID) { function addAllSafeEvictionKeysToRecentlyAccessedList() { return getAllKeys().then((keys) => { _.each(evictionAllowList, (safeEvictionKey) => { - _.each(keys, (key) => { + keys.forEach((key) => { if (!isKeyMatch(safeEvictionKey, key)) { return; } @@ -376,8 +385,13 @@ function addAllSafeEvictionKeysToRecentlyAccessedList() { * @returns {Object} */ function getCachedCollection(collectionKey) { - const collectionMemberKeys = _.filter(cache.getAllKeys(), (storedKey) => isCollectionMemberKey(collectionKey, storedKey)); - + const collectionMemberKeys = []; + cache.getAllKeys().forEach((storedKey) => { + if (!isCollectionMemberKey(collectionKey, storedKey)) { + return; + } + collectionMemberKeys.push(storedKey); + }); return _.reduce( collectionMemberKeys, (prev, curr) => { @@ -901,7 +915,14 @@ function connect(mapping) { // We search all the keys in storage to see if any are a "match" for the subscriber we are connecting so that we // can send data back to the subscriber. Note that multiple keys can match as a subscriber could either be // subscribed to a "collection key" or a single key. - const matchingKeys = _.filter(keys, (key) => isKeyMatch(mapping.key, key)); + + const matchingKeys = []; + keys.forEach((key) => { + if (!isKeyMatch(mapping.key, key)) { + return; + } + matchingKeys.push(key); + }); // If the key being connected to does not exist we initialize the value with null. For subscribers that connected // directly via connect() they will simply get a null value sent to them without any information about which key matched @@ -1381,7 +1402,7 @@ function clear(keysToPreserve = []) { // status, or activeClients need to remain in Onyx even when signed out) // 2. Any keys with a default state (because they need to remain in Onyx as their default, and setting them // to null would cause unknown behavior) - _.each(keys, (key) => { + keys.forEach((key) => { const isKeyToPreserve = _.contains(keysToPreserve, key); const isDefaultKey = _.has(defaultKeyStates, key); @@ -1488,7 +1509,7 @@ function mergeCollection(collectionKey, collection) { return true; }) .keys() - .partition((key) => persistedKeys.includes(key)) + .partition((key) => persistedKeys.has(key)) .value(); const existingKeyCollection = _.pick(collection, existingKeys); diff --git a/lib/OnyxCache.ts b/lib/OnyxCache.ts index 1b0b2d99f..50ce0a075 100644 --- a/lib/OnyxCache.ts +++ b/lib/OnyxCache.ts @@ -28,25 +28,11 @@ class OnyxCache { /** Maximum size of the keys store din cache */ private maxRecentKeysSize = 0; - /** - * Array to store the keys that are created from the storageKeys using - * `Array.from` method. This is used to prevent creating a new array - * every time `getAllKeys` is called. - */ - private storageKeysArray: Key[]; - - /** - * Flag to indicate if the keys have changed. - */ - private hasKeysChanged: boolean; - constructor() { this.storageKeys = new Set(); this.recentKeys = new Set(); this.storageMap = {}; this.pendingPromises = new Map(); - this.storageKeysArray = []; - this.hasKeysChanged = false; // bind all public methods to prevent problems with `this` bindAll( @@ -67,12 +53,8 @@ class OnyxCache { } /** Get all the storage keys */ - getAllKeys(): Key[] { - if (this.hasKeysChanged) { - this.storageKeysArray = Array.from(this.storageKeys); - } - this.hasKeysChanged = false; - return this.storageKeysArray; + getAllKeys(): Set { + return this.storageKeys; } /** @@ -96,7 +78,6 @@ class OnyxCache { */ addKey(key: Key): void { this.storageKeys.add(key); - this.hasKeysChanged = true; } /** @@ -116,7 +97,6 @@ class OnyxCache { delete this.storageMap[key]; this.storageKeys.delete(key); this.recentKeys.delete(key); - this.hasKeysChanged = true; } /** @@ -134,9 +114,6 @@ class OnyxCache { const mergedKeys = Object.keys(data); this.storageKeys = new Set([...storageKeys, ...mergedKeys]); - // set the flag to indicate that the keys have changed - this.hasKeysChanged = true; - mergedKeys.forEach((key) => this.addToAccessedKeys(key)); } diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index 0b6ead7ad..5a78d1760 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -26,7 +26,7 @@ describe('Onyx', () => { const allKeys = cache.getAllKeys(); // Then the result should be empty - expect(allKeys).toEqual([]); + expect(allKeys).toEqual(new Set()); }); it('Should keep storage keys', () => { @@ -37,7 +37,7 @@ describe('Onyx', () => { // Then the keys should be stored in cache const allKeys = cache.getAllKeys(); - expect(allKeys).toEqual(['mockKey', 'mockKey2', 'mockKey3']); + expect(allKeys).toEqual(new Set(['mockKey', 'mockKey2', 'mockKey3'])); }); it('Should keep storage keys even when no values are provided', () => { @@ -48,7 +48,7 @@ describe('Onyx', () => { // Then the keys should be stored in cache const allKeys = cache.getAllKeys(); - expect(allKeys).toEqual(['mockKey', 'mockKey2', 'mockKey3']); + expect(allKeys).toEqual(new Set(['mockKey', 'mockKey2', 'mockKey3'])); }); it('Should not store duplicate keys', () => { @@ -62,7 +62,7 @@ describe('Onyx', () => { // Then getAllKeys should not include a duplicate value const allKeys = cache.getAllKeys(); - expect(allKeys).toEqual(['mockKey', 'mockKey2', 'mockKey3']); + expect(allKeys).toEqual(new Set(['mockKey', 'mockKey2', 'mockKey3'])); }); }); @@ -121,7 +121,7 @@ describe('Onyx', () => { expect(cache.hasCacheForKey('mockKey')).toBe(false); // Then but a key should be available - expect(cache.getAllKeys()).toEqual(expect.arrayContaining(['mockKey'])); + expect(cache.getAllKeys()).toEqual(new Set(['mockKey'])); }); it('Should not make duplicate keys', () => { @@ -135,7 +135,7 @@ describe('Onyx', () => { // Then getAllKeys should not include a duplicate value const allKeys = cache.getAllKeys(); - expect(allKeys).toEqual(['mockKey', 'mockKey2']); + expect(allKeys).toEqual(new Set(['mockKey', 'mockKey2'])); }); }); @@ -158,7 +158,7 @@ describe('Onyx', () => { cache.set('mockKey', {value: 'mockValue'}); // Then but a key should be available - expect(cache.getAllKeys()).toEqual(expect.arrayContaining(['mockKey'])); + expect(cache.getAllKeys()).toEqual(new Set(['mockKey'])); }); it('Should overwrite existing cache items for the Given key', () => { @@ -186,7 +186,7 @@ describe('Onyx', () => { // Then a value should not be available in cache expect(cache.hasCacheForKey('mockKey')).toBe(false); expect(cache.getValue('mockKey')).not.toBeDefined(); - expect(cache.getAllKeys('mockKey').includes('mockKey')).toBe(false); + expect(cache.getAllKeys('mockKey').has('mockKey')).toBe(false); }); }); @@ -336,7 +336,7 @@ describe('Onyx', () => { }); // Then getAllStorage keys should return updated storage keys - expect(cache.getAllKeys()).toEqual(['mockKey', 'mockKey2', 'mockKey3', 'mockKey4']); + expect(cache.getAllKeys()).toEqual(new Set(['mockKey', 'mockKey2', 'mockKey3', 'mockKey4'])); }); it('Should throw if called with anything that is not an object', () => { diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index 36b005cd0..5879edcfc 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -40,18 +40,18 @@ describe('Onyx', () => { Onyx.set(ONYX_KEYS.OTHER_TEST, 42) .then(() => Onyx.getAllKeys()) .then((keys) => { - expect(keys.includes(ONYX_KEYS.OTHER_TEST)).toBe(true); + expect(keys.has(ONYX_KEYS.OTHER_TEST)).toBe(true); return Onyx.set(ONYX_KEYS.OTHER_TEST, null); }) .then(() => { // Checks if cache value is removed. - expect(cache.getAllKeys().length).toBe(0); + expect(cache.getAllKeys().size).toBe(0); // When cache keys length is 0, we fetch the keys from storage. return Onyx.getAllKeys(); }) .then((keys) => { - expect(keys.includes(ONYX_KEYS.OTHER_TEST)).toBe(false); + expect(keys.has(ONYX_KEYS.OTHER_TEST)).toBe(false); })); it('should set a simple key', () => { From 73a1e8235585234b9efebcdda8b89410a53a7111 Mon Sep 17 00:00:00 2001 From: hurali97 Date: Tue, 12 Mar 2024 17:19:35 +0500 Subject: [PATCH 4/4] refactor: remove extra line --- lib/OnyxCache.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/OnyxCache.ts b/lib/OnyxCache.ts index 3fcbcd9d1..9ec447ed4 100644 --- a/lib/OnyxCache.ts +++ b/lib/OnyxCache.ts @@ -114,7 +114,6 @@ class OnyxCache { const storageKeys = this.getAllKeys(); const mergedKeys = Object.keys(data); this.storageKeys = new Set([...storageKeys, ...mergedKeys]); - mergedKeys.forEach((key) => this.addToAccessedKeys(key)); }