Repository navigation
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
Cleaning every value read by OnyxUtils.get() walked the whole value and made the Reassure get test 41% slower. The only provider that could hand out nested nulls is SQLite, because JSON_REPLACE stores the replacement object as is. Clean the replacement objects before they are stored, and clean values that older versions already stored when they are read, skipping the walk for objects whose JSON has no null.
|
I have read the CLA Document and I hereby sign the CLA |
|
I'll review this one. Let me know once it's ready for review. Thanks! |
|
@MelvinBot review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@fabioh8010 could you please take a look too? thanks! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94edaee019
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const jsonPath = `$.${pathArray.join('.')}`; | ||
| return [jsonPath, JSON.stringify(value), key]; | ||
| // JSON_REPLACE stores the object as is, so nested nulls are removed first. | ||
| return [jsonPath, JSON.stringify(utils.removeNestedNullValues(value as OnyxValue<OnyxKey>)), key]; |
There was a problem hiding this comment.
In a native batch that removes and recreates overlapping objects—for example merges {a: null}, {a: {b: null}}, then {a: {b: {c: 1}}}—mergeAndMarkChanges emits $.a -> {b: null} followed by $.a.b -> {c: 1}. Cleaning the first patch here turns it into {}; after SQLite applies that outer JSON_REPLACE, the subsequent JSON_REPLACE cannot create the now-missing $.a.b path, so storage ends as {a: {}} while the cache has {a: {b: {c: 1}}}, losing data on restart. Preserve or fold these null placeholders until dependent patches have been applied, and only then normalize.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed, the replace patches are stored as they were before this PR, and nested nulls are removed on read instead (getItem, cache.hydrate, cache.merge), so the cache still never holds them. Added a regression test for nested replacements in SQLiteProviderTest.ts
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
fabioh8010
left a comment
fabioh8010
left a comment
There was a problem hiding this comment.
@arekm213 In case you didn't before, could you run some perf benchmarks of before/after this change around some main metrics? For example:
- ManualNavigateToInboxTab
- ManualOpenReport
- ManualAppStartup
- ManualSendMessageVisible
Just to make sure we are safe perf-wise
| } | ||
|
|
||
| if (value === nullFreeReference) { | ||
| if (process.env.NODE_ENV === 'development' && needsNormalization(value)) { |
There was a problem hiding this comment.
| if (process.env.NODE_ENV === 'development' && needsNormalization(value)) { | |
| if ((process.env.NODE_ENV === 'development' || process.env.NODE_ENV === 'test') && needsNormalization(value)) { |
We should log during tests too, no?
| /** | ||
| * Deep removes the nested null values from the given value. Returns the original reference if no nulls were found. | ||
| * | ||
| * Nested objects that are the same reference as the object at the same path in `nullFreeReference` are returned as is, | ||
| * without being traversed. Pass the live cache value here, since cached values never hold nested nulls. | ||
| */ |
There was a problem hiding this comment.
NAB: arrays are never cleaned (neither null elements nor nulls in objects inside them), so "never hold nested nulls" isn't fully true. Could we mention arrays here? Otherwise someone may add array cleaning later and break the skip without noticing.
|
@Krishna2323 could you take a look on the PR when you get a chance? Thanks! |
|
I'll review it today, probably in an hour or two. |
| if (!isKeyToPreserve) { | ||
| const oldValue = cache.get(key); | ||
| const newValue = defaultKeyStates[key] ?? null; | ||
| const newValue = utils.removeNestedNullValues(defaultKeyStates[key] ?? null); |
There was a problem hiding this comment.
NAB: removeNestedNullValues() returns a new object on every clear() when the default holds nested nulls, so newValue !== oldValue is always true. Example: default {errors: null, isLoading: false}, 3 calls to clear(). A useOnyx hook without a selector re-renders 1 time on main, 3 times on this PR.
| continue; | ||
| } | ||
|
|
||
| const value = utils.needsNormalization(valueFromStorage) ? utils.removeNestedNullValues(valueFromStorage) : valueFromStorage; |
There was a problem hiding this comment.
NAB: No test covers this fix. If this line is changed back to const value = valueFromStorage;, all tests still pass.
| if (!isKeyToPreserve) { | ||
| const oldValue = cache.get(key); | ||
| const newValue = defaultKeyStates[key] ?? null; | ||
| const newValue = utils.removeNestedNullValues(defaultKeyStates[key] ?? null); |
There was a problem hiding this comment.
NAB: No test covers this either. Reverting to defaultKeyStates[key] ?? null keeps all 693 tests passing. The randomized null-free cache test never calls clear().
Details
Every
Onyx.set(andmultiSet,setCollection, new members inmergeCollection) runsremoveNestedNullValuesover the whole value before caching and storing it. The App rewrites some large values after almost every update, for example derived values that hold one entry per report, so each write walks the entire object even when only one field changed.Since #765, values built from the cached one keep the same object reference for every subtree that didn't change. This PR uses that:
Skip subtrees the cache already holds.
removeNestedNullValues(value, nullFreeReference)takes the current cached value and returns any nested object that is the same reference as in the cache as is, without walking it. The cache holds it, so it has no nulls. The returned objects are exactly the same as before (same references, same values), so subscribers get the same values and notifications.setWithRetrypasses the existing value,prepareKeyValuePairsForStoragepassescache.get(key).Close the four paths that put nested nulls into the cache today, since step 1 relies on the cache being null-free:
mergeObjectnow cleans the replacement object whenshouldRemoveNestedNullsis set (lib/utils.ts).JSON_REPLACEstores the replacement objects of these merges as they are, nulls included, and lazy reads then put them into the cache.getItemnow cleans them on read, and only walks objects whose JSON containsnull, so reads of values without nulls cost the same as before. They are not cleaned before they are stored, because a later patch in the same batch can target a path inside an earlier replacement, and that path must still exist when the later patch runs (lib/storage/providers/SQLiteProvider.ts).multiGetnow hands out what the cache holds aftercache.merge(temp), so callers and cache agree (lib/OnyxUtils.ts). On web, IndexedDBgetItemcleans the same way, since older versions could also store nested nulls there (lib/storage/providers/IDBKeyValProvider/index.ts).cache.set(lib/Onyx.ts).Onyx.clear(): default key states are cleaned beforecache.set. For example the App's default foraccountis{errors: null, ...}, so after a clear subscribers saw anerrorskey that every other path omits (lib/Onyx.ts).On these paths subscribers now get an omitted key instead of
{key: null}, the same as on every other path.Dev-only guard. If a skipped subtree does hold nulls, App code mutated a cached object in place, which Onyx doesn't support. In development this logs an alert, so it is caught early. In production the check is skipped, because it would need the walk this PR removes.
Prior work:
skipNullRemoval(#655, #659) was removed in #666 because the App mutated shared nested objects. This PR still removes nulls from everything that changed and only skips subtrees that are the same reference as the cache. Draft #751 removes null pruning entirely, which would change what subscribers receive. This PR doesn't.Results (release builds, heavy test account, median before vs. after; 8 runs per variant on an iOS simulator and 6 on an Android emulator, web in Chrome with CPU x4 throttling):
On top of the pending OnyxDerived change that skips writes of unchanged derived values (callstack-internal/Expensify-App#76, part of Expensify/App#98956), the gain holds on web: each Pusher update 341 to 316 ms (-7.2%), incremental reconnect 580 to 532 ms (-5.5%), faster in 5 of 5 runs.
Reassure: the
gettest is unchanged against main.setCollectionwith 10k heavy objects can show up to about +8% locally. That test replaces every object with a new one, so nothing is shared and the lookup in the cached value is pure overhead. In the App, writes mostly reuse cached objects.Related Issues
Expensify/App#103616
Linked E/App PR
Expensify/App#103978
Automated Tests
utilsTest.ts:removeNestedNullValueswith a reference. It returns the same output as without one for any null-free reference on randomized (seeded) inputs, returns the reference itself as is, doesn't traverse shared subtrees, still removes nulls from subtrees that differ, and ignores references that aren't plain objects. Also covers the replace branch ofmergeObjectcleaning only whenshouldRemoveNestedNullsis true.SQLiteProviderTest.ts(real SQLite engine): a replacement nested inside an earlier replacement in the same batch is applied, andgetItemremoves nested nulls from a stored value while leaving arrays as they are.IDBKeyvalProviderTest.ts:getItemremoves nested nulls from a stored value while leaving arrays as they are.onyxTest.ts(null-free cache): the native replace branch and a batched remove-then-replace update,multiGethanding out the cleaned cached value,setandmultiSetkeeping cached subtrees while removing new nulls, the dev alert on a mutated shared subtree, and a randomized test that runs sequences ofset,merge,multiSet,mergeCollectionandupdatewith nested nulls and checks that every cached value stays null-free.Manual Tests
Tested with the App pinned to this branch (Expensify/App#103978), against the production API.
errorskey from the defaultaccountstate is omitted now).removeNestedNullValues skipped a cached subtreealert during the steps above.Done on web with a high traffic account (read only) and a smaller account (including writes), and on all native and mobile web platforms with a test account. No new console errors and no dev alerts. When
Onyx.clear()ran on sign in or on a Copilot switch, the only null this PR removed waserrorsfrom the defaultaccountstate, as expected.Author Checklist
### Related Issuessection above### Linked E/App PRsection above, and verified this change against it (E/App CI passed and manual testing completed)TestssectiontoggleReportand 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
No UI change. Smoke tested on all platforms listed above, no errors or regressions found.