Prevent updating values in cache when they do not change by marcaaron · Pull Request #191 · Expensify/react-native-onyx

marcaaron · 2022-10-05T20:01:24Z

cc @iwiznia @tgolen

Details

There are many cases where we are updating values in the cache that have not changed
We are also triggering updates to components when the values have not changed
That leads to unnecessary re-renders and writing to storage data that is basically already there
Yeah, we are using isEqual() whenever writing data - which seems like it would be "inefficient", but the savings from avoiding re-renders appear much greater than any performance hit caused by the value equality check.

Related Issues

https://github.com/Expensify/Expensify/issues/230464

Automated Tests

Added some automated tests to show that we will prevent updates when the values don't change.

Linked PRs

TODO, but the changes can be tested against main

Benchmarks

While timing a switch_report event on Android these are some before and after readings

Screenshots

Android

Before

DEBUG Timing:expensify.cash.switch_report.cold 6078
DEBUG Timing:expensify.cash.switch_report.cold 6699
DEBUG Timing:expensify.cash.switch_report.cold 7158
DEBUG Timing:expensify.cash.switch_report.cold 7359
DEBUG Timing:expensify.cash.switch_report.cold 7606

After

DEBUG Timing:expensify.cash.switch_report.cold 2850
DEBUG Timing:expensify.cash.switch_report.cold 2872
DEBUG Timing:expensify.cash.switch_report.cold 3051
DEBUG Timing:expensify.cash.switch_report.cold 3218
DEBUG Timing:expensify.cash.switch_report.cold 3372

iOS

Before

DEBUG Timing:expensify.cash.switch_report.cold 1708
DEBUG Timing:expensify.cash.switch_report.cold 1724
DEBUG Timing:expensify.cash.switch_report.cold 1739
DEBUG Timing:expensify.cash.switch_report.cold 1631
DEBUG Timing:expensify.cash.switch_report.cold 1656

After

DEBUG Timing:expensify.cash.switch_report.cold 1176
DEBUG Timing:expensify.cash.switch_report.cold 1134
DEBUG Timing:expensify.cash.switch_report.cold 1098
DEBUG Timing:expensify.cash.switch_report.cold 1135
DEBUG Timing:expensify.cash.switch_report.cold 1092

Web

Before

Timing:expensify.cash.switch_report.cold 297
Timing:expensify.cash.switch_report.cold 276
Timing:expensify.cash.switch_report.cold 281
Timing:expensify.cash.switch_report.cold 278
Timing:expensify.cash.switch_report.cold 284

After

Timing:expensify.cash.switch_report.cold 186
Timing:expensify.cash.switch_report.cold 188
Timing:expensify.cash.switch_report.cold 186
Timing:expensify.cash.switch_report.cold 180
Timing:expensify.cash.switch_report.cold 185

…ryChanges

tgolen · 2022-10-05T20:29:27Z

lib/Onyx.js

    for (let i = stateMappingKeys.length; i--;) {
        const subscriber = callbackToStateMapping[stateMappingKeys[i]];
-        if (!subscriber || !isKeyMatch(subscriber.key, key)) {
+        if (!subscriber || !isKeyMatch(subscriber.key, key) || (_.isFunction(canUpdateSubscriber) && !canUpdateSubscriber(subscriber))) {


NAB: Is this new logic the same thing as using _.result(subscriber, 'canUpdateSubscriber')? https://underscorejs.org/#result

Maybe not... I think I am thinking of this: https://github.com/Expensify/expensify-common/blob/main/lib/Func.jsx#L13-L19 which would be cool if we could just import it without any additional work :D I don't think that's possible though.

_.result() takes a property name as it's second arg so it's a little different

What you are asking sounds similar to Str.result() and I think would work if we did:

Str.result(canUpdateSubscriber, subscriber)

and would return undefined if the first argument was undefined

tgolen · 2022-10-05T20:30:38Z

lib/Onyx.js

+    // If the value in the cache is the same as what we have then do not update subscribers unless they
+    // have initWithStoredValues: false then they MUST get all updates even if nothing has changed.
+    if (!cache.hasValueChanged(key, value)) {
+        notifySubscribersOnNextTick(key, value, subscriber => subscriber.initWithStoredValues === false);


Suggested change

notifySubscribersOnNextTick(key, value, subscriber => subscriber.initWithStoredValues === false);

notifySubscribersOnNextTick(key, value, !subscriber.initWithStoredValues);

Why does the third argument need to be a function? Can it be a simple boolean instead?

It doesn't have to be a function. I had it as a simple boolean, but feel the function is more intuitive and also puts the context for why we are doing what we are doing where we are doing it. Check this commit to get a feel for what I mean:

8c7b7d9

tgolen · 2022-10-05T20:31:52Z

lib/OnyxCache.js

+     */
+    hasValueChanged(key, value) {
+        this.addToAccessedKeys(key);
+        return !_.isEqual(this.storageMap[key], value);


Did you by any chance explore other deep equal solutions? If we're doing this for performance reasons, then maybe we should try to find the fastest one?

I explored this one dequal and it does seem faster. But decided to use _.isEqual() since we already have the underscore dependency.

So I went and benchmarked a few of these today and tried to compare what I think are our biggest data types here's what I came back with:

Comparing two sets of very large personalDetails with one user displayName changed

_.isEqual() x 1,248 ops/sec ±0.43% (96 runs sampled) dequal() x 277 ops/sec ±2.30% (86 runs sampled) deepEqual() x 1,231 ops/sec ±0.65% (93 runs sampled) fastDeepEqual() x 1,130 ops/sec ±0.84% (93 runs sampled) microDiff() x 214 ops/sec ±1.58% (87 runs sampled) Fastest is _.isEqual()

Comparing two report objects one with a different lastVisitedTimestamp

_.isEqual() x 1,857,706 ops/sec ±0.17% (92 runs sampled) dequal() x 2,556,428 ops/sec ±0.35% (93 runs sampled) deepEqual() x 2,457,190 ops/sec ±0.33% (96 runs sampled) fastDeepEqual() x 1,768,857 ops/sec ±0.09% (99 runs sampled) microDiff() x 795,347 ops/sec ±0.19% (96 runs sampled) Fastest is dequal()

Comparing two reportActions objects each with 250 comments and one with an additional comment

_.isEqual() x 258,482 ops/sec ±0.39% (95 runs sampled) dequal() x 3,394 ops/sec ±0.47% (97 runs sampled) deepEqual() x 260,473 ops/sec ±0.98% (94 runs sampled) fastDeepEqual() x 268,251 ops/sec ±0.33% (96 runs sampled) microDiff() x 1,828 ops/sec ±0.41% (99 runs sampled) Fastest is fastDeepEqual()

So going off of these numbers it looks to me like:

dequal is trailing hard for very large objects, but best at small stuff

_.isEqual seems pretty comparable to other libs at the large stuff

Based on that I think it's not really worth using dequal or any of the others. I'd be more concerned about performance with these really large objects and underscore isn't too far behind the competition.

deepEqual looks like the best option here, no?
Given this is super low level code that will get called everywhere, I think is worth to add a new dependency for the sake of performance

Cool I'm good with that. I wasn't sure if there was enough of a meaningful difference between that and _.isEquals()

tests/unit/onyxTest.js

tgolen · 2022-10-05T20:40:42Z

tests/unit/onyxTest.js

+            })
+            .then(() => {
+                // WHEN merge is called again with an object of equivalent value but not the same reference
+                Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_POLICY}${1}`, _.clone(collectionUpdate.test_policy_1));


I think this makes sense, that a different reference wouldn't trigger the callback. I've seen so many bugs in the past where code thinks a variable is one reference, but it ends up being another, and there is stale data... It makes me leary, but until it's an actual problem, I think it's OK.

Mainly wanted to show here that whether it's a reference or not makes no difference since we are doing a value comparison and not reference comparison. In the previous test, we pass a reference and in the new test we pass a new object. Both have the same value so both should not trigger any data to be set or updates to occur.

I've seen so many bugs in the past where code thinks a variable is one reference, but it ends up being another, and there is stale data... It makes me leary, but until it's an actual problem, I think it's OK.

I don't think we have any problems like this or I have not observed any at least. I am pretty confident we are always creating new object references before saving them to the cache (at least when using the Onyx.merge() method)

e.g.

react-native-onyx/lib/Onyx.js

Line 766 in 0846e16

const newData = Object.assign({}, fastMerge(modifiedData, mergeValue));

marcaaron · 2022-10-05T22:49:12Z

Tested on iOS, Android, and Web and updated the switch_report metrics in the description. Going to take this out of draft.

iwiznia · 2022-10-06T12:18:38Z

lib/OnyxCache.js

+     */
+    hasValueChanged(key, value) {
+        this.addToAccessedKeys(key);
+        return !_.isEqual(this.storageMap[key], value);


deepEqual looks like the best option here, no?
Given this is super low level code that will get called everywhere, I think is worth to add a new dependency for the sake of performance

tests/unit/onyxTest.js

API.md

lib/OnyxCache.js

…ryChanges

marcaaron · 2022-10-06T18:57:40Z

Updated and using the fast-equals package instead of underscore. Going to have to re-test to make sure everything is still working after that (but I think it should be pretty much the same).

marcaaron · 2022-10-07T19:57:51Z

re-tested and ready for another review whenever 🙇

tgolen · 2022-10-10T16:38:22Z

@PauloGasparSv @iwiznia all yours

marcaaron · 2022-10-11T18:17:44Z

Looks like @iwiznia is ooo but does not have any unresolved comments so going to pull the merge trigger on this one.

marcaaron added 8 commits October 4, 2022 17:38

Use simple deep equal idea instead

23a135c

just use underscore

5e93a95

Merge remote-tracking branch 'origin' into marcaaron-preventUnnecessa…

483ff70

…ryChanges

Add comments for onlyUpdateSubscribersWithoutInitialValues

0e63c88

Use a filter for the subscribers

8c7b7d9

Add test for preventing updates when values do not change

2fbf183

Add new test

1ee97da

Update docs

17e0903

marcaaron self-assigned this Oct 5, 2022

tgolen requested changes Oct 5, 2022

View reviewed changes

marcaaron added 3 commits October 5, 2022 11:32

Clean up tests

d049e6d

Clear up some waitForPromisesToResolve

194e555

Add test for ensuring that we return a promise from Onyx.set()

b24806b

marcaaron marked this pull request as ready for review October 5, 2022 22:49

marcaaron requested a review from a team as a code owner October 5, 2022 22:49

marcaaron requested a review from tgolen October 5, 2022 22:49

melvin-bot bot requested review from PauloGasparSv and removed request for a team October 5, 2022 22:49

iwiznia reviewed Oct 6, 2022

View reviewed changes

marcaaron added 2 commits October 6, 2022 08:39

Merge remote-tracking branch 'origin' into marcaaron-preventUnnecessa…

72feff5

…ryChanges

Make requested changes. De-capitalize test comments

0181cf0

tgolen approved these changes Oct 10, 2022

View reviewed changes

sketchydroide approved these changes Oct 11, 2022

View reviewed changes

PauloGasparSv approved these changes Oct 11, 2022

View reviewed changes

marcaaron merged commit 4efd75c into main Oct 11, 2022

marcaaron mentioned this pull request Oct 11, 2022

[HOLD Web-Expensify/pull/35134] Add DEBUG_ONYX_SET_STATE mode + bump Onyx version to 1.0.22 to help prevent re-renders Expensify/App#11598

Closed

90 tasks

roryabraham mentioned this pull request Oct 18, 2022

[No QA] Upgrade Onyx to 1.0.24 Expensify/App#11945

Merged

92 tasks

	notifySubscribersOnNextTick(key, value, subscriber => subscriber.initWithStoredValues === false);
	notifySubscribersOnNextTick(key, value, !subscriber.initWithStoredValues);

Conversation

marcaaron commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Related Issues

Automated Tests

Linked PRs

Benchmarks

Screenshots

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Oct 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marcaaron commented Oct 6, 2022

Uh oh!

marcaaron commented Oct 7, 2022

Uh oh!

tgolen commented Oct 10, 2022

Uh oh!

marcaaron commented Oct 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marcaaron commented Oct 5, 2022 •

edited

Loading