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

Remove fretboard to improve startup performance #5182

Closed
hawkinsw opened this issue Sep 10, 2019 · 9 comments
Closed

Remove fretboard to improve startup performance #5182

hawkinsw opened this issue Sep 10, 2019 · 9 comments
Assignees
Labels
🐞 bug Crashes, Something isn't working, .. eng:qa:verified QA Verified P1 Current sprint performance Possible performance wins
Milestone

Comments

@hawkinsw
Copy link
Contributor

hawkinsw commented Sep 10, 2019

ExperimentsInternalAPI.initialize and FenixApplication.loadExperiments are called from within the FenixApplication.SetupApplication function. SetupApplication takes 290ms and the two experiment-related functions take 45ms (approximately 15%).

There are also StrictMode violations that come from loading experiments on the main thread.

It would appear that we have regressed somehow from #1616 where we moved experiment loading off the main thread. I'm not sure if this was intentional, but we need to look at it.

┆Issue is synchronized with this Jira Task

@hawkinsw hawkinsw added the 🐞 bug Crashes, Something isn't working, .. label Sep 10, 2019
@hawkinsw
Copy link
Contributor Author

@csadilek or @colintheshots Can you add the eng:performance label to this? Thank you!

@csadilek csadilek added the performance Possible performance wins label Sep 24, 2019
@mcomella mcomella added this to Needs triage in Performance, front-end roadmap Oct 7, 2019
@mcomella mcomella moved this from Needs prioritization to Backlog (prioritized) in Performance, front-end roadmap Nov 11, 2019
@colintheshots colintheshots self-assigned this Dec 23, 2019
@colintheshots colintheshots moved this from Backlog (prioritized) to In progress in Performance, front-end roadmap Dec 27, 2019
@colintheshots
Copy link
Contributor

colintheshots commented Dec 27, 2019

I was looking at fixing this by removing Fretboard in favor of our new services-experiements framework to do less work. However, I believe the new services-experiments A-C code is not ready. As written, the new library code appears to statically load only a single experiment at one time within the call to Experiments.initialize(). If we cannot support multiple experiments with services-experiments, this performance fix is not possible. In fact, the new library seems fundamentally flawed in its implementation by only allowing a single, active experiment.

Instead, I could maybe look at using Fretboard exclusively until the new library is fixed. As for migrating work to a background thread, I need to be sure experiments have reasonable defaults if experiments have not yet been loaded.

@boek boek added the P1 Current sprint label Dec 28, 2019
@colintheshots
Copy link
Contributor

At a minimum, we should probably remove Fretboard. FxA is the last consumer and it's no longer being used to host any experiments.

colintheshots added a commit to colintheshots/fenix that referenced this issue Jan 6, 2020
…ve Fretboard

This removes Fretboard. The goal is to reduce cold startup costs associated with loading the experiments on the main thread. We currently have two experiments frameworks in use and should only require one.
colintheshots added a commit to colintheshots/fenix that referenced this issue Jan 6, 2020
…ve Fretboard

This removes Fretboard. The goal is to reduce cold startup costs associated with loading the experiments on the main thread. We currently have two experiments frameworks in use and should only require one.
colintheshots added a commit to colintheshots/fenix that referenced this issue Jan 6, 2020
…ve Fretboard

This removes Fretboard. The goal is to reduce cold startup costs associated with loading the experiments on the main thread. We currently have two experiments frameworks in use and should only require one.
colintheshots added a commit to colintheshots/fenix that referenced this issue Jan 10, 2020
…ve Fretboard

This removes Fretboard. The goal is to reduce cold startup costs associated with loading the experiments on the main thread. We currently have two experiments frameworks in use and should only require one.
colintheshots added a commit to colintheshots/fenix that referenced this issue Jan 10, 2020
…ve Fretboard

This removes Fretboard. The goal is to reduce cold startup costs associated with loading the experiments on the main thread. We currently have two experiments frameworks in use and should only require one.
colintheshots added a commit to colintheshots/fenix that referenced this issue Jan 13, 2020
…ve Fretboard

This removes Fretboard. The goal is to reduce cold startup costs associated with loading the experiments on the main thread. We currently have two experiments frameworks in use and should only require one.
colintheshots added a commit that referenced this issue Jan 13, 2020
…7510)

This removes Fretboard. The goal is to reduce cold startup costs associated with loading the experiments on the main thread. We currently have two experiments frameworks in use and should only require one.
@mcomella
Copy link
Contributor

@colintheshots Are there any next steps here or can this be closed?

@colintheshots
Copy link
Contributor

Fretboard was removed. We should file a new issue if Experiments.initialize needs optimizations as well.

@mcomella mcomella changed the title Loading experiments on startup is slow Remove fretboard to improve startup performance Jan 17, 2020
@mcomella
Copy link
Contributor

Addressed in #7510.

@mcomella
Copy link
Contributor

@baron-severin Would you mind writing a QA note for this issue? I assume we should at least test that the FxA features are enabled/disabled properly. tbh, I'm a little confused about the phrasing of the landed code – Disabled = false – so I wonder if there's a bug lurking there.

@mcomella mcomella moved this from In progress to Done in Performance, front-end roadmap Jan 17, 2020
@severinrudie
Copy link
Contributor

severinrudie commented Jan 17, 2020

@baron-severin Would you mind writing a QA note for this issue?

Note to whoever from QA picks this up, would you mind verifying that there have been no regressions in App Services web channels, login sync, and pairing? No need to verify performance, the perf team is handling that.

tbh, I'm a little confused about the phrasing of the landed code – Disabled = false – so I wonder if there's a bug lurking there.

Agreed that the names are a bit misleading, but I'm less concerned about bugs there as the properties were moved from an existing file (Experiments.kt), and they haven't otherwise changed.

@severinrudie severinrudie added the eng:qa:needed QA Needed label Jan 17, 2020
@sv-ohorvath
Copy link
Contributor

Done some testing around signup, login, sync and pairing and haven't noticed anything suspicious. I'll mark this as verified.

@sv-ohorvath sv-ohorvath added eng:qa:verified QA Verified and removed eng:qa:needed QA Needed labels Jan 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐞 bug Crashes, Something isn't working, .. eng:qa:verified QA Verified P1 Current sprint performance Possible performance wins
Development

No branches or pull requests

8 participants