Limit number of transactions passed outside of TransactionController #9010
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Gudahtt
reviewed
Jul 16, 2020
There was a problem hiding this comment.
This seems kinda dangerous to me. Our UI assumes that all of the transactions in a 'group' (i.e. with the same nonce) are present, and it would display things incorrectly otherwise. e.g. a cancellation might be displayed as sending ether to yourself, or a transaction might show up as 'dropped' if an earlier transaction with the same nonce completed, but wasn't included in this set. Splitting up transaction groups would be nice to avoid.
Maybe instead of counting transactions, we could count nonce's backwards? and keep the first X nonces encountered.
|
That sounds feasible, I'll take a swing at that. |
Refs MetaMask#8572 Refs MetaMask#8991 This change limits the number of transactions (`txMeta`s) that are passed outside of the `TransactionController`, resulting in shorter serialization and deserialization times when state is moved between the background and UI contexts. `TransactionController#_updateMemstore` --------------------------------------- The `currentNetworkTxList` state of the `TransactionController` is used externally (i.e. outside of the controller) as the canonical source for the full transaction history. Prior to this change, the method would iterate the full transaction history and possibly return all of it. This change limits it to `MAX_MEMSTORE_TX_LIST_SIZE` to make sure that: 1. Calls to `_updateMemstore` are fast(er) 2. Passing `currentNetworkTxList` around is fast(er) (Shown in MetaMask#8377, `_updateMemstore`, is called _frequently_ when a transaction is pending.) The list is iterated backwards because it is possible that new transactions are at the end of the list. [1] Results ------- In profiles before this change, with ~3k transactions locally, `PortDuplexStream._onMessage` took up to ~4.5s to complete when the set of transactions is included. [2] In profiles after this change, `PortDuplexStream._onMessage` took ~90ms to complete. [3] Before vs. after profile screenshots: ![Profile 1][2] ![Profile 2][3] [1]:https://github.com/MetaMask/metamask-extension/blob/5a3ae85b728096cb45c8cc6822249eed5555ee25/app/scripts/controllers/transactions/tx-state-manager.js#L172-L174 [2]:https://user-images.githubusercontent.com/1623628/87613203-36f51d80-c6e7-11ea-89bc-11a1cc2f3b1e.png [3]:https://user-images.githubusercontent.com/1623628/87613215-3bb9d180-c6e7-11ea-8d85-aff3acbd0374.png [8337]:MetaMask#8377 [8572]:MetaMask#8572 [8991]:MetaMask#8991
whymarrh
force-pushed
the
fix-perf-issues
branch
from
191fd8b to
a0b1ef0
Compare
|
I've updated the logic here to look for N unique nonces (and added tests!). This still iterates the whole tx list but should account for how the set of transactions are used by the UI. |
rekmarks
reviewed
Jul 16, 2020
rekmarks
reviewed
Jul 16, 2020
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refs #8377
Refs #8572
Refs #8991
This change limits the number of transactions (
txMetas) that are passed outside of theTransactionController, resulting in shorter serialization and deserialization times when state is moved between the background and UI contexts.TransactionController#_updateMemstoreThe
currentNetworkTxListstate of theTransactionControlleris used externally (i.e. outside of the controller) as the canonical source for the full transaction history. Prior to this change, the method would iterate the full transaction history and possibly return all of it.This change limits it to
MAX_MEMSTORE_TX_LIST_SIZEto make sure that:_updateMemstoreare fast(er)currentNetworkTxListaround is fast(er)(Shown in #8377,
_updateMemstore, is called frequently when a transaction is pending.)The list is iterated backwards because it is possible that new transactions are at the end of the list.[1]
Results
In profiles before this change, with ~3k transactions locally,
PortDuplexStream._onMessagetook up to ~4.5s to complete when the set of transactions is included.[2]In profiles after this change,
PortDuplexStream._onMessagetook ~90ms to complete.[3]Before vs. after profile screenshots