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.

Sign up for GitHub

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

Jump to bottom

Slow Cold Start Times #5382

Closed
sethkfman opened this issue Dec 12, 2022 · 4 comments · Fixed by MetaMask/core#1086
Closed

Slow Cold Start Times #5382

sethkfman opened this issue Dec 12, 2022 · 4 comments · Fixed by MetaMask/core#1086
Assignees
@Fatxx @Cal-L
Labels
team-mobile-platform

Comments

@sethkfman
Copy link
Contributor

sethkfman commented Dec 12, 2022 •
edited by Cal-L
Loading

Description

The users have been reporting slow start up times in the iOS and Android mobile app. Between the initial bouncing diamond to the fox head animation. 10+ seconds.

Technical Requirements

  • Investigate and identify the root cause for the slow down
  • Propose solutions and write a ticket for resolution
  • Findings by @owencraston @Cal-L
    • app/components/Nav/App/index.js calls Authentication.appTriggeredAuth , a method that lives inAuthentication.ts
    • Logging shows that appTriggeredAuth is where the business logic takes a long time to process
      • SecureKeychain.getGenericPassword is the first culprit
      • loginVaultCreationis another when it calls recreateVaultWithSamePassword
@sethkfman
Copy link
Contributor Author

sethkfman commented Dec 13, 2022 •
edited
Loading

Notes from data science team:

Screenshot 2022-12-13 at 9.36.16 AM.png

@tommasini tommasini mentioned this issue Dec 15, 2022
Closed
@owencraston
Copy link
Contributor

owencraston commented Dec 15, 2022 •
edited
Loading

My hunch is that the major slow down occurs (for the login flow) from the use of this recreateVaultWithNewPassword function which we are calling from Login. There are a few other cases through out the app where we perform very similar logic. In my testing of Auth refactor I noticed this method being slow. More investigation is needed though.

@sethkfman
Copy link
Contributor Author

When a user puts a device in battery saver mode it the application running slow.

@Fatxx Fatxx mentioned this issue Jan 2, 2023
Merged
3 tasks
@sethkfman sethkfman added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 11, 2023
@sethkfman sethkfman removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 30, 2023
@sethkfman
Copy link
Contributor Author

@Cal-L Attach PR to ticket. Patch the controllers and open a PR for the patch.

@Cal-L Cal-L added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jan 30, 2023
This was referenced Jan 30, 2023
Merged
Merged
Cal-L added a commit to MetaMask/core that referenced this issue Feb 1, 2023
@Cal-L
Use Set to improve lookup performance (#1086)
253230f 
<!--
Thanks for your contribution!

Please ensure that any applicable requirements below are satisfied
before submitting this pull request. This will help ensure a quick and
efficient review cycle.
-->

**Improve lookup time in PhishingController**

This PR creates a Set from `metamaskConfig.blocklist` in the
`PhishingController`. The blocklist Set is later used as a lookup inside
of a filter method. As a result, look up performance is significantly
improved by using a `Set` + `has` method instead of `includes` method
from an Array.

**Description**

_Itemize the changes you have made into the categories below_

- CHANGED:

  - `PhishingController.updatePhishingLists`
- Create a `Set` from `metamaskConfig.blocklist` to be used as lookup.

**Checklist**

- [x] Tests are included if applicable
- [x] Any added code is fully documented

**Issue**

Resolves MetaMask/metamask-mobile#5382
MajorLift pushed a commit to MetaMask/core that referenced this issue Oct 11, 2023
@Cal-L @MajorLift
Use Set to improve lookup performance (#1086)
026f0ca 
<!--
Thanks for your contribution!

Please ensure that any applicable requirements below are satisfied
before submitting this pull request. This will help ensure a quick and
efficient review cycle.
-->

**Improve lookup time in PhishingController**

This PR creates a Set from `metamaskConfig.blocklist` in the
`PhishingController`. The blocklist Set is later used as a lookup inside
of a filter method. As a result, look up performance is significantly
improved by using a `Set` + `has` method instead of `includes` method
from an Array.

**Description**

_Itemize the changes you have made into the categories below_

- CHANGED:

  - `PhishingController.updatePhishingLists`
- Create a `Set` from `metamaskConfig.blocklist` to be used as lookup.

**Checklist**

- [x] Tests are included if applicable
- [x] Any added code is fully documented

**Issue**

Resolves MetaMask/metamask-mobile#5382
MajorLift pushed a commit to MetaMask/core that referenced this issue Oct 11, 2023
@Cal-L @MajorLift
Use Set to improve lookup performance (#1086)
2c2cbfd 
<!--
Thanks for your contribution!

Please ensure that any applicable requirements below are satisfied
before submitting this pull request. This will help ensure a quick and
efficient review cycle.
-->

**Improve lookup time in PhishingController**

This PR creates a Set from `metamaskConfig.blocklist` in the
`PhishingController`. The blocklist Set is later used as a lookup inside
of a filter method. As a result, look up performance is significantly
improved by using a `Set` + `has` method instead of `includes` method
from an Array.

**Description**

_Itemize the changes you have made into the categories below_

- CHANGED:

  - `PhishingController.updatePhishingLists`
- Create a `Set` from `metamaskConfig.blocklist` to be used as lookup.

**Checklist**

- [x] Tests are included if applicable
- [x] Any added code is fully documented

**Issue**

Resolves MetaMask/metamask-mobile#5382
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Fatxx Fatxx

@Cal-L Cal-L

Labels
team-mobile-platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use Set to improve lookup performance MetaMask/core
5 participants
@Fatxx @gauthierpetetin @sethkfman @Cal-L @owencraston