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

MetaMask performance got worse in v8 #8991

Closed
wjmelements opened this issue Jul 15, 2020 · 19 comments
Closed

MetaMask performance got worse in v8 #8991

wjmelements opened this issue Jul 15, 2020 · 19 comments
Labels
area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. blocked needs-reproduction Sev2-normal Normal severity; minor loss of service or inconvenience. stale issues and PRs marked as stale type-bug

Comments

@wjmelements
Copy link
Contributor

Reopening #8572 for version 8.0.4

Whether connecting, opening the popup, or sending a transaction, I am met by a blank page that must be watched the whole time or all progress will be lost.

From clicking the fox icon, it takes 6 seconds before the blank white window appears and 19 seconds before I can see an account.

@rekmarks
Copy link
Member

@wjmelements thank you for alerting us to this.

@brad-decker, are there any low-hanging fruits in the UI that could help with this, like buffering the transaction list?

@rekmarks rekmarks added the area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. label Jul 15, 2020
@wjmelements
Copy link
Contributor Author

Is there something I can do to help you profile the delay?

@bunsen2
Copy link

bunsen2 commented Jul 15, 2020

Same here. New version is running at least 10x slower than the old version.

@Gudahtt Gudahtt changed the title Metamask performance got worse in v8 MetaMask performance got worse in v8 Jul 15, 2020
@Gudahtt
Copy link
Member

Gudahtt commented Jul 15, 2020

@wjmelements To start, we'll need to reproduce this delay you're experiencing. Are you only experiencing this when using an account with >1000 transactions, as with #8572? Which browser are you using?

@wjmelements
Copy link
Contributor Author

@Gudahtt Unlike #8572 I also experience a delay when I have an unused account selected, but it could still be relevant that I have two accounts with over 2000 transactions and one with over 5000.

I am using Chrome.

@wjmelements
Copy link
Contributor Author

I profiled the background process during the delay using the chrome devtools, but it looks like minified garbage. Might still be helpful.
Screen Shot 2020-07-14 at 7 49 48 PM

@wjmelements
Copy link
Contributor Author

This looks relevant; lots of GC, with write() taking the most time
Screen Shot 2020-07-14 at 7 52 14 PM

@rstormsf

This comment has been minimized.

@danfinlay
Copy link
Contributor

@wjmelements A few questions:

  • Are these thousands of txs incoming, or are outgoing also listed?
  • To clarify, this is only on v8, even though you opened the issue before that was live? (I assume you were living off develop?)

I ask because we have long limited the length of the outgoing history, so either this is a bug and we're showing more than 40 or this is related to how v8 now shows incoming txs and does not impose that limit.

Would we solve this issue if we truncated the incoming tx list for now, until we allow a more incremental loading of tx history?

@ElderOrb
Copy link

I'm experiencing this issue too

@whymarrh
Copy link
Contributor

@ElderOrb would you be able to share a rough count of how many transactions you have across your accounts?

@whymarrh
Copy link
Contributor

@danfinlay tx performance was bad in 7.x as well (i.e. #8572) and v8 has possibly made it worse. I don't believe this is related to the tx state history, but I'll look at that.

@wjmelements thanks for re-opening this, I'm looking into it now.

@bunsen2
Copy link

bunsen2 commented Jul 15, 2020

It's running smoothly after I deleted the extension and re-installed it

@wjmelements
Copy link
Contributor Author

@wjmelements A few questions:

  • Are these thousands of txs incoming, or are outgoing also listed?

Outgoing

  • To clarify, this is only on v8, even though you opened the issue before that was live? (I assume you were living off develop?)

The prior issue was for v7.

@whymarrh
Copy link
Contributor

I can confirm that this is a severe performance problem. The "good" news is that this isn't significantly worse in v8—the slowness here is a result of the serializing/deserializing of state between MetaMask's background (persistent) and UI processes, and this hasn't changed recently.

I have a stopgap fix (#9010) that will limit the number of transactions exposed to the UI (more or less). This does indeed increase the responsiveness of the application quite drastically. 2.5k-3k transactions can take upwards of 4.5 seconds to move around internally, and with #9010 that is down to 90ms for de/serialization in some tests.

Before vs. after profile screenshots

The stopgap fix is just that. The bad news is that we don't have a good short-term fix. A proper fix for this will involve reworking how our background and UI contexts communicate, which is definitely on our radar, but isn't something we've got planned for a patch release.

It's running smoothly after I deleted the extension and re-installed it

A very immediate workaround is, yes, to restore from a seed phrase, which will reset the installation's tracked transactions. This isn't ideal, obviously, but is the only option until 8.0.5 is published.

@xmaysonnave
Copy link
Contributor

Metamask v8.0.4, debian buster, I5-7300U, 16GB, NVME

Here some informations how slow could be the UI.

I started to use Metamask encryption.

1 - Encryption performed with eth-sig-util and the public encryption key retrieved from Metamask
The sample is a 3MB sample (base64)

sigUtil.encrypt(
  currentPublicKey,
  { data: text },
  'x25519-xsalsa20-poly1305'
)

Here is my output:
Ethereum Encrypt: 152ms, In: 2524460, Out: 3366129, Ratio: 133%

2 - Decryption occurs in Metamask
Metamask is unlocked and the DApp has already the permission to access Metamask
Here is my output:
Ethereum Decrypt: 24435ms, In: 3366157, Out: 2524480, Ratio: 74%
In some situation, Metamask when locked and no permission the UI could take up to 50s.

var tStart = new Date();
var decryptedText = await provider.request({ method: "eth_decrypt", params: [text, accounts[0]] })
if (decryptedText !== undefined || decryptedText !== null) {
  var tStop = new Date()-tStart;
  var ratio = Math.floor(decryptedText.length*100/text.length);
  console.log(`Ethereum Decrypt: ${tStop}ms, In: ${text.length}, Out: ${decryptedText.length}, Ratio: ${ratio}%`);
}

3 - Decryption when tested with only eth-sig-util and the appropriate private key.
It takes roughly 300ms to decrypt the content.

Thanks

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
@Gudahtt Gudahtt added Sev2-normal Normal severity; minor loss of service or inconvenience. type-bug blocked labels Jan 13, 2021
@Gudahtt
Copy link
Member

Gudahtt commented Jan 13, 2021

The stopgap measure implemented in #9010 has hopefully worked well as a temporary solution, but I'm going to leave this open for now until we've at least documented in GitHub what our longer-term solution for this will be. In the meantime I'll mark this as "blocked", since further work is blocked on these yet-to-be-specified improvements to our background <=> UI communication.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2023

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.

@github-actions github-actions bot added the stale issues and PRs marked as stale label Sep 4, 2023
@github-actions
Copy link
Contributor

This issue was closed because there has been no follow up activity in the last 45 days. If you feel this was closed in error, please reopen and provide evidence on the latest release of the extension. Thank you for your contributions.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. blocked needs-reproduction Sev2-normal Normal severity; minor loss of service or inconvenience. stale issues and PRs marked as stale type-bug
Projects
None yet
Development

No branches or pull requests

9 participants