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

Pending Transaction Performance Problems #8377

Closed
whymarrh opened this issue Apr 22, 2020 · 4 comments
Closed

Pending Transaction Performance Problems #8377

whymarrh opened this issue Apr 22, 2020 · 4 comments
Assignees
Labels
area-transactions degraded performance Issues relating to slowness of app, cpu usage, and/or blank screens. type-bug

Comments

@whymarrh
Copy link
Contributor

whymarrh commented Apr 22, 2020

This issue details a possible performance issue that exists with pending transactions are handled.

Setup:

  1. Load MetaMask
  2. Onboarding, create wallet, get Ropsten Ether, etc.
  3. Enable "Customize transaction nonce"
    • Under Advanced Settings
  4. Copy account address
  5. Send transaction to the same account with nonce of 1 (pictured below)
    • This is supposed to be incorrect
  6. Confirm that transaction
  7. The transaction should be pending
Screenshot

On master @ a7bcc17f9/v7.7.8

Result:

Screenshots

Once this history has grown significantly large, snapshotting the txMeta becomes prohibitvely expensive. (This is v7.7.8, before we added #8363.)

The rapid case—the worse one—is primarily the following loop:

  • Update a transaction via updateTx(…)
  • The transaction update is committed to memory via _saveTxList (tx-state-manager.js:466)
  • TransactionController#_updateMemstore is run via this.txStateManager.store.subscribe(…) (index.js:111)
  • Pending txs are updated via updatePendingTxs (pending-tx-tracker.js:43)
  • A pending tx update fails, emitting a warning via this.emit('tx:warning', txMeta, err) (pending-tx-tracker.js:186)
  • updateTx(…) is called via this.pendingTxTracker.on('tx:warning', …) (index.js:613)
  • Rinse, repeat
Full stack trace

v778-network-fail-stacktrace-pathological.txt

updateTx (tx-state-manager.js:195) // Line 195 here is simply where I had the breakpoint
(anonymous) (index.js:613) // this.pendingTxTracker.on('tx:warning', ...)
safeApply (index.js:70)
SafeEventEmitter.emit (index.js:56)
_checkPendingTx (pending-tx-tracker.js:186) // this.emit('tx:warning', txMeta, err)

--- await in _checkPendingTx (async) ---

(anonymous) (pending-tx-tracker.js:43) // await Promise.all(pendingTxs.map((txMeta) => this._checkPendingTx(txMeta)))
updatePendingTxs (pending-tx-tracker.js:43)

--- await in updatePendingTxs (async) ---

_updateMemstore (index.js:727)
(anonymous) (index.js:111) // this.txStateManager.store.subscribe(() => this._updateMemstore())
safeApply (index.js:70)
SafeEventEmitter.emit (index.js:62)
putState (index.js:22)
updateState (index.js:30)
_saveTxList (tx-state-manager.js:466)
updateTx (tx-state-manager.js:205)
(anonymous) (index.js:613) // this.pendingTxTracker.on('tx:warning', ...)
safeApply (index.js:70)
SafeEventEmitter.emit (index.js:56)
_checkPendingTx (pending-tx-tracker.js:186)

--- await in _checkPendingTx (async) ---

(anonymous) (pending-tx-tracker.js:43) // await Promise.all(pendingTxs.map((txMeta) => this._checkPendingTx(txMeta)))
updatePendingTxs (pending-tx-tracker.js:43)

--- await in updatePendingTxs (async) ---

_onLatestBlock (index.js:712)
safeApply (index.js:70)
SafeEventEmitter.emit (index.js:62)
_setCurrentBlock (base.js:131)
_newPotentialLatest (base.js:125)
_updateLatestBlock (polling.js:65)

--- await in _updateLatestBlock (async) ---

_performSync (polling.js:48)

I believe the behaviour described here is as old as 507397f (#5431) which first shipped in v4.14.0.

On develop @ d90810263

Result:

  • The transaction is stuck in pending (as expected)
  • The txMeta.history for the transaction will grow unbounded at one rate:

This is all worse on develop where we essentially hit the pathological case with each pending transaction. As of #8351, _checkPendingTx will always emit a warning for a pending transaction. Admittedly this is also related to #7483, where the fallback for a null transaction receipt was originally mismanaged.


Notes:

@whymarrh whymarrh added type-bug area-transactions degraded performance Issues relating to slowness of app, cpu usage, and/or blank screens. labels Apr 22, 2020
@whymarrh whymarrh self-assigned this Apr 22, 2020
@whymarrh
Copy link
Contributor Author

cc @danfinlay @rekmarks @Gudahtt

@rekmarks
Copy link
Member

This is wonderful work!

The rapid case—the worse one—is primarily the following loop:

Just to confirm: your use of "primarily" here is to indicate that the most relevant steps of the loop are those you describe, not that you have reason to believe that there's some other, unknown cause?

@whymarrh
Copy link
Contributor Author

Yes, I cut a few frames out of the stack trace in describing it

whymarrh added a commit to whymarrh/metamask-extension that referenced this issue Apr 28, 2020
whymarrh added a commit that referenced this issue Apr 29, 2020
* Don't updatePendingTxs outside of block updates

Refs #8377

Reverts 507397f (#5431)

* Check for new block data on unlock
Gudahtt pushed a commit that referenced this issue Apr 30, 2020
Backport #8445 to v7.7.9. Original commit description:

* Don't updatePendingTxs outside of block updates

Refs #8377

Reverts 507397f (#5431)

* Check for new block data on unlock
Gudahtt added a commit that referenced this issue Apr 30, 2020
…8474)

Backport #8445 to v7.7.9. Original commit description:

* Don't updatePendingTxs outside of block updates

Refs #8377

Reverts 507397f (#5431)

* Check for new block data on unlock

Co-authored-by: Whymarrh Whitby <whymarrh.whitby@gmail.com>
@whymarrh
Copy link
Contributor Author

whymarrh commented May 4, 2020

These fixes are slated for 7.7.9: #8444

whymarrh added a commit to whymarrh/metamask-extension that referenced this issue Jul 16, 2020
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 added a commit that referenced this issue Jul 16, 2020
…9010)

Refs #8572
Refs #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 #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]:#8377
  [8572]:#8572
  [8991]:#8991
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-transactions degraded performance Issues relating to slowness of app, cpu usage, and/or blank screens. type-bug
Projects
None yet
Development

No branches or pull requests

3 participants