diff --git a/lib/OnyxCache.ts b/lib/OnyxCache.ts index 6c88d168..08963a89 100644 --- a/lib/OnyxCache.ts +++ b/lib/OnyxCache.ts @@ -34,6 +34,12 @@ class OnyxCache { /** Cache of complete collection data objects for O(1) retrieval */ private collectionData: Record>>; + /** Track which collections have changed since last getCollectionData() call */ + private dirtyCollections: Set; + + /** The stable reference returned last time for each collection */ + private stableCollectionReference: Record> | undefined>; + /** * Captured pending tasks for already running storage methods * Using a map yields better performance on operations such a delete @@ -55,6 +61,14 @@ class OnyxCache { /** Set of collection keys for fast lookup */ private collectionKeys = new Set(); + /** + * Mark a collection as dirty when its data changes + * This ensures getCollectionData() creates a new reference + */ + private markCollectionDirty(collectionKey: OnyxKey): void { + this.dirtyCollections.add(collectionKey); + } + constructor() { this.storageKeys = new Set(); this.nullishStorageKeys = new Set(); @@ -62,6 +76,8 @@ class OnyxCache { this.storageMap = {}; this.collectionData = {}; this.pendingPromises = new Map(); + this.dirtyCollections = new Set(); + this.stableCollectionReference = {}; // bind all public methods to prevent problems with `this` bindAll( @@ -174,6 +190,7 @@ class OnyxCache { // Remove from collection data cache if it's a collection member if (collectionKey && this.collectionData[collectionKey]) { delete this.collectionData[collectionKey][key]; + this.markCollectionDirty(collectionKey); } return undefined; } @@ -186,6 +203,7 @@ class OnyxCache { this.collectionData[collectionKey] = {}; } this.collectionData[collectionKey][key] = value; + this.markCollectionDirty(collectionKey); } return value; @@ -199,11 +217,14 @@ class OnyxCache { const collectionKey = this.getCollectionKey(key); if (collectionKey && this.collectionData[collectionKey]) { delete this.collectionData[collectionKey][key]; + this.markCollectionDirty(collectionKey); } // If this is a collection key, clear its data if (this.isCollectionKey(key)) { delete this.collectionData[key]; + this.dirtyCollections.delete(key); + delete this.stableCollectionReference[key]; } this.storageKeys.delete(key); @@ -238,6 +259,7 @@ class OnyxCache { // Remove from collection data cache if it's a collection member if (collectionKey && this.collectionData[collectionKey]) { delete this.collectionData[collectionKey][key]; + this.markCollectionDirty(collectionKey); } } else { this.nullishStorageKeys.delete(key); @@ -248,6 +270,7 @@ class OnyxCache { this.collectionData[collectionKey] = {}; } this.collectionData[collectionKey][key] = this.storageMap[key]; + this.markCollectionDirty(collectionKey); } } }); @@ -323,6 +346,7 @@ class OnyxCache { const collectionKey = this.getCollectionKey(key); if (collectionKey && this.collectionData[collectionKey]) { delete this.collectionData[collectionKey][key]; + this.markCollectionDirty(collectionKey); } this.recentKeys.delete(key); } @@ -440,6 +464,8 @@ class OnyxCache { return; } this.collectionData[collectionKey] = {}; + // New empty collection - mark as dirty so first getCollectionData creates reference + this.markCollectionDirty(collectionKey); }); } @@ -464,15 +490,26 @@ class OnyxCache { /** * Get all data for a collection key + * Returns stable references when data hasn't changed to prevent unnecessary rerenders */ getCollectionData(collectionKey: OnyxKey): Record> | undefined { + if (!this.dirtyCollections.has(collectionKey) && this.stableCollectionReference[collectionKey]) { + return this.stableCollectionReference[collectionKey]; + } + const cachedCollection = this.collectionData[collectionKey]; if (!cachedCollection || Object.keys(cachedCollection).length === 0) { + this.dirtyCollections.delete(collectionKey); + delete this.stableCollectionReference[collectionKey]; return undefined; } - // Return a shallow copy to ensure React detects changes when items are added/removed - return {...cachedCollection}; + // Collection changed, create new reference + const newReference = {...cachedCollection}; + this.stableCollectionReference[collectionKey] = newReference; + this.dirtyCollections.delete(collectionKey); + + return newReference; } } diff --git a/tests/perf-test/OnyxCache.perf-test.ts b/tests/perf-test/OnyxCache.perf-test.ts index 3a50df70..a8dca769 100644 --- a/tests/perf-test/OnyxCache.perf-test.ts +++ b/tests/perf-test/OnyxCache.perf-test.ts @@ -220,4 +220,59 @@ describe('OnyxCache', () => { }); }); }); + + describe('getCollectionData', () => { + test('one call getting collection with stable reference (10k members)', async () => { + await measureFunction(() => cache.getCollectionData(collectionKey), { + beforeEach: async () => { + resetCacheBeforeEachMeasure(); + cache.setCollectionKeys(new Set([collectionKey])); + // Set all collection members + Object.entries(mockedReportActionsMap).forEach(([k, v]) => cache.set(k, v)); + // First call to create stable reference + cache.getCollectionData(collectionKey); + }, + }); + }); + + test('one call getting collection with dirty collection (10k members)', async () => { + await measureFunction(() => cache.getCollectionData(collectionKey), { + beforeEach: async () => { + resetCacheBeforeEachMeasure(); + cache.setCollectionKeys(new Set([collectionKey])); + // Set all collection members + Object.entries(mockedReportActionsMap).forEach(([k, v]) => cache.set(k, v)); + // Mark collection as dirty by updating a member + cache.set(mockedReportActionsKeys[0], createRandomReportAction(Number(mockedReportActionsMap[mockedReportActionsKeys[0]].reportActionID))); + }, + }); + }); + + test('one call getting collection among 1000 different collections', async () => { + const targetCollectionKey = `collection_100_`; + + await measureFunction(() => cache.getCollectionData(targetCollectionKey), { + beforeEach: async () => { + resetCacheBeforeEachMeasure(); + + // Create 500 collection keys + const collectionKeys = Array.from({length: 1000}, (_, i) => `collection_${i}_`); + cache.setCollectionKeys(new Set(collectionKeys)); + + // Populate each collection with mockedReportActionsMap data + const partOfMockedReportActionsKeys = mockedReportActionsKeys.slice(0, 20); + collectionKeys.forEach((colKey) => { + partOfMockedReportActionsKeys.forEach((originalKey) => { + const memberId = originalKey.replace(collectionKey, ''); + const memberKey = `${colKey}${memberId}`; + cache.set(memberKey, mockedReportActionsMap[originalKey]); + }); + }); + + // First call to create stable reference + cache.getCollectionData(targetCollectionKey); + }, + }); + }); + }); }); diff --git a/tests/unit/onyxCacheTest.tsx b/tests/unit/onyxCacheTest.tsx index 26f9976c..93249274 100644 --- a/tests/unit/onyxCacheTest.tsx +++ b/tests/unit/onyxCacheTest.tsx @@ -414,6 +414,312 @@ describe('Onyx', () => { }); }); }); + + describe.only('getCollectionData', () => { + const COLLECTION_KEY = 'test_collection_'; + const MEMBER_KEY_1 = `${COLLECTION_KEY}member1`; + const MEMBER_KEY_2 = `${COLLECTION_KEY}member2`; + const MEMBER_KEY_3 = `${COLLECTION_KEY}member3`; + + beforeEach(() => { + cache.setCollectionKeys(new Set([COLLECTION_KEY])); + }); + + it('Should return undefined when collection does not exist', () => { + // When getCollectionData is called for non-existent collection + const result = cache.getCollectionData('non_existent_collection_'); + + // Then it should return undefined + expect(result).toBeUndefined(); + }); + + it('Should return undefined when collection is empty', () => { + // Given a collection key is set but has no members + cache.setCollectionKeys(new Set([COLLECTION_KEY])); + + // When getCollectionData is called + const result = cache.getCollectionData(COLLECTION_KEY); + + // Then it should return undefined + expect(result).toBeUndefined(); + }); + + it('Should return new reference when collection is dirty', () => { + // Given a collection with some members + cache.set(MEMBER_KEY_1, {id: 1, value: 'test1'}); + cache.set(MEMBER_KEY_2, {id: 2, value: 'test2'}); + + // When getCollectionData is called (collection is dirty after set) + const result1 = cache.getCollectionData(COLLECTION_KEY); + + // Then it should return a new reference with all members + expect(result1).toBeDefined(); + expect(result1).toEqual({ + [MEMBER_KEY_1]: {id: 1, value: 'test1'}, + [MEMBER_KEY_2]: {id: 2, value: 'test2'}, + }); + }); + + it('Should return stable reference when collection is not dirty', () => { + // Given a collection with some members + cache.set(MEMBER_KEY_1, {id: 1, value: 'test1'}); + cache.set(MEMBER_KEY_2, {id: 2, value: 'test2'}); + + // When getCollectionData is called first time (marks as clean) + const result1 = cache.getCollectionData(COLLECTION_KEY); + + // When getCollectionData is called again (not dirty) + const result2 = cache.getCollectionData(COLLECTION_KEY); + + // Then it should return the same reference + expect(result1).toBe(result2); + expect(result1).toEqual({ + [MEMBER_KEY_1]: {id: 1, value: 'test1'}, + [MEMBER_KEY_2]: {id: 2, value: 'test2'}, + }); + }); + + it('Should return new reference after collection member is added', () => { + // Given a collection with some members + cache.set(MEMBER_KEY_1, {id: 1, value: 'test1'}); + cache.set(MEMBER_KEY_2, {id: 2, value: 'test2'}); + + // When getCollectionData is called first time + const result1 = cache.getCollectionData(COLLECTION_KEY); + + // When a new member is added + cache.set(MEMBER_KEY_3, {id: 3, value: 'test3'}); + + // When getCollectionData is called again + const result2 = cache.getCollectionData(COLLECTION_KEY); + + // Then it should return a new reference with the new member + expect(result1).not.toBe(result2); + expect(result2).toEqual({ + [MEMBER_KEY_1]: {id: 1, value: 'test1'}, + [MEMBER_KEY_2]: {id: 2, value: 'test2'}, + [MEMBER_KEY_3]: {id: 3, value: 'test3'}, + }); + }); + + it('Should return new reference after collection member is updated', () => { + // Given a collection with some members + cache.set(MEMBER_KEY_1, {id: 1, value: 'test1'}); + cache.set(MEMBER_KEY_2, {id: 2, value: 'test2'}); + + // When getCollectionData is called first time + const result1 = cache.getCollectionData(COLLECTION_KEY); + + // When a member is updated + cache.set(MEMBER_KEY_1, {id: 1, value: 'updated'}); + + // When getCollectionData is called again + const result2 = cache.getCollectionData(COLLECTION_KEY); + + // Then it should return a new reference with updated member + expect(result1).not.toBe(result2); + expect(result2).toEqual({ + [MEMBER_KEY_1]: {id: 1, value: 'updated'}, + [MEMBER_KEY_2]: {id: 2, value: 'test2'}, + }); + }); + + it('Should return new reference after collection member is deleted', () => { + // Given a collection with some members + cache.set(MEMBER_KEY_1, {id: 1, value: 'test1'}); + cache.set(MEMBER_KEY_2, {id: 2, value: 'test2'}); + cache.set(MEMBER_KEY_3, {id: 3, value: 'test3'}); + + // When getCollectionData is called first time + const result1 = cache.getCollectionData(COLLECTION_KEY); + + // When a member is deleted + cache.set(MEMBER_KEY_2, null); + + // When getCollectionData is called again + const result2 = cache.getCollectionData(COLLECTION_KEY); + + // Then it should return a new reference without the deleted member + expect(result1).not.toBe(result2); + expect(result2).toEqual({ + [MEMBER_KEY_1]: {id: 1, value: 'test1'}, + [MEMBER_KEY_3]: {id: 3, value: 'test3'}, + }); + }); + + it('Should return undefined after all collection members are deleted', () => { + // Given a collection with some members + cache.set(MEMBER_KEY_1, {id: 1, value: 'test1'}); + cache.set(MEMBER_KEY_2, {id: 2, value: 'test2'}); + + // When getCollectionData is called first time + const result1 = cache.getCollectionData(COLLECTION_KEY); + expect(result1).toBeDefined(); + + // When all members are deleted + cache.set(MEMBER_KEY_1, null); + cache.set(MEMBER_KEY_2, null); + + // When getCollectionData is called again + const result2 = cache.getCollectionData(COLLECTION_KEY); + + // Then it should return undefined + expect(result2).toBeUndefined(); + }); + + it('Should return undefined after collection is dropped', () => { + // Given a collection with some members + cache.set(MEMBER_KEY_1, {id: 1, value: 'test1'}); + cache.set(MEMBER_KEY_2, {id: 2, value: 'test2'}); + + // When getCollectionData is called first time + const result1 = cache.getCollectionData(COLLECTION_KEY); + expect(result1).toBeDefined(); + + // When collection is dropped + cache.drop(COLLECTION_KEY); + + // When getCollectionData is called again + const result2 = cache.getCollectionData(COLLECTION_KEY); + + // Then it should return undefined + expect(result2).toBeUndefined(); + }); + + it('Should return correct data structure with all collection members', () => { + // Given a collection with multiple members + cache.set(MEMBER_KEY_1, {id: 1, value: 'test1'}); + cache.set(MEMBER_KEY_2, {id: 2, value: 'test2'}); + cache.set(MEMBER_KEY_3, {id: 3, value: 'test3'}); + + // When getCollectionData is called + const result = cache.getCollectionData(COLLECTION_KEY); + + // Then it should return an object with all members + expect(result).toBeDefined(); + expect(Object.keys(result!)).toHaveLength(3); + expect(result![MEMBER_KEY_1]).toEqual({id: 1, value: 'test1'}); + expect(result![MEMBER_KEY_2]).toEqual({id: 2, value: 'test2'}); + expect(result![MEMBER_KEY_3]).toEqual({id: 3, value: 'test3'}); + }); + + it('Should maintain stable reference across multiple calls when no changes occur', () => { + // Given a collection with some members + cache.set(MEMBER_KEY_1, {id: 1, value: 'test1'}); + cache.set(MEMBER_KEY_2, {id: 2, value: 'test2'}); + + // When getCollectionData is called multiple times without changes + const result1 = cache.getCollectionData(COLLECTION_KEY); + const result2 = cache.getCollectionData(COLLECTION_KEY); + const result3 = cache.getCollectionData(COLLECTION_KEY); + + // Then all results should be the same reference + expect(result1).toBe(result2); + expect(result2).toBe(result3); + }); + + it('Should handle merge operations correctly', () => { + // Given a collection with some members + cache.set(MEMBER_KEY_1, {id: 1, value: 'test1'}); + cache.set(MEMBER_KEY_2, {id: 2, value: 'test2'}); + + // When getCollectionData is called first time + const result1 = cache.getCollectionData(COLLECTION_KEY); + + // When merge is called with new member + cache.merge({ + [MEMBER_KEY_3]: {id: 3, value: 'test3'}, + }); + + // When getCollectionData is called again + const result2 = cache.getCollectionData(COLLECTION_KEY); + + // Then it should return a new reference with merged data + expect(result1).not.toBe(result2); + expect(result2).toEqual({ + [MEMBER_KEY_1]: {id: 1, value: 'test1'}, + [MEMBER_KEY_2]: {id: 2, value: 'test2'}, + [MEMBER_KEY_3]: {id: 3, value: 'test3'}, + }); + }); + + it('Should return new reference after collection member is dropped', () => { + // Given a collection with some members + cache.set(MEMBER_KEY_1, {id: 1, value: 'test1'}); + cache.set(MEMBER_KEY_2, {id: 2, value: 'test2'}); + cache.set(MEMBER_KEY_3, {id: 3, value: 'test3'}); + + // When getCollectionData is called first time + const result1 = cache.getCollectionData(COLLECTION_KEY); + + // When a member is dropped + cache.drop(MEMBER_KEY_2); + + // When getCollectionData is called again + const result2 = cache.getCollectionData(COLLECTION_KEY); + + // Then it should return a new reference without the dropped member + expect(result1).not.toBe(result2); + expect(result2).toEqual({ + [MEMBER_KEY_1]: {id: 1, value: 'test1'}, + [MEMBER_KEY_3]: {id: 3, value: 'test3'}, + }); + }); + + it.only('Should return new reference after collection member is evicted via removeLeastRecentlyUsedKeys', () => { + // Given a collection with some members + cache.set(MEMBER_KEY_1, {id: 1, value: 'test1'}); + cache.set(MEMBER_KEY_2, {id: 2, value: 'test2'}); + cache.set(MEMBER_KEY_3, {id: 3, value: 'test3'}); + // Set eviction allow list to allow all keys to be evicted when removeLeastRecentlyUsedKeys is called + cache.setEvictionAllowList([MEMBER_KEY_1, MEMBER_KEY_2, MEMBER_KEY_3]); + + // When getCollectionData is called first time + const result1 = cache.getCollectionData(COLLECTION_KEY); + expect(result1).toBeDefined(); + expect(Object.keys(result1!)).toHaveLength(3); + + // When removeLeastRecentlyUsedKeys is called to evict collection members + // Add keys in order - first added will be least recent + cache.addToAccessedKeys(MEMBER_KEY_1); + cache.addToAccessedKeys(MEMBER_KEY_2); + cache.addToAccessedKeys(MEMBER_KEY_3); + // MEMBER_KEY_3 is most recent, so MEMBER_KEY_1 and MEMBER_KEY_2 should be evicted + cache.removeLeastRecentlyUsedKeys(); + + // When getCollectionData is called again + const result2 = cache.getCollectionData(COLLECTION_KEY); + // Then it should return a new reference (collection is dirty after member eviction) + expect(result1).not.toBe(result2); + // Only the most recent member should remain + expect(Object.keys(result2!)).toHaveLength(1); + expect(result2![MEMBER_KEY_3]).toEqual({id: 3, value: 'test3'}); + }); + + it('Should mark collection as dirty when setCollectionKeys is called with new collection', () => { + // Given an existing collection with members + cache.set(MEMBER_KEY_1, {id: 1, value: 'test1'}); + cache.set(MEMBER_KEY_2, {id: 2, value: 'test2'}); + + // When getCollectionData is called first time + const result1 = cache.getCollectionData(COLLECTION_KEY); + expect(result1).toBeDefined(); + + // When a new collection key is added via setCollectionKeys + const NEW_COLLECTION_KEY = 'new_collection_'; + cache.setCollectionKeys(new Set([COLLECTION_KEY, NEW_COLLECTION_KEY])); + + // When getCollectionData is called for the new collection (should be dirty) + const newCollectionResult = cache.getCollectionData(NEW_COLLECTION_KEY); + + // Then it should return undefined (empty collection) + expect(newCollectionResult).toBeUndefined(); + + // And the original collection should still work + const result2 = cache.getCollectionData(COLLECTION_KEY); + expect(result2).toBe(result1); // Same reference, not dirty + }); + }); }); describe('Onyx with Cache', () => {