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

feat: enhance code performance #211

Merged

Conversation

stanleyyconsensys
Copy link
Contributor

@stanleyyconsensys stanleyyconsensys commented Nov 30, 2023

the PR is to enhance the code performance by not doing N square time complexity loop, instead using set data structure to improve to O(N) time complexity

e.g:

  • in deserialize method, when execute filtering, it compare if the account address in accountDetails by using Object.keys(this.accountDetails).includes(XXX) , which is a O(n*n) time complexity, and Object.keys will create extra memory for every filter loop
    • instead, we can use using set to reduce the complexity
  • the same case as migrateAccountDetails method
  • in removeAccount method, it is using a redundant loop to search where the address is exist or not

@stanleyyconsensys stanleyyconsensys requested a review from a team as a code owner November 30, 2023 09:04
}
});
// fix a edge case when accounts itself has duplicate address
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks to me like this part is actually a fix unrelated to the performance improvement - does it warrant breaking out and a test-case? Also: Do we know that this is always fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks to me like this part is actually a fix unrelated to the performance improvement - does it warrant breaking out and a test-case? Also: Do we know that this is always fine?

actually i find that is no necessary, or it will not cause any issue if any edge case happen
e.g
accounts = [acc1, acc1, acc2] , accountDetails = {acc2}

if we take a dry run on the Prev code

  1. filter accounts where those not in accountDetails, accounts become [acc1, acc1]
  2. acc1 not in accountDetails, acc1 add to accountDetails
  3. acc1 not in accountDetails, acc1 add to accountDetails , but the content will be same
  4. at the end result will become accountDetails = {acc2, acc1}

if we take a dry run on the Updated code

  1. acc2 in set, skip
  2. acc1 not in set, acc1 add to accountDetails , add to set
  3. acc1 in set, skip
  4. at the end result will become accountDetails = {acc2, acc1}

src/ledger-keyring.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

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

LGTM!

@stanleyyconsensys
Copy link
Contributor Author

@legobeat can you merge for me, i dont have the permission

@legobeat legobeat merged commit 7749409 into MetaMask:main Feb 27, 2024
17 checks passed
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.

2 participants