Defer initialization and wait for defaultKeyStates #91
Conversation
`isCollectionKey(mapping.key)` and `isSafeEvictionKey(mapping.key)` are not working correctly in this case since neither `onyxKeys` or `safeEvictionKeys` are set This also helps other logic running during init, as connections would wait a bit and allow the rest of the app to initialize
The variable `defaultKeyStates` is already preserved in memory for the life of the application, there's no need to keep it in hard storage as well Instead of reading the keys, then merging the defaults and then write them back, the logic becomes - read the keys (in one go) and merge with the defaults in cache, skipping the write This way we've prefilled the cache with some storage data that is immediately needed during init
lib/Onyx.js
Outdated
| addAllSafeEvictionKeysToRecentlyAccessedList(), | ||
| initializeWithDefaultKeyStates() | ||
| ]) | ||
| .then(deferredInit.resolve); |
There was a problem hiding this comment.
It took me some time to wrap my head around this but this is a pretty cool trick we are doing with the deferred task stuff. I'm curious, how much better this would be compared to making Onyx.init() itself a promise and calling it before allowing any connections to happen? Does it take a significant amount of time to run this code?
There was a problem hiding this comment.
Does it take a significant amount of time to run this code?
Before the change the initial get calls would start at 2ms past app launch and resolve at ~250ms
After the change they would start at 212ms and resolve at 255ms
So even though we delay things by 200ms they resolve at the same time, the speed up comes from the rest of the calls happening and in the end the last storage call is resolved 0.3-0.7 sec earlier
I'm curious, how much better this would be compared to making
Onyx.init()itself a promise and calling it before allowing any connections to happen?
Calling Onyx.init before any other Onyx calls would require many changes - every usage that is not a component based withOnyx can happen before Onyx.init is called, depending on script load order
Even if we decide to do so it would not bring anything more to the table in terms of performance
There was a problem hiding this comment.
the speed up comes from the rest of the calls happening
Hmm which calls? Sorry not sure I understand. Could you maybe say it another way?
in the end the last storage call is resolved 0.3-0.7 sec earlier
It looks like iOS experienced almost no changes (and in some cases got worse)? Does that sound right? If so, can you account for the inconsistency there?
Calling Onyx.init before any other Onyx calls would require many changes - every usage that is not a component based withOnyx can happen before Onyx.init is called, depending on script load order
👍
lib/createDeferredTask.js
Outdated
| const deferred = {}; | ||
| deferred.promise = new Promise((res, rej) => { | ||
| deferred.resolve = res; | ||
| deferred.reject = rej; |
There was a problem hiding this comment.
Let's implement this or remove it?
There was a problem hiding this comment.
What do you mean the reject functionality?
It should be up to the caller - if they need to reject they should have the option exposed here
All the deferred tasks provide the reject/resolve/promise interface - this is the standard pattern
There was a problem hiding this comment.
It should be up to the caller - if they need to reject they should have the option exposed here
I get what you are saying but I only see one caller. As a general rule, we either implement code or we remove it.
|
Updated, ready for another review |
|
Left a couple more comments. Changes look good and looks like there will be some improvement for Android. Not a blocker, but I am concerned about the results on iOS and curious why there is inconsistency there. @timszot feel free to give this a look 🙇 |
If the reason is "Last call ended at" metrics, I find those to depend on network calls and render as well and that they're not a good indication of performance gains/regression - the time there can vary by up to a second for two consecutive runs Maybe we can identify a point in loading that does not depend on factors as network and use that as a reference I'll update with the requested changes on Monday |
kidroca
force-pushed
the
kidroca/perf-default-key-states
branch
from
7960695 to
9549e40
Compare
|
Ready: Addressed all requested changes |
Ah so the data is not reliable since network calls can interfere with their accuracy? Is the same true for Android?
Perhaps we can test Onyx in isolation somehow, by creating a sample app or "test" page in our main app for running diagnostics without making API calls? Anyways, I don't doubt that this PR will be helpful and I'm not against merging it so we can move on to the other changes, but feels like the benchmarks could be improved. |
Yes, now when I've inspected the data again, the "Last call ended at" is from
I've made something similar working on the ticket here: Expensify/App#2667 but it only tests AsyncStorage and it turned out it works very fast in isolation My next PR is on Onyx.set and preventing duplicate writes. As a summary I think it would be best to capture more metrics in Expensify, but only such that give consistent result like the total time spent in |
So, it sounds like at least some of Onyx's "waiting time" is time it spends waiting for other JS calls to finish?
Could work, but seems like we also would still have that same problem. |
I think I'm starting to see this as well. The app tries to do too many things simultaneously and everything ends up waiting for everything else. |
|
@marcaaron looked this over and agree with what's being said in the comments. Not sure what the next steps currently are here though. |
|
I think we should leave this PR open until there is a way to prove it has value in one form or another.
That makes sense but there's no open issue for that problem and I don't see the relationship to performance. @kidroca Can we get started with the next PR please? And focus on something that helps performance? |
marcaaron
changed the title
What do you mean? Isn't 30sec less time spent in Onyx enough of an evidence?
It's a fix by coincidence
How would we prove the value of the remaining PRs when they use the same metric (time spent) - if it's unreliable for this PR surely it would be unreliable for the rest? |
I went off of how long it takes the last Onyx call to finish and seems like there is not much difference (and in some cases taking longer e.g. iOS). So, I presumed this PR would help reduce boot time (when Onyx init is called) or help in some other obvious way. But it looks like Onyx finishes working at or around the same time as before. Can you help explain what are we achieving?
Right, I think the change makes sense for those reasons (meaning we can merge it with an appropriate linked issue). It might be a perfectly reasonable thing to do, but doesn't really match up with the problem we set out to solve and our top focus which is performance.
To clarify, are you saying that you agree that the metrics are flawed and that we can't prove that any future changes we're making here are valuable? Or did you want to work together to create some better metrics? |
|
I like the idea of being guided by @kidroca's overall metric of time spent waiting on Onyx, even though it does not directly correlate to user time. Sorta like how when you use Granted, "wall clock" (ie, user wait time) is by far the more important, but if we can measurably reduce system time without introducing complexity, then I think we should take those wins as we get them with the belief that efficiency floats all boats in the end. |
I understand this theory, but it seems more practical to prioritize "user time" over "system time". |
I agree we should prioritize user time over system time, but I'm suggesting we want both. However, that assumes there's no real downside to merging this. Is there a downside you are concerned about? I 100% agree we shouldn't ship anything that has no defined/measurable benefit. But this is a measurable benefit, to system time, just not to user time. |
If the bar for merging changes is "has no downsides" then we'd be making a lot of useless changes that have no downsides. But the answer is no, I think the changes are great and valuable for other reasons besides performance that may not be priorities. But I'm confused about the metrics and what they are showing. I'm mainly trying to:
|
|
The bar is "has a measurable benefit AND has no downsides". I think we are
just debating on whether or not "reducing system time" is a benefit if it
doesn't also reduce user time. I think we obviously would prefer reducing
user time, but if we have a change that "only" reduces system time and
doesn't have any downsides, I don't see why we wouldn't take that small win.
…On Wed, Jul 28, 2021 at 11:43 AM Marc Glasser ***@***.***> wrote:
Is there a downside you are concerned about?
If the bar for merging changes is "has no downsides" then we'd be making a
lot of useless changes that have no downsides. But the answer is no, I
think the changes are great and valuable for other reasons besides
performance that may not be priorities. But I'm confused about the metrics
and what they are showing.
I'm mainly trying to:
- understand how this will make things go faster
- get us to prioritize things that will make things go faster
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#91 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEMNURDAX45RYRXXF2TGB3T2BFV7ANCNFSM5A2475MA>
.
|
Only the "Last call ended at" is not very reliable, it helps but only if you know what you're doing. The "time spent" metric is reliable, both for total and individual method calls. So I see an improvement here even though it doesn't seem to improve boot time
I'm trying different things to capture better metrics. When I find something that works I'll bring it up. |
I agree that reducing "system time" is valuable on its own and I think you agree that it it's more valuable to reduce "user time".
The problem I'm experiencing is that I'm not sure how to prioritize things that only reduce system time and don't add complexity. If this is not the highest value performance improvement we could make right now or its value is not well understood my instinct is to suggest we find something better to work on first. We can take any small wins that have no downsides, but it feels like we are digging in our couch for spare change or something. |
|
The task I'm working on is to optimize AsyncStorage usage. This won't necessarily fix boot time, but helps with everything in general. The total time spent was decreased by 1/4. I've made many experiments and managed to speed up reading - with batching, but even then boot time didn't drop significantly, but things like chat switching are becoming faster. The thing is measuring chat switch is hard to do manually. But since app init is loading a chat from storage, benchmarking it provides data that captures improvements in that direction as well |
Now you are speaking my language! Where is that PR? Let's do it!! 😂
I agree it's hard to do (and also time consuming if you are doing release builds - which I think is the most reliable way). I've spent the entire week doing benchmarks with the production API and release builds on iOS and Android. So, let me know if I can help test anything. |
marcaaron
changed the title
|
Have you tried the hash 93c30be with all the changes from this PR: #88 Unfortunately since then "total time" have increased, a lot due to I think all the changes complement each other, but for chat switching the set and cache improvement should be the most valuable What could speed boot time is persisting the recent access list and loading it with a single get during init (@quinthar had the idea some time ago). I can try it (the changes needed are in one of the following PRs) and report what I've found. But then we'll definitely have to defer init so that we capture the full benefit of pre-filling cache, so we might as well merge the defer logic now |
|
Merged this since I don't have any reason to block and agree with the deferred Onyx connections stuff regardless of performance.
Sorry if I'm coming off as skeptical here. I just prefer to have a clear idea which single action we are taking has the greatest impact on performance.
That's true. I'm also curious to do an experiment where we do a deferred init then a big multiGet with ALL of the available data and never clear the cache at all. I'd imagine that would reduce virtually all movement across the bridge. Not sure if that would be a good idea or not but I'd expect massive improvements in chat switching since we will effectively spend 0 time crossing the bridge. If Onyx's biggest bottleneck is |
|
Yes we can get all keys and then multi get the values. That's pretty easy to test and should review how much the app can improve with just changes in Onyx That's pretty much how Redux works with local storage During init the Redux store is created and hydrated with everything from storage Now with Expensify there are concerns that data might become too much, so a hybrid approach can be made where we hydrate everything but chat messages...
I think if we read evening at start and then only write to storage the effect would be the same |
|
Yes, we are aiming to build a platform that has shit tons of data stored
locally -- let's not limit ourselves to what fits in RAM.
…On Wed, Jul 28, 2021 at 2:41 PM Peter Velkov ***@***.***> wrote:
Yes we can get all keys and then multi get the values. That's pretty easy
to test and should review how much the app can improve with just changes in
Onyx
That's pretty much how Redux works with local storage
During init the Redux store is created and hydrated with everything from
storage
Then when a change happens it's applied to data in memory and scheduled to
be persisted in storage
Now with Expensify there are concerns that data might become too much, so
a hybrid approach can be made where we hydrate everything but chat
messages...
If Onyx's biggest bottleneck is AsyncStorage - should we try to make a
version of Onyx that doesn't ever interact with AsyncStorage and see how
the app runs?
I think if we read evening at start and then only write to storage the
effect would be the same
The bottleneck IMO is caused by withOnyx and this "get all during init"
might prove it if even then boot does not improve
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#91 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEMNUTGINFG6AJFCMNDI4DT2B2SFANCNFSM5A2475MA>
.
|
Ah sorry, to clarify, the suggestion was to experimentally do this. Then we can see how the app runs without a dependency on |
| */ | ||
| function initializeWithDefaultKeyStates() { | ||
| _.each(defaultKeyStates, (state, key) => merge(key, state)); | ||
| return AsyncStorage.multiGet(_.keys(defaultKeyStates)) |
There was a problem hiding this comment.
Just caught this. But do the "after" metrics in the description include this time at all?
There was a problem hiding this comment.
Yes, because we decorate and track this method here:
https://github.com/kidroca/react-native-onyx/blob/9549e4057abaf2a8f96bad1f255acd1c0b002cd2/lib/Onyx.js#L809
It's tracked under Onyx:defaults and adds time against "total time"
@marcaaron
Details
Make connections wait for Onyx.init
Some connections start really early before Onyx.init is even called
They should wait as Onyx.init adds some initial values that might be needed
Also waiting helps as every call uses
getAllKeysand the keys are retrieved during init - if connections wait, every following connection would instantly retrieve them from cache, otherwise even though they hook to the same storage read they still create a new promiseRead defaults in one go and keep them in cache
Instead of reading default keys, then merging default values to them and then writing them back in storage
We read the default keys in one go, and then write the merged result to cache
We also make sure to never remove the default keys from cache - they don't take much space and are used often
Related Issues
Expensify/App#2667
Automated Tests
Covered by existing tests
Linked PRs
Benchmarks
Android Before
Android After
iOS Before
iOS After
Benchmark info:
Add
global.Onyx = Onyxin Eexpensify.jsOpen the dev menu and make sure dev=false
Disable all breakpoints in the debugger
Wait for any syncing (top right corner) to end before starting any tests
Test
Onyx.printMetrics({ format: 'csv', raw: true })Run 7 times
Remove the fastest and the slowest runs
All benchmarks were performed against this hash in Expensify/App: Expensify/App@fc965f6