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

Snapshot txMeta without cloning history #8363

Merged
merged 1 commit into from Apr 20, 2020

Conversation

whymarrh
Copy link
Contributor

@whymarrh whymarrh commented Apr 19, 2020

Refs #6547 (comment)

This PR updates how we snapshot txMeta objects, cloning them without the history instead of cloning the history and then throwing it away. This should improve the performance of TransactionStateManager#updateTx when the transaction has a particularly large history.

There is a test to confirm that the history property is excluded—this is an implementation detail.

@whymarrh whymarrh force-pushed the snapshot-tx-perf branch 2 times, most recently from a31d0e0 to 45e7a56 Compare April 19, 2020 22:48
@kumavis
Copy link
Member

kumavis commented Apr 20, 2020

nit: history: undefined, is not the same as not having a history property
id suggest re-adding delete snapshot.history

even better would be to have { history: [...], value: txMeta } but thats a more significant change and would require a migration

@whymarrh
Copy link
Contributor Author

@kumavis updated

@whymarrh whymarrh requested a review from kumavis April 20, 2020 11:01
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@whymarrh whymarrh merged commit a83d264 into MetaMask:develop Apr 20, 2020
@whymarrh whymarrh deleted the snapshot-tx-perf branch April 20, 2020 14:56
Gudahtt pushed a commit that referenced this pull request Apr 29, 2020
Backport #8363 to v7.7.9. Note that this uses `clone` instead of
`cloneDeep`, because `clone` hadn't yet been replaced by `cloneDeep` on
`master`.

Backporting that change as well would have been very disruptive, so
I've updated this to use `clone` instead to minimize conflicts. It is
functionally equivalent.
Gudahtt added a commit that referenced this pull request Apr 29, 2020
Backport #8363 to v7.7.9. Note that this uses `clone` instead of
`cloneDeep`, because `clone` hadn't yet been replaced by `cloneDeep` on
`master`.

Backporting that change as well would have been very disruptive, so
I've updated this to use `clone` instead to minimize conflicts. It is
functionally equivalent.

Co-authored-by: Whymarrh Whitby <whymarrh.whitby@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants