-
Notifications
You must be signed in to change notification settings - Fork 94
Load all storage data into cache during Onyx.init #752
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,7 +100,9 @@ let lastSubscriptionID = 0; | |
| // Connections can be made before `Onyx.init`. They would wait for this task before resolving | ||
| const deferredInitTask = createDeferredTask(); | ||
|
|
||
| // Holds a set of collection member IDs which updates will be ignored when using Onyx methods. | ||
| // Collection member IDs that Onyx should silently ignore across all operations — reads, writes, cache, and subscriber | ||
| // notifications. This is used to filter out keys formed from invalid/default IDs (e.g. "-1", "0", | ||
| // "undefined", "null", "NaN") that can appear when an ID variable is accidentally coerced to string. | ||
| let skippableCollectionMemberIDs = new Set<string>(); | ||
| // Holds a set of keys that should always be merged into snapshot entries. | ||
| let snapshotMergeKeys = new Set<string>(); | ||
|
|
@@ -1111,19 +1113,76 @@ function mergeInternal<TValue extends OnyxInput<OnyxKey> | undefined, TChange ex | |
| * Merge user provided default key value pairs. | ||
| */ | ||
| function initializeWithDefaultKeyStates(): Promise<void> { | ||
| // Filter out RAM-only keys from storage reads as they may have stale persisted data | ||
| // from before the key was migrated to RAM-only. | ||
| const keysToFetch = Object.keys(defaultKeyStates).filter((key) => !isRamOnlyKey(key)); | ||
| return Storage.multiGet(keysToFetch).then((pairs) => { | ||
| const existingDataAsObject = Object.fromEntries(pairs) as Record<string, unknown>; | ||
|
|
||
| const merged = utils.fastMerge(existingDataAsObject, defaultKeyStates, { | ||
| shouldRemoveNestedNulls: true, | ||
| }).result; | ||
| cache.merge(merged ?? {}); | ||
|
|
||
| for (const [key, value] of Object.entries(merged ?? {})) keyChanged(key, value); | ||
| }); | ||
| // Eagerly load the entire database into cache in a single batch read. | ||
| // This is faster than lazy-loading individual keys because: | ||
| // 1. One DB transaction instead of hundreds | ||
| // 2. All subsequent reads are synchronous cache hits | ||
| return Storage.getAll() | ||
| .then((pairs) => { | ||
| const allDataFromStorage: Record<string, unknown> = {}; | ||
| for (const [key, value] of pairs) { | ||
| // RAM-only keys should not be cached from storage as they may have stale persisted data | ||
| // from before the key was migrated to RAM-only. | ||
| if (isRamOnlyKey(key)) { | ||
| continue; | ||
| } | ||
|
|
||
| // Skip collection members that are marked as skippable | ||
fabioh8010 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (skippableCollectionMemberIDs.size && getCollectionKey(key)) { | ||
| const [, collectionMemberID] = splitCollectionMemberKey(key); | ||
|
|
||
| if (skippableCollectionMemberIDs.has(collectionMemberID)) { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| allDataFromStorage[key] = value; | ||
fabioh8010 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| // Load all storage data into cache silently (no subscriber notifications) | ||
| cache.setAllKeys(Object.keys(allDataFromStorage)); | ||
| cache.merge(allDataFromStorage); | ||
|
|
||
| // For keys that have a developer-defined default (via `initialKeyStates`), merge the | ||
| // persisted value with the default so new properties added in code updates are applied | ||
| // without wiping user data that already exists in storage. | ||
| const defaultKeysFromStorage = Object.keys(defaultKeyStates).reduce((obj: Record<string, unknown>, key) => { | ||
| if (key in allDataFromStorage) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| obj[key] = allDataFromStorage[key]; | ||
| } | ||
| return obj; | ||
| }, {}); | ||
|
|
||
| const merged = utils.fastMerge(defaultKeysFromStorage, defaultKeyStates, { | ||
| shouldRemoveNestedNulls: true, | ||
| }).result; | ||
| cache.merge(merged ?? {}); | ||
|
|
||
| // Notify subscribers about default key states so that any subscriber that connected | ||
| // before init (e.g. during module load) receives the merged default values immediately | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. Who connects before? I guess we do not block subscriptions on init phase?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes from a first glance I agree it shouldn't be needed as we are now waiting the operations until initialisation is done, but I remember I got a couple of test failures in E/App when we didn't include this part. I will have a look again. |
||
| for (const [key, value] of Object.entries(merged ?? {})) { | ||
| keyChanged(key, value); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| Logger.logAlert(`Failed to load data from storage during init. The app will boot with default key states only. Error: ${error}`); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably keep track of this in logs somehow. I'd be very curious to see the cases where this throws and why.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if I understood your comment, but we are already sending these logs with |
||
|
|
||
| // Populate the key index so getAllKeys() returns correct results for default keys. | ||
| // Without this, subscribers that check getAllKeys() would see an empty set even | ||
| // though we have default values in cache. | ||
| cache.setAllKeys(Object.keys(defaultKeyStates)); | ||
|
|
||
| // Boot with defaults so the app renders instead of deadlocking. | ||
| // Users will get a fresh-install experience but the app won't be bricked. | ||
| cache.merge(defaultKeyStates); | ||
|
|
||
| // Notify subscribers about default key states so that any subscriber that connected | ||
| // before init (e.g. during module load) receives the merged default values immediately | ||
| for (const [key, value] of Object.entries(defaultKeyStates)) { | ||
| keyChanged(key, value); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -198,6 +198,17 @@ const provider: StorageProvider<NitroSQLiteConnection | undefined> = { | |
| return (result ?? []) as StorageKeyList; | ||
| }); | ||
| }, | ||
| getAll() { | ||
| if (!provider.store) { | ||
| throw new Error('Store is not initialized!'); | ||
| } | ||
|
|
||
| return provider.store.executeAsync<OnyxSQLiteKeyValuePair>('SELECT record_key, valueJSON FROM keyvaluepairs;').then(({rows}) => { | ||
| // eslint-disable-next-line no-underscore-dangle | ||
| const result = rows?._array.map((row) => [row.record_key, JSON.parse(row.valueJSON)]); | ||
| return (result ?? []) as StorageKeyValuePair[]; | ||
| }); | ||
|
Comment on lines
+206
to
+210
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You tested this on a high traffic account right? I wonder if parsing all JSON synchronously may slow the app down or even block it. It's unlikely, just worth checking imo.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with the concern. Maybe you can set up some performance timing for this? I'd like to know what impact it will have on real accounts, especially the large ones.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. results from dev environment with 33k keys:
It's hard to compare it 1:1 with current solution, because on main this cost is spread across the entire startup - but in general |
||
| }, | ||
| removeItem(key) { | ||
| if (!provider.store) { | ||
| throw new Error('Store is not initialized!'); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.