Skip to content
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

Prevent setting state for values that have not changed. Add additional Onyx debugging metrics. #185

Closed
wants to merge 1 commit into from

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Sep 27, 2022

Details

There are many calls to setState() in Onyx that can be avoided. The cost to calculate whether they have changed is lower than the cost to re-render the components that are subscribing to them. Therefore, it makes sense to use an optimized equality check to see if something has changed before we update a component.

I also added metrics that give us insights into why the setState() call is happening which can be enabled by using the ONYX_METRICS=true flag in the .env file.

Note: These metrics are currently broken on web due to some unrelated issue. And removing the applyDecorators() method will fix the build and allow our new logs to appear.

Metrics

FCP/LCP/etc on Web

without checks

Screen Shot 2022-09-27 at 10 42 26 AM

with checks

Screen Shot 2022-09-27 at 10 20 14 AM

Chat Switch Time on Web

without dequal checks:

Screen Shot 2022-09-27 at 10 09 36 AM

with dequal checks:

Screen Shot 2022-09-27 at 10 06 16 AM

There's ~ 90 ms improvement in the chat switching from reducing set state.

So far... not super impressive... but check out Android profiles on chat switching...

Chat Switch Time on Android

without checks

Screen Shot 2022-09-27 at 10 59 30 AM

with checks

Screen Shot 2022-09-27 at 11 00 54 AM

That's 664 ms difference which is pretty great.

Related Issues

Automated Tests

Linked PRs

@marcaaron marcaaron self-assigned this Sep 27, 2022
…o update

Add test

add dequal and debug setState calls

remove throw

Add setState logging methods

remove dequal for now

new PR

Only include difference not previous/new value
@Szymon20000
Copy link
Contributor

I'm not familiar with onyx but knowing other state management libraries I'm wondering if
would it be possible to make all data immutable so that it's enough to compare reference?
So whenever you want to make a change you take the object from the state management and even if you want to just change one property you create a new object.

@marcaaron
Copy link
Contributor Author

Yeah, I think we can walk through that as an exercise. Probably first we need to understand whether we are passing around object references or new copies in some places to begin with. I am not actually 100% sure about this...

All the data a component initially receives will come from here:

// If we have a withOnyxInstance that means a React component has subscribed via the withOnyx() HOC and we need to
// group collection key member data into an object.
if (mapping.withOnyxInstance) {
if (isCollectionKey(mapping.key)) {
getCollectionDataAndSendAsObject(matchingKeys, mapping);
return;
}
// If the subscriber is not using a collection key then we just send a single value back to the subscriber
get(mapping.key).then(val => sendDataToConnection(mapping, val, mapping.key));
return;
}

This next part is confusing because we have two kinds of "subscribers" the collection or just normal key.

If we have a normal key the value in the cache it would be returned to the component otherwise we get it from storage, put it in the cache, and then return it:

// When we already have the value in cache - resolve right away
if (cache.hasCacheForKey(key)) {
return Promise.resolve(cache.getValue(key));
}
const taskName = `get:${key}`;
// When a value retrieving task for this key is still running hook to it
if (cache.hasPendingTask(taskName)) {
return cache.getTaskPromise(taskName);
}
// Otherwise retrieve the value from storage and capture a promise to aid concurrent usages
const promise = Storage.getItem(key)
.then((val) => {
cache.set(key, val);
return val;
})

When it goes in the cache we don't do anything special to it so it should be a good reference

set(key, value) {
this.addKey(key);
this.addToAccessedKeys(key);
this.storageMap[key] = value;

If the key we are subscribing to is normal (non-"collection") we just set the value directly to state:

// If the subscriber is not using a collection key then we just send a single value back to the subscriber
get(mapping.key).then(val => sendDataToConnection(mapping, val, mapping.key));

config.withOnyxInstance.setWithOnyxState(config.statePropertyName, val);

And the reference to these simple object I think should survive and be sent to the component:

setWithOnyxState(statePropertyName, val) {
if (!this.state.loading) {
this.setState({[statePropertyName]: val});
return;
}
this.tempState[statePropertyName] = val;
// All state keys should exist and at least have a value of null
if (_.some(requiredKeysForInit, key => _.isUndefined(this.tempState[key]))) {
return;
}
this.setState({...this.tempState, loading: false});

"Collection keys" work a bit different because we collect all the keys and then reduce them into a new object here:

.then(values => _.reduce(values, (finalObject, value, i) => {
// eslint-disable-next-line no-param-reassign
finalObject[matchingKeys[i]] = value;
return finalObject;
}, {}))

Then the whole big object is set to a single reports key in the withOnyx state.

That's just to give an idea of the references the components start with:
- simple key same object in cache
- collection key new object with values referencing objects in the cache

Next we should look at how they are updated...

If a single key with object value is changed via a merge() then we batch all the merge calls and combine them with a reduce call (creating a new object each time):

if (_.isObject(data) || _.every(mergeValues, _.isObject)) {
// Object values are merged one after the other
return _.reduce(mergeValues, (modifiedData, mergeValue) => {
const newData = mergeWithCustomized({}, modifiedData, mergeValue);
// We will also delete any object keys that are undefined or null.
// Deleting keys is not supported by AsyncStorage so we do it this way.
// Remove all first level keys that are explicitly set to null.
return _.omit(newData, (value, finalObjectKey) => _.isNull(mergeValue[finalObjectKey]));
}, data || {});
}

Finally this new object is saved:

return set(key, modifiedData);

And then stored in the cache + subscribers are notified with the new value:

cache.set(key, value);
notifySubscribersOnNextTick(key, value);

I think at this point it's safe to say that for the simple values this will always create a new object in the cache.

So even though we take the value from the cache and pass it to a subscriber here without modifying it when it is pulled from the cache:

subscriber.withOnyxInstance.setState({
[subscriber.statePropertyName]: data,
});

So if we were to look at prevState I think we'd find that what is in the cache is not the same object and the simple compare wouldn't work (unless we first fix that or change how it works - I think that is the suggestion you are making 😄).

And if the key we changed belongs to a collection and the subscriber wants all the data then we do this:

subscriber.withOnyxInstance.setState((prevState) => {
const collection = prevState[subscriber.statePropertyName] || {};
return {
[subscriber.statePropertyName]: {
...collection,
[key]: data,
},
};
});

Which in the case of reports would definitely be a new object since we are using a spread on the old collection.

Hopefully that's not TMI and helps give some ideas about how things work.

@marcaaron
Copy link
Contributor Author

Actually going to split these debugging metrics and anything related to equality checks into separate PRs.

@marcaaron marcaaron closed this Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants