Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Improve performance of SecureAbove22Preferences in cold startup (40ms perfTest) #7450

Closed
mcomella opened this issue Dec 31, 2019 · 9 comments
Closed

Improve performance of SecureAbove22Preferences in cold startup (40ms perfTest) #7450

mcomella opened this issue Dec 31, 2019 · 9 comments
Assignees
@grigoryk
Labels
P2 Upcoming release performance Possible performance wins
Projects
Performance, front-end roadmap

Comments

@mcomella
Copy link
Contributor

mcomella commented Dec 31, 2019 •
edited by data-sync-user

Note: some of the init of SecureAbove22 will happen in #8056

SecureAbove22Preferences takes up 9.66% of FenixApplication.onCreate start time:

  • Core.getSecureAbove22Preferences is 3.90%
  • SecureAbove22Preferences.getString() is 5.76%

We should see what we can do to improve performance – e.g. can this be deferred until after startup?

Priority

Difficulty (1-5): 2? Patch already in progress.

Impact (Pixel 2):

  • forPerfTest: 41.72ms

@MarcLeclair Please add any additional details as necessary!

I think there may be some SharedPreferences disk accesses that are causing this.

┆Issue is synchronized with this Jira Task
@mcomella mcomella added the performance Possible performance wins label Dec 31, 2019
@mcomella mcomella added this to Needs prioritization in Performance, front-end roadmap via automation Dec 31, 2019
@mcomella mcomella mentioned this issue Dec 31, 2019
Closed
MarcLeclair added a commit to MarcLeclair/fenix that referenced this issue Dec 31, 2019
@MarcLeclair
mozilla-mobile#7450 experiment to fetch SecureAbove22Preferences asyn…
6d122e8 
…chronously to test if it improves start up time
MarcLeclair added a commit to MarcLeclair/fenix that referenced this issue Dec 31, 2019
@MarcLeclair
mozilla-mobile#7450 fixes for linting for pre push hook
6355485
MarcLeclair added a commit to MarcLeclair/fenix that referenced this issue Dec 31, 2019
@MarcLeclair
mozilla-mobile#7450 fixes for linting for pre push hook
6b3c895
MarcLeclair added a commit to MarcLeclair/fenix that referenced this issue Dec 31, 2019
@MarcLeclair
mozilla-mobile#7450 fixes for linting for pre push hook
2a4f294
@MarcLeclair
Copy link
Contributor

MarcLeclair commented Dec 31, 2019 •
edited

So, for a quick summary: the implementation of the SecureAbove22Preferences is done in AC but, I think we should be able to go read it in a background thread. I did a quick implementation to try it out ( anyone is welcome to test it too to make sure I didn't do anything to break the app with other flows).

Here's my branch

Trace with original code / new implementation:
SecureAbove22Preferences_trace.zip

@boek boek added the P2 Upcoming release label Jan 3, 2020
@mcomella mcomella moved this from Needs prioritization to Backlog (prioritized) in Performance, front-end roadmap Jan 6, 2020
@colintheshots colintheshots moved this from Backlog (prioritized) to In progress in Performance, front-end roadmap Jan 10, 2020
@colintheshots colintheshots self-assigned this Jan 10, 2020
@mcomella mcomella moved this from In progress to Backlog (prioritized) in Performance, front-end roadmap Jan 16, 2020
@mcomella mcomella changed the title Improve performance of SecureAbove22Preferences in cold startup Improve performance of SecureAbove22Preferences in cold startup (40ms) Jan 21, 2020
@mcomella mcomella changed the title Improve performance of SecureAbove22Preferences in cold startup (40ms) Improve performance of SecureAbove22Preferences in cold startup (40ms perfTest) Jan 21, 2020
@mcomella
Copy link
Contributor Author

@ekager I see you're assigned to #8324: do you think that bug will remove SecureAbove22Preferences init on Release and Beta channels?

I'm not sure we want the behavior to diverge significantly but it could be a way to speed up those channels a bit.

@ekager
Copy link
Contributor

ekager commented Feb 19, 2020

@mcomella so #8324 was because we were seeing crashes with using the Keystore and want to continue to monitor those in Nightly. The solution was to force the insecure storage impl in SecureAbove22Preferences via #8446 and AC changes mozilla-mobile/android-components#5902

Release/Beta will still init SecureAbove22Preferences in a likely similar fashion. The only change I can think of that would possibly help perf is that I added a way to try to migrate secure -> insecure and insecure -> secure on every init (so we could in theory flip back and forth via this forceInsecure flag) but only if the current selected prefs are empty (so hopefully just on first startup?)

Let me know if this makes sense

@mcomella
Copy link
Contributor Author

mcomella commented Feb 24, 2020 •
edited

Triage: before working on this issue, if possible, validate the performance impact of SecureAbove22Preferences in a release variant.

@mcomella mcomella moved this from Backlog (prioritized) to Waiting in Performance, front-end roadmap Mar 2, 2020
@mcomella
Copy link
Contributor Author

mcomella commented Mar 2, 2020

Triage: @csadilek said there is a PR open to get rid of this during startup and that we should re-evaluate after that lands.

@grigoryk
Copy link
Contributor

grigoryk commented Mar 3, 2020

#8875 removes passwordStorage init (and SecureAbove22Preferences access) from the startup path.

@mcomella
Copy link
Contributor Author

mcomella commented Mar 3, 2020

@grigoryk Do you think this issue can be closed then?

@grigoryk
Copy link
Contributor

Not yet. Currently, we're triggering it via engine initialization. I'm working through it right now - fixed the engine init problem, now need to deal with background services touching this stuff.

@grigoryk
Copy link
Contributor

grigoryk commented Mar 17, 2020 •
edited

See mozilla-mobile/android-components#6304

grigoryk pushed a commit to grigoryk/fenix that referenced this issue Mar 18, 2020
Closes mozilla-mobile#7450: Lazy storage initialization
d308742 
Make sure that we actually lazily initialize our storage layers.

With this patch applied, storage layers (history, logins, bookmarks) will be initialized when first
accessed. We will no longer block GeckoEngine init, for example, on waiting for the logins storage
to initialize (which needs to access the costly securePrefStorage).
Similarly, BackgroundServices init will no longer require initialized instances of the storage
components - references to their "lazy wrappers" will suffice.

In practice, this change changes when our storage layers are initialized in the following ways.
Currently, we will initialize everything on startup. This includes loading our megazord, as well.

With this change, init path depends on if the user is signed-into FxA or not.

If user is not an FxA user:
- on startup, none of the storage layers are initialized
- history storage will be initialized once, whenever:
  - first non-customTab page is loaded (access to the HistoryDelegate)
  - first interaction with the awesomebar
  - history UI is accessed
- bookmarks storage will be initialized once, whenever:
  - something is bookmarked, or we need to figure out if something's bookmarked
  - bookmarks UI is accessed
- logins storage will be initialized once, whenever:
  - first page is loaded with a login/password fields that can be autofilled
  - (or some other interaction by GV with the autofill/loginStorage delegates)
  - logins UI is accessed
- all of these storages will be initialized if the user logs into FxA and starts syncing data
  - except, if a storage is not chosen to be synced, it will not be initialized

If user is an FxA user:
- on startup, none of the storage layers are initialized
- sometime shortly after startup is complete, when a sync worker runs in the background, all storage
layers that are enabled to sync will be initialized.

This change also means that we delay loading the megazord until first access (as described above).
grigoryk pushed a commit to grigoryk/fenix that referenced this issue Mar 18, 2020
Closes mozilla-mobile#7450: Lazy storage initialization
b576537 
Make sure that we actually lazily initialize our storage layers.

With this patch applied, storage layers (history, logins, bookmarks) will be initialized when first
accessed. We will no longer block GeckoEngine init, for example, on waiting for the logins storage
to initialize (which needs to access the costly securePrefStorage).
Similarly, BackgroundServices init will no longer require initialized instances of the storage
components - references to their "lazy wrappers" will suffice.

In practice, this change changes when our storage layers are initialized in the following ways.
Currently, we will initialize everything on startup. This includes loading our megazord, as well.

With this change, init path depends on if the user is signed-into FxA or not.

If user is not an FxA user:
- on startup, none of the storage layers are initialized
- history storage will be initialized once, whenever:
  - first non-customTab page is loaded (access to the HistoryDelegate)
  - first interaction with the awesomebar
  - history UI is accessed
- bookmarks storage will be initialized once, whenever:
  - something is bookmarked, or we need to figure out if something's bookmarked
  - bookmarks UI is accessed
- logins storage will be initialized once, whenever:
  - first page is loaded with a login/password fields that can be autofilled
  - (or some other interaction by GV with the autofill/loginStorage delegates)
  - logins UI is accessed
- all of these storages will be initialized if the user logs into FxA and starts syncing data
  - except, if a storage is not chosen to be synced, it will not be initialized

If user is an FxA user:
- on startup, none of the storage layers are initialized
- sometime shortly after startup is complete, when a sync worker runs in the background, all storage
layers that are enabled to sync will be initialized.

This change also means that we delay loading the megazord until first access (as described above).
@csadilek csadilek moved this from Waiting to In progress in Performance, front-end roadmap Mar 19, 2020
grigoryk pushed a commit to grigoryk/fenix that referenced this issue Mar 19, 2020
Closes mozilla-mobile#7450: Lazy storage initialization
36f072e 
Make sure that we actually lazily initialize our storage layers.

With this patch applied, storage layers (history, logins, bookmarks) will be initialized when first
accessed. We will no longer block GeckoEngine init, for example, on waiting for the logins storage
to initialize (which needs to access the costly securePrefStorage).
Similarly, BackgroundServices init will no longer require initialized instances of the storage
components - references to their "lazy wrappers" will suffice.

In practice, this change changes when our storage layers are initialized in the following ways.
Currently, we will initialize everything on startup. This includes loading our megazord, as well.

With this change, init path depends on if the user is signed-into FxA or not.

If user is not an FxA user:
- on startup, none of the storage layers are initialized
- history storage will be initialized once, whenever:
  - first non-customTab page is loaded (access to the HistoryDelegate)
  - first interaction with the awesomebar
  - history UI is accessed
- bookmarks storage will be initialized once, whenever:
  - something is bookmarked, or we need to figure out if something's bookmarked
  - bookmarks UI is accessed
- logins storage will be initialized once, whenever:
  - first page is loaded with a login/password fields that can be autofilled
  - (or some other interaction by GV with the autofill/loginStorage delegates)
  - logins UI is accessed
- all of these storages will be initialized if the user logs into FxA and starts syncing data
  - except, if a storage is not chosen to be synced, it will not be initialized

If user is an FxA user:
- on startup, none of the storage layers are initialized
- sometime shortly after startup is complete, when a sync worker runs in the background, all storage
layers that are enabled to sync will be initialized.

This change also means that we delay loading the megazord until first access (as described above).
Performance, front-end roadmap automation moved this from In progress to Done Mar 19, 2020
@liuche liuche mentioned this issue Mar 24, 2020
Closed
32 tasks
@mcomella mcomella moved this from Done (03-20) to Done in Performance, front-end roadmap Apr 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@grigoryk grigoryk

Labels
P2 Upcoming release performance Possible performance wins
Projects
Performance, front-end roadmap
Done
Milestone
No milestone
Development

No branches or pull requests
6 participants
@grigoryk @colintheshots @mcomella @boek @MarcLeclair @ekager