Refactor assets slice to fix performance problems

[jagodarybacka]

Resolves #3466

What

As Redux was having serious performance problems when we were using an array to store all assets let's refactor that slice so it is now storing assets as an object that is indexed by assets' symbols. Seems like this is working great with redux and we are able to reduce CPU usage as we were updating assets very often.

Testing

• checkout main, add ccounts, activate networks, add some custom networks, add custom assets etc
• checkout load-assets-perf-fix, reload extension
• open extension - it shouldn't throw errors, no errors in the background script console
• see if balances on the wallet page are displayed correctly (compare with for example Etherscan)
• see if token lists on swaps page are loaded correctly
• see if token list on send asset pag is loaded correctly
• see if portfolio page is looking correctly (compare with for example Zapper - there can be some differences with prices but token balances should be the same)
• test send - check if balance is updating correctly
• test swap - check if balance is updating correctly

Latest build: extension-builds-3530 (as of Tue, 18 Jul 2023 16:17:42 GMT).

[Shadowfiend]

Looks like we have two competing approaches here with #3526 .

I directionally prefer the approach in 3526, because the question of “what do we use as the key for an asset” is one that seems easy and then inevitably gets complicated, even though we're trivializing it right now with getAssetID. We can see this already because we've had to introduce the getFullAssetID alternative, and future changes to supported chains or asset types are going to make things even more complicated. We're also in a scenario here where displaying “assets for this network” is a more taxing flow than with a flat list, a price that we're likely going to pay in unexpected and hard-to-detect ways down the line.

With that said, I'm interested in seeing how the two perform relative to each other, and, if this performs better, developing a little bit of a deeper understanding of why to guide future decisions for how redux is structured. My intuition would expect this approach to have the same performance characteristics of the current flat list, because I would expect any change of the assets object (even for a subkey) to trigger updates in the per-network assets list and full asset list, which in turn I would expect to trigger most of the same selectors as an update of the assets array does.

Obviously you're seeing performance improvements, so I would love to understand what isn't triggering here that is in the current approach. Maybe I can do some digging on my own there :)

[jagodarybacka]

I'll dig further today but I think #3526 should work fine with this PR - my current theory is that keeping assets as a flat list is making redux work really hard while merging state. Obviously, I have to confirm it to be sure.
But overall if my theory is correct we can think what will be the best structure to store assets state, I'm far from claiming that indexing by the symbol is the best 😉

[Shadowfiend]

The reason I didn't expect that to be the case is that if you look at the package of updates that comes across the wire it's a diff, so should be relatively few changes to the list, and I believe the changes should be in place. That might not be how it's happening in practice though!

[jagodarybacka]

Ok so I've investigated it a little bit more and the problem is actually in the webext-redux package. We are using deepDiff strategy there and it is taking many repetition of the same function (mostly this one) to get proper diff for an array that is about 1k+ nested objects.

The second problem is that we are dispatching the "assets" event multiple times in a row which is unnecessary and we can probably batch these updates. Every time it is dispatched we give webext-redux another state to calculate diff for.

Seems like changing the structure of the redux slice is already making it easier to calculate diff but I'm looking for other options on how to handle it best.

The last part of solving this problem would be to make sure react is not triggered unnecessarily by these assets slice changes.

[Shadowfiend]

The second problem is that we are dispatching the "assets" event multiple times in a row which is unnecessary and we can probably batch these updates. Every time it is dispatched we give webext-redux another state to calculate diff for.

I… Have an idea here. We've looked at auto-batching actions and subscribers before and it's seemed very complicated and had some potential unintended consequences (see #1296). But what I just realized is what want here isn't to batch actions, it's to batch store updates, because that's the expensive thing here. I think we can (ab)use the patch function to debounce… #3535 has a quick draft attempt.

[jagodarybacka]

Ok, migration was added, this is good to go to QA.

Problems with debouncing state updates I think can be done in another PR as they are not that simple - let's see if this thing alone is making a visible difference.

[hyphenized]

Ran a few tests on this, noted a few things:

The following tests were ran using tiny-bench and benchmarkjs, the later was dropped due to needing manual patching to work.

• Loading state (hydrating)
  Fills an empty state with the assets array
• Updating state
  Updates state by inserting 1 new asset
• Diffing empty -> full
  Performs deepDiff over an empty state and an loaded state
• Diffing loaded -> insert 1
  Performs deepDiff over a loaded state and an updated state
• Encoding/Decoding
  Performs encodeJSON/decodeJSON

From all these tests the more important to us is the one that says diffing (loaded -> insert 1), because state "hydration" is only done once and we're frequently dispatching small updates.

• There's some benefit from this approach over the past implementation, which is seen in diffing hydration and diffing updates.
• Performance was best when using an object indexed by asset ids (20x improvement for diffing updates, 2x for state updates), for diff hydration the performance was actually worse but, since it's done only once I think it's worth the trade off.
• Major benefit came from using produce when updating nested objects and when working over an object instead of an array, though that is expected.

We're also keeping assets from all networks in state, which is not ideal.

These tests can be run with this patch:
diff.zip

[hyphenized]

Closed until reprioritization