Skip to content

[Structural Sharing] PR 3: Frozen collection snapshots with lazy rebuild and per-key merge #768

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fabioh8010 wants to merge 6 commits into
base: main
Choose a base branch
from
+304 −48
Open

[Structural Sharing] PR 3: Frozen collection snapshots with lazy rebuild and per-key merge #768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3217c90
Merge branch 'feature/structural-sharing-cache-pr-2' into feature/str…
fabioh8010 Mar 19, 2026
aace5a9
perf: frozen collection snapshots with lazy rebuild and per-key merge
fabioh8010 Mar 19, 2026
bb1a329
Merge branch 'main' into feature/structural-sharing-cache-pr-3
fabioh8010 Apr 1, 2026
a200a0d
Merge branch 'main' into feature/structural-sharing-cache-pr-3
fabioh8010 Apr 3, 2026
7f580ce
Add unit tests
fabioh8010 Apr 3, 2026
eac6774
fix: use longest-prefix matching in rebuildCollectionSnapshot fallbac…
fabioh8010 Apr 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
199 changes: 151 additions & 48 deletions lib/OnyxCache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,19 @@ import {deepEqual} from 'fast-equals';
import bindAll from 'lodash/bindAll';
import type {ValueOf} from 'type-fest';
import utils from './utils';
import type {OnyxKey, OnyxValue} from './types';
import type {CollectionKeyBase, KeyValueMapping, NonUndefined, OnyxCollection, OnyxKey, OnyxValue} from './types';
import OnyxKeys from './OnyxKeys';

/** Frozen object containing all collection members — safe to return by reference */
type CollectionSnapshot = Readonly<NonUndefined<OnyxCollection<KeyValueMapping[OnyxKey]>>>;
Copy link
Copy Markdown
Contributor

@roryabraham roryabraham Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand there's a high cost to deep-freezing the object at runtime, but should we consider using ReadonlyDeep from type-fest to prevent accidental mutations lower in the tree at compile time?


/**
* Stable frozen empty object used as the canonical value for empty collections.
* Returning the same reference avoids unnecessary re-renders in useSyncExternalStore,
* which relies on === equality to detect changes.
*/
const FROZEN_EMPTY_COLLECTION: Readonly<NonUndefined<OnyxCollection<KeyValueMapping[OnyxKey]>>> = Object.freeze({});

// Task constants
const TASK = {
GET: 'get',
Expand All @@ -31,9 +41,6 @@ class OnyxCache {
/** A map of cached values */
private storageMap: Record<OnyxKey, OnyxValue<OnyxKey>>;

/** Cache of complete collection data objects for O(1) retrieval */
private collectionData: Record<OnyxKey, Record<OnyxKey, OnyxValue<OnyxKey>>>;

/**
* Captured pending tasks for already running storage methods
* Using a map yields better performance on operations such a delete
Expand All @@ -52,13 +59,20 @@ class OnyxCache {
/** List of keys that have been directly subscribed to or recently modified from least to most recent */
private recentlyAccessedKeys = new Set<OnyxKey>();

/** Frozen collection snapshots for structural sharing */
private collectionSnapshots: Map<OnyxKey, CollectionSnapshot>;

/** Collections whose snapshots need rebuilding (lazy — rebuilt on next read) */
private dirtyCollections: Set<CollectionKeyBase>;

constructor() {
this.storageKeys = new Set();
this.nullishStorageKeys = new Set();
this.recentKeys = new Set();
this.storageMap = {};
this.collectionData = {};
this.pendingPromises = new Map();
this.collectionSnapshots = new Map();
this.dirtyCollections = new Set();

// bind all public methods to prevent problems with `this`
bindAll(
Expand Down Expand Up @@ -88,8 +102,8 @@ class OnyxCache {
'addEvictableKeysToRecentlyAccessedList',
'getKeyForEviction',
'setCollectionKeys',
'getCollectionData',
'hasValueChanged',
'getCollectionData',
);
}

Expand Down Expand Up @@ -168,24 +182,21 @@ class OnyxCache {
this.nullishStorageKeys.delete(key);

const collectionKey = OnyxKeys.getCollectionKey(key);
const oldValue = this.storageMap[key];

if (value === null || value === undefined) {
delete this.storageMap[key];

// Remove from collection data cache if it's a collection member
if (collectionKey && this.collectionData[collectionKey]) {
delete this.collectionData[collectionKey][key];
if (collectionKey && oldValue !== undefined) {
this.dirtyCollections.add(collectionKey);
}
return undefined;
}

this.storageMap[key] = value;

// Update collection data cache if this is a collection member
if (collectionKey) {
if (!this.collectionData[collectionKey]) {
this.collectionData[collectionKey] = {};
}
this.collectionData[collectionKey][key] = value;
if (collectionKey && oldValue !== value) {
this.dirtyCollections.add(collectionKey);
}

return value;
Expand All @@ -195,15 +206,14 @@ class OnyxCache {
drop(key: OnyxKey): void {
delete this.storageMap[key];

// Remove from collection data cache if this is a collection member
const collectionKey = OnyxKeys.getCollectionKey(key);
if (collectionKey && this.collectionData[collectionKey]) {
delete this.collectionData[collectionKey][key];
if (collectionKey) {
this.dirtyCollections.add(collectionKey);
}

// If this is a collection key, clear its data
// If this is a collection key, clear its snapshot
if (OnyxKeys.isCollectionKey(key)) {
delete this.collectionData[key];
this.collectionSnapshots.delete(key);
}

this.storageKeys.delete(key);
Expand All @@ -220,38 +230,55 @@ class OnyxCache {
throw new Error('data passed to cache.merge() must be an Object of onyx key/value pairs');
}

this.storageMap = {
...utils.fastMerge(this.storageMap, data, {
shouldRemoveNestedNulls: true,
objectRemovalMode: 'replace',
}).result,
};
const affectedCollections = new Set<OnyxKey>();

for (const [key, value] of Object.entries(data)) {
this.addKey(key);
this.addToAccessedKeys(key);

const collectionKey = OnyxKeys.getCollectionKey(key);

if (value === null || value === undefined) {
if (value === undefined) {
this.addNullishStorageKey(key);
// undefined means "no change" — skip storageMap modification
continue;
}

if (value === null) {
this.addNullishStorageKey(key);
delete this.storageMap[key];

// Remove from collection data cache if it's a collection member
if (collectionKey && this.collectionData[collectionKey]) {
delete this.collectionData[collectionKey][key];
if (collectionKey) {
affectedCollections.add(collectionKey);
}
} else {
this.nullishStorageKeys.delete(key);

// Update collection data cache if this is a collection member
// Per-key merge instead of spreading the entire storageMap
const existing = this.storageMap[key];
const merged = utils.fastMerge(existing, value, {
shouldRemoveNestedNulls: true,
objectRemovalMode: 'replace',
}).result;

// fastMerge is reference-stable: returns the original target when
// nothing changed, so a simple === check detects no-ops.
if (merged === existing) {
continue;
}

this.storageMap[key] = merged;

if (collectionKey) {
if (!this.collectionData[collectionKey]) {
this.collectionData[collectionKey] = {};
}
this.collectionData[collectionKey][key] = this.storageMap[key];
affectedCollections.add(collectionKey);
}
}
}

// Mark affected collections as dirty — snapshots will be lazily rebuilt on next read
for (const collectionKey of affectedCollections) {
this.dirtyCollections.add(collectionKey);
}
}

/**
Expand Down Expand Up @@ -317,26 +344,35 @@ class OnyxCache {
iterResult = iterator.next();
}

const affectedCollections = new Set<OnyxKey>();

for (const key of keysToRemove) {
delete this.storageMap[key];

// Remove from collection data cache if this is a collection member
// Track affected collections for snapshot rebuild
const collectionKey = OnyxKeys.getCollectionKey(key);
if (collectionKey && this.collectionData[collectionKey]) {
delete this.collectionData[collectionKey][key];
if (collectionKey) {
affectedCollections.add(collectionKey);
Copy link
Copy Markdown
Collaborator

@tgolen tgolen Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than having a separate affectedCollections variable (which is kind of confusing), could you add these directly to this.dirtyollections?

}
this.recentKeys.delete(key);
}

for (const collectionKey of affectedCollections) {
this.dirtyCollections.add(collectionKey);
}
}

/** Set the recent keys list size */
setRecentKeysLimit(limit: number): void {
this.maxRecentKeysSize = limit;
}

/** Check if the value has changed */
/** Check if the value has changed. Uses reference equality as a fast path, falls back to deep equality. */
hasValueChanged(key: OnyxKey, value: OnyxValue<OnyxKey>): boolean {
const currentValue = this.get(key, false);
const currentValue = this.storageMap[key];
if (currentValue === value) {
return false;
}
return !deepEqual(currentValue, value);
}

Expand Down Expand Up @@ -425,26 +461,93 @@ class OnyxCache {
setCollectionKeys(collectionKeys: Set<OnyxKey>): void {
OnyxKeys.setCollectionKeys(collectionKeys);

// Initialize collection data for existing collection keys
// Initialize frozen snapshots for collection keys
for (const collectionKey of collectionKeys) {
if (this.collectionData[collectionKey]) {
if (!this.collectionSnapshots.has(collectionKey)) {
this.collectionSnapshots.set(collectionKey, Object.freeze({}));
}
}
}

/**
* Rebuilds the frozen collection snapshot from current storageMap references.
* Uses the indexed collection->members map for O(collectionMembers) instead of O(totalKeys).
* Returns the previous snapshot reference when all member references are identical,
* preventing unnecessary re-renders in useSyncExternalStore.
*
* @param collectionKey - The collection key to rebuild
*/
private rebuildCollectionSnapshot(collectionKey: OnyxKey): void {
const oldSnapshot = this.collectionSnapshots.get(collectionKey);
Copy link
Copy Markdown
Collaborator

@tgolen tgolen Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename to previousSnapshot or currentSnapshot. I don't think "old" is as helpful.


const members: NonUndefined<OnyxCollection<KeyValueMapping[OnyxKey]>> = {};
let hasChanges = false;
Copy link
Copy Markdown
Collaborator

@tgolen tgolen Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename this to be a little more descriptive? What has changes? The old snapshot?

let newMemberCount = 0;

// Use the indexed forward lookup for O(collectionMembers) iteration.
// Falls back to scanning all storageKeys if the index isn't populated yet.
const memberKeys = OnyxKeys.getMembersOfCollection(collectionKey);
const keysToScan = memberKeys ?? this.storageKeys;
const needsPrefixCheck = !memberKeys;

for (const key of keysToScan) {
if (needsPrefixCheck && OnyxKeys.getCollectionKey(key) !== collectionKey) {
Copy link
Copy Markdown
Collaborator

@tgolen tgolen Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a code comment to explain why this early continue is there (I'm not sure I understand why).

continue;
}
this.collectionData[collectionKey] = {};
const val = this.storageMap[key];
if (val !== undefined && val !== null) {
members[key] = val;
newMemberCount++;

// Check if this member's reference changed from the old snapshot
if (!hasChanges && (!oldSnapshot || oldSnapshot[key] !== val)) {
hasChanges = true;
}
}
}

// Check if any members were removed (old snapshot had more keys)
if (!hasChanges && oldSnapshot) {
const oldMemberCount = Object.keys(oldSnapshot).length;
if (oldMemberCount !== newMemberCount) {
Copy link
Copy Markdown
Collaborator

@tgolen tgolen Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this logic safe? What if Key A was removed and Key B was added? The counts would be the same, but the snapshot would be different.

hasChanges = true;
}
}

// If nothing actually changed, reuse the old snapshot reference.
// This is critical: useSyncExternalStore uses === to detect changes,
// so returning the same reference prevents unnecessary re-renders.
if (!hasChanges && oldSnapshot) {
return;
}

Object.freeze(members);

this.collectionSnapshots.set(collectionKey, members);
}

/**
* Get all data for a collection key
* Get all data for a collection key.
* Returns a frozen snapshot with structural sharing — safe to return by reference.
* Lazily rebuilds the snapshot if the collection was modified since the last read.
*/
getCollectionData(collectionKey: OnyxKey): Record<OnyxKey, OnyxValue<OnyxKey>> | undefined {
const cachedCollection = this.collectionData[collectionKey];
if (!cachedCollection || Object.keys(cachedCollection).length === 0) {
if (this.dirtyCollections.has(collectionKey)) {
this.rebuildCollectionSnapshot(collectionKey);
this.dirtyCollections.delete(collectionKey);
}

const snapshot = this.collectionSnapshots.get(collectionKey);
if (!snapshot || Object.keys(snapshot).length === 0) {
Copy link
Copy Markdown
Contributor

@roryabraham roryabraham Apr 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

micro-optimization: Skip an array allocation of all the keys - it's not necessary for an empty check. Using for-in instead will stop as soon as the first value is found:

diff --git a/lib/OnyxCache.ts b/lib/OnyxCache.ts
index 063ea5eb..6980e69c 100644
--- a/lib/OnyxCache.ts
+++ b/lib/OnyxCache.ts
@@ -538,7 +538,7 @@ class OnyxCache {
         }
 
         const snapshot = this.collectionSnapshots.get(collectionKey);
-        if (!snapshot || Object.keys(snapshot).length === 0) {
+        if (utils.isEmptyObject(snapshot)) {
             // If we know we have storage keys loaded, return a stable empty reference
             // to avoid new {} allocations that break useSyncExternalStore === equality.
             if (this.storageKeys.size > 0) {
diff --git a/lib/utils.ts b/lib/utils.ts
index 1fca1c8a..efdc5173 100644
--- a/lib/utils.ts
+++ b/lib/utils.ts
@@ -172,7 +172,17 @@ function mergeObject<TObject extends Record<string, unknown>>(
 
 /** Checks whether the given object is an object and not null/undefined. */
 function isEmptyObject<T>(obj: T | EmptyValue): obj is EmptyValue {
-    return typeof obj === 'object' && Object.keys(obj || {}).length === 0;
+    if (typeof obj !== 'object') {
+        return false;
+    }
+    // Use is for-in loop to avoid using Object.keys to do an unnecessary array allocation
+    // eslint-disable-next-line no-restricted-syntax
+    for (const key in obj) {
+        if (Object.hasOwn(obj, key)) {
+            return false;
+        }
+    }
+    return true;
 }
 
 /**

This should be faster (O(1) instead of O(n)), particularly for objects with lots of keys

// If we know we have storage keys loaded, return a stable empty reference
// to avoid new {} allocations that break useSyncExternalStore === equality.
if (this.storageKeys.size > 0) {
return FROZEN_EMPTY_COLLECTION;
}
return undefined;
Comment on lines +544 to 547
Copy link
Copy Markdown
Contributor

@Julesssss Julesssss Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is feedback from Claude so I could be mistaken, but should we be checking for storage keys from a specific key? Rather than any key.

The decision between "this collection is empty" vs "this collection hasn't loaded yet"
hinges entirely on whether storageKeys has any key in it — from any collection or
non-collection key.

  • getCollectionData("report_") is called before the report collection has loaded from
    storage.

So in theory, subscribers might temporarily receive an empty object instead of FROZEN_EMPTY_COLLECTION?

}

// Return a shallow copy to ensure React detects changes when items are added/removed
return {...cachedCollection};
return snapshot;
}
}

Expand Down
Loading
Loading