Batch updates second try #315

Szymon20000 · 2023-08-23T16:27:21Z

The Problem this pull-request solves

One of the main problems with react-native-onyx is that it renders every component in a separate micro-task. Because right now we don't use concurrent rendering every setState not within the same event listener will cause a separate render. Every react render has certain overhead so if it's possible to render 2 state changes together we will only pay for the overhead once. Additionally we introduce additional overhead in render side-effects. For instance in SidebarLinks component we compute which report should be visible. That method is heavy and we run it almost on every render. If we batch 2 setStates together that would save us one such computation. This pull-requests batches all setStates caused by onyx changes that occurs within the same microtask cycle. That saves a ton of time.

Examples of the problem

Imagine there are 10 components that are listening to a current report changes. Currently all those components will be notified separately
Imagine we have one component that listens to 4 different onyx props that are changed one by one (SidebarLinks is such component) right now we will render is as many times as we have changes. With this pr we will merge it only once per micotask cycle what will avoid a lot of recomputation

Note that is change won't be needed anymore once we use concurrent rendering.

Details

We use unstable_batchedUpdates method from react (That is currently used in even listeners so it's pretty safe).
This method triggers single react render for all the setState changes passed to it.

How it works

For every notifySubscribers or collection subscribers we don't queue the update on micro task queue instead we add that change to pending changes list that is flushed at the end of the current micro task cycle. We do it by setting macro task which is setTimeout(() => {}, 0).

Related Issues

GH_LINK

Automated Tests

Linked PRs

…zymon/batch_updates_2_try

Li357 · 2023-08-28T15:24:21Z

Is this ready for review?

Szymon20000 · 2023-08-28T17:00:04Z

We are still adjusting unit tests in Expensfiy/App to this change but we also don't expect a need to modify this pr.
Feel free to review it :)

Julesssss · 2023-08-31T08:00:52Z

lib/Onyx.js

+            pendingCollectionUpdates = [];
+            unstable_batchedUpdates(() => {
+                for (let i = 0; i < updatesCopy.length; ++i) {
+                    keyChanged(updatesCopy[i][0], updatesCopy[i][1], updatesCopy[i][2]);


I couldn't find anything that confirms that the pendingUpdate|updatesCopy is has a 3rd child. Presumably, this is guaranteed as that's how our Onyx updates are written?

If by 3rd child you mean the 3 argument at updatesCopy[i][2] then it will at least always be undefined.

The keyChanged method has 3 arguments so we only delay the execution.

Julesssss

While this looks good, it would be great to test the component renders to prove the updates are now batched.

tgolen

Is there any way to write some unit tests to verify that the updates were batched as expected? Something like... have a mocked function that is triggered on render, and updating multiple properties that the component is listening to triggers the render callback the right number of times.

This is an example test of what I'm thinking of:

react-native-onyx/tests/unit/withOnyxTest.js

Lines 59 to 77 in ad5533e

    
           it('should update withOnyx subscriber multiple times when merge is used', () => { 
        
               const TestComponentWithOnyx = withOnyx({ 
        
                   text: { 
        
                       key: ONYX_KEYS.COLLECTION.TEST_KEY, 
        
                   }, 
        
               })(ViewWithCollections); 
        
               const onRender = jest.fn(); 
        
               render(<TestComponentWithOnyx onRender={onRender} />); 
        
               return waitForPromisesToResolve() 
        
                   .then(() => { 
        
                       Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {ID: 123}); 
        
                       Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}2`, {ID: 234}); 
        
                       return Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}3`, {ID: 345}); 
        
                   }) 
        
                   .then(() => { 
        
                       expect(onRender).toHaveBeenCalledTimes(4); 
        
                   }); 
        
           });

tgolen · 2023-08-31T16:43:26Z

jestSetup.js

 jest.mock('react-native-device-info', () => ({getFreeDiskStorage: () => {}}));
 jest.mock('react-native-quick-sqlite', () => ({
    open: () => ({execute: () => {}}),
 }));
+
+jest.useRealTimers();


Why is this added here? Do you mind adding a code comment to explain?

Hanno added that because otherwise setTimeout is not executed unless we call jest.advanceTime(). So to avoid calling that everywhere we used real times.

tgolen · 2023-08-31T16:45:42Z

lib/Onyx.js

+// [key, value, canUpdateSubscriber]
+let pendingUpdates = [];
+let scheduledPromise;
+let pendingCollectionUpdates = [];


Could you also please add a doc comment for this (like you did for pendingUpdates)?

tgolen · 2023-08-31T16:46:06Z

lib/Onyx.js

+    }
+
+    scheduledPromise = new Promise((resolve) => {
+        setTimeout(() => {


Please add a code comment explaining why it's necessary to have a setTimout(0) here.

I added a comment explaining when the batch will be flushed. Explaining how timers and batching works in react native is not possible in a single comment so I simplified it a bit.

tgolen · 2023-08-31T16:48:54Z

lib/Onyx.js

@@ -20,6 +21,7 @@ const METHOD = {

 // Key/value store of Onyx key and arrays of values to merge
 const mergeQueue = {};
+const mergeQueuePromise = {};


I followed most of the changes in this PR except for where it came to mergeQueuePromise. Could you add some code comments to give a general idea around what it's for and how it's used?

This change is mostly for adjusting tests from what I remember.
Looks like we use it in tests for making sure all the pending merges has been applied.

marcaaron

Just small typos to fix now. I will resolve my NABs.

lib/Onyx.js

Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>

Szymon20000 · 2023-09-06T09:58:22Z

The App tests are failing with this pr. I will let you know when they are fixed.
I just realised that we only want to batch withOnyx subscribers as regular ones should be notified right away.

Szymon20000 · 2023-09-06T11:44:39Z

@tgolen @marcaaron Are there any places in the app when we trigger Onyx update in Onyx.connect subscriber?
If that's the case then I think we shouldn't batch them.
I can image that we have places like that when once onyx.update triggers another one.
So for examples. update A -> update B -> update C would require 3 batches now.
If you think we may have such case in the app I create a pr that fixes that problem. margelo#1

tgolen · 2023-09-06T16:17:35Z

I just looked at all the usages of Onyx.update and there are none that happen inside a connect() subscriber.

marcaaron · 2023-09-06T19:46:37Z

I just realised that we only want to batch withOnyx subscribers as regular ones should be notified right away

Oh, right, because they do not call setState(). Makes sense.

Are there any places in the app when we trigger Onyx update in Onyx.connect subscriber?
If that's the case then I think we shouldn't batch them.

Sorry, I did not follow the reasoning. Is there some kind of critical problem that we will see if we batch those updates?

Szymon20000 · 2023-09-07T09:50:26Z

Ah if we don't change state in onyx connect then probably it doesn't make any difference. I did it because I thought you may have some reactive code that is triggered once we merge something.

Szymon20000 · 2023-09-07T14:03:38Z

@tgolen is may be the case with network calls

tgolen · 2023-09-07T15:42:42Z

Ah, you're right. I'm on a branch where we've refactored that specific code to be a little different and it's not clear anymore that the Onyx.update() is inside the connect().

https://github.com/Expensify/App/blob/5373041c5776fd8a51ea3646710ed9c7ccc50658/src/libs/Network/SequentialQueue.js#L126

Szymon20000 · 2023-09-07T21:42:24Z

I was able to fix most of failing tests in the app repo. There are 6 that left.
I think we need the pr for only batching withOnyx subscribers as networking flushing would be delayed otherwise.

Only batch withOnyx updates

marcaaron · 2023-09-12T20:06:35Z

We have two approving reviews. What's next? Test the changes?

Szymon20000 · 2023-09-14T13:01:10Z

The next step is to adjust tests in the App repo. Expensify/App#27230

mountiny · 2023-09-15T07:42:51Z

To get this going ahead I will merge this now given there are two approvals here

marcaaron · 2023-09-15T17:30:12Z

Should we make an announcement not to update the Onyx version until that testing is done? These feel like pretty significant changes.

Julesssss · 2023-09-18T02:35:47Z

Should we make an announcement not to update the Onyx version until that testing is done? These feel like pretty significant changes.

Yes, great point.

Szymon20000 and others added 3 commits August 23, 2023 18:24

batch updates

d690f1c

fix lint

4abb23d

consider canUpdateSubscriber

3f5b5fb

hannojg force-pushed the @szymon/batch_updates_2_try branch from ccb13e7 to 3f5b5fb Compare August 24, 2023 15:09

hannojg added 11 commits August 24, 2023 17:21

sprinkle it with explanations

5a332fb

Merge branch 'main' of github.com:Expensify/react-native-onyx into @s…

7bd52b5

…zymon/batch_updates_2_try

wip: fixing tests

ee0f4dd

promisify the callback of broadcast update and return that

d135986

fix onyx tests

a6e8a8e

fix onyx cache tests

7d97ce4

fix metrics test

ceae90b

move jest.useRealTimers() to jestSetup.js

27fd1f2

return async operation, fixes two tests

079b041

fix tests

8140dda

remove run all timers

b5c0a6b

Szymon20000 marked this pull request as ready for review August 28, 2023 13:09

Szymon20000 requested a review from a team as a code owner August 28, 2023 13:09

melvin-bot bot requested review from Li357 and removed request for a team August 28, 2023 13:09

fix build

75a8049

Merge branch 'main' into @szymon/batch_updates_2_try

aee30ff

mountiny requested review from tgolen, marcaaron and roryabraham August 31, 2023 06:45

hannojg mentioned this pull request Aug 31, 2023

[Performance] Onyx.get batching #84

Open

Julesssss reviewed Aug 31, 2023

View reviewed changes

Julesssss previously approved these changes Aug 31, 2023

View reviewed changes

tgolen requested changes Aug 31, 2023

View reviewed changes

marcaaron requested changes Sep 6, 2023

View reviewed changes

lib/Onyx.js Show resolved Hide resolved

lib/Onyx.js Outdated Show resolved Hide resolved

lib/Onyx.js Outdated Show resolved Hide resolved

lib/Onyx.js Show resolved Hide resolved

Szymon20000 and others added 2 commits September 6, 2023 07:48

Update lib/Onyx.js

cbef4a8

Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>

Update lib/Onyx.js

e8fe453

Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>

Szymon20000 added 2 commits September 6, 2023 12:37

stash work

6e3e08a

only batch withOnyx updates

1890eee

Szymon20000 added 2 commits September 12, 2023 13:09

Merge pull request #1 from margelo/@szymon/better_batch

9b28b02

Only batch withOnyx updates

clean up

5da4cac

Szymon20000 mentioned this pull request Sep 12, 2023

Adjust tests to the batching updates feature Expensify/App#27230

Merged

59 tasks

tgolen previously approved these changes Sep 12, 2023

View reviewed changes

marcaaron previously approved these changes Sep 12, 2023

View reviewed changes

Merge branch 'main' into @szymon/batch_updates_2_try

cd48425

Szymon20000 dismissed stale reviews from marcaaron and tgolen via cd48425 September 14, 2023 12:59

tgolen approved these changes Sep 14, 2023

View reviewed changes

mountiny merged commit 18a07d1 into Expensify:main Sep 15, 2023
3 checks passed

janicduplessis mentioned this pull request Sep 20, 2023

Batch initial hydration #354

Merged

muttmuure mentioned this pull request Sep 25, 2023

Screen transitions trigger an excessive number of react commits Expensify/App#28158

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch updates second try #315

Batch updates second try #315

Szymon20000 commented Aug 23, 2023 •

edited

Loading

Li357 commented Aug 28, 2023

Szymon20000 commented Aug 28, 2023

Julesssss Aug 31, 2023

marcaaron Aug 31, 2023

Szymon20000 Sep 5, 2023

Julesssss left a comment

tgolen left a comment

tgolen Aug 31, 2023

Szymon20000 Sep 5, 2023

tgolen Aug 31, 2023

tgolen Aug 31, 2023

Szymon20000 Sep 5, 2023

tgolen Aug 31, 2023

Szymon20000 Sep 5, 2023

marcaaron left a comment

Szymon20000 commented Sep 6, 2023

Szymon20000 commented Sep 6, 2023

tgolen commented Sep 6, 2023

marcaaron commented Sep 6, 2023

Szymon20000 commented Sep 7, 2023

Szymon20000 commented Sep 7, 2023

tgolen commented Sep 7, 2023

Szymon20000 commented Sep 7, 2023

marcaaron commented Sep 12, 2023

Szymon20000 commented Sep 14, 2023

mountiny commented Sep 15, 2023

marcaaron commented Sep 15, 2023

Julesssss commented Sep 18, 2023

	it('should update withOnyx subscriber multiple times when merge is used', () => {
	const TestComponentWithOnyx = withOnyx({
	text: {
	key: ONYX_KEYS.COLLECTION.TEST_KEY,
	},
	})(ViewWithCollections);
	const onRender = jest.fn();
	render(<TestComponentWithOnyx onRender={onRender} />);

	return waitForPromisesToResolve()
	.then(() => {
	Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {ID: 123});
	Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}2`, {ID: 234});
	return Onyx.merge(`${ONYX_KEYS.COLLECTION.TEST_KEY}3`, {ID: 345});
	})
	.then(() => {
	expect(onRender).toHaveBeenCalledTimes(4);
	});
	});

Batch updates second try #315

Batch updates second try #315

Conversation

Szymon20000 commented Aug 23, 2023 • edited Loading

The Problem this pull-request solves

Examples of the problem

Details

How it works

Related Issues

Automated Tests

Linked PRs

Li357 commented Aug 28, 2023

Szymon20000 commented Aug 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Julesssss left a comment

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

Szymon20000 commented Sep 6, 2023

Szymon20000 commented Sep 6, 2023

tgolen commented Sep 6, 2023

marcaaron commented Sep 6, 2023

Szymon20000 commented Sep 7, 2023

Szymon20000 commented Sep 7, 2023

tgolen commented Sep 7, 2023

Szymon20000 commented Sep 7, 2023

marcaaron commented Sep 12, 2023

Szymon20000 commented Sep 14, 2023

mountiny commented Sep 15, 2023

marcaaron commented Sep 15, 2023

Julesssss commented Sep 18, 2023

Szymon20000 commented Aug 23, 2023 •

edited

Loading