For #5182: Loading experiments on startup is slow, remove Fretboard!! #7510

colintheshots · 2020-01-06T22:42:22Z

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.

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

To download an APK when reviewing a PR:

click on Show All Checks,
click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
click on the "Fenix - assemble" task, then click "Run Artifacts".
the APK links should be on the left side of the screen, named for each CPU architecture

codecov-io · 2020-01-06T23:54:49Z

Codecov Report

Merging #7510 into master will decrease coverage by 0.68%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #7510      +/-   ##
============================================
- Coverage     19.62%   18.94%   -0.69%     
+ Complexity      469      438      -31     
============================================
  Files           300      297       -3     
  Lines         11531    11367     -164     
  Branches       1561     1532      -29     
============================================
- Hits           2263     2153     -110     
+ Misses         9093     9046      -47     
+ Partials        175      168       -7

Impacted Files	Coverage Δ	Complexity Δ
app/src/main/java/org/mozilla/fenix/Experiments.kt	`0% <ø> (-100%)`	`0 <0> (-3)`
...main/java/org/mozilla/fenix/components/Services.kt	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...rc/main/java/org/mozilla/fenix/FenixApplication.kt	`5.88% <ø> (+0.93%)`	`2 <0> (ø)`	⬇️
...ain/java/org/mozilla/fenix/components/FxaServer.kt	`0% <ø> (-100%)`	`0 <0> (-4)`
...in/java/org/mozilla/fenix/search/SearchFragment.kt	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...a/org/mozilla/fenix/browser/BaseBrowserFragment.kt	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...org/mozilla/fenix/components/BackgroundServices.kt	`20% <ø> (-37.5%)`	`0 <0> (-4)`
...in/java/org/mozilla/fenix/AppRequestInterceptor.kt	`56.41% <0%> (ø)`	`7 <0> (ø)`	⬇️
app/src/main/java/org/mozilla/fenix/ext/Context.kt	`25% <0%> (-0.93%)`	`0 <0> (ø)`
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 399df17...fb99387. Read the comment docs.

severinrudie

r+

severinrudie · 2020-01-08T23:01:13Z

app/src/main/java/org/mozilla/fenix/ext/Context.kt

@@ -64,6 +64,9 @@ fun Context.asFragmentActivity() = (this as? ContextThemeWrapper)?.baseContext a
 fun Context.getPreferenceKey(@StringRes resourceId: Int): String =
    resources.getString(resourceId)

+@Suppress("unused")
+fun Context.isInExperiment(@Suppress("UNUSED_PARAMETER") experimentName: String): Boolean = false


Nit: would you mind adding a comment along the lines of Feature flags are not supported by Experimentor, we are currently looking into another way to handle them. See {issue number}

app/src/main/java/org/mozilla/fenix/Experiments.kt

mcomella · 2020-01-10T01:12:39Z

I'm probably good with Severin's review but adding myself for a quick skim when we've confirmed we're removing it.

colintheshots · 2020-01-10T16:18:07Z

I'm probably good with Severin's review but adding myself for a quick skim when we've confirmed we're removing it.

@mcomella We now have acknowledgement from the FxA team to remove it. They asked me to backlog an A-C issue for server-side flags, which I did.

The only open question is if I should remove the isInExperiment function entirely or else add @Deprecated flags and @Suppress flags to the statements that call them.

colintheshots · 2020-01-10T16:54:08Z

@baron-severin and I agreed that it makes more sense to migrate this functionality to FeatureFlags rather than keeping a deprecated isInExperiment method.

severinrudie

LGTM. I think moving these into FeatureFlags makes sense

colintheshots · 2020-01-10T18:48:41Z

bors r+

bors · 2020-01-10T18:48:59Z

Configuration problem

bors.toml: not found

colintheshots · 2020-01-10T18:49:30Z

How can I get it to re-run the UI tests at this point?

mcomella · 2020-01-10T19:15:41Z

How can I get it to re-run the UI tests at this point?

iirc, we're not using bors so we do the old Focus thing: edit the PR title.

mcomella · 2020-01-10T19:15:48Z

Which I already did.

mcomella · 2020-01-10T19:16:12Z

profile

mcomella · 2020-01-10T19:36:22Z

We may want to land this tomorrow to be able to detect a monitor for possible regressions in other PRs in tonight's build.

mcomella

Skimmed over and lgtm with Severin's review.

@colintheshots Can we also remove the fretboard (and any other) unused dependencies?

mcomella · 2020-01-11T18:51:26Z

CI failure looks like a network error: rerunning.

Note: I did not see any regressions from #7487 (comment) so I'm happy for this to merge. I had wanted to land that potential regression separately from this PR so that any improvements here weren't washed away by a possible known regression.

Reminder from #7510 (review) that we should remove the fretboard dependencies.

…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 force-pushed the fix5182 branch 2 times, most recently from ba7f9b9 to 6ddb663 Compare January 6, 2020 23:23

mcomella requested review from mcomella and removed request for mcomella January 8, 2020 02:37

severinrudie self-assigned this Jan 8, 2020

severinrudie approved these changes Jan 8, 2020

View reviewed changes

mcomella self-requested a review January 10, 2020 01:12

colintheshots force-pushed the fix5182 branch 2 times, most recently from f6a4229 to 005a79d Compare January 10, 2020 17:11

severinrudie approved these changes Jan 10, 2020

View reviewed changes

mcomella changed the title ~~For #5182: Loading experiments on startup is slow, remove Fretboard~~ For #5182: Loading experiments on startup is slow, remove Fretboard! Jan 10, 2020

ekager added the pr:needs-landing PRs that are ready to land [Will be merged by Mergify] label Jan 10, 2020

mcomella removed the pr:needs-landing PRs that are ready to land [Will be merged by Mergify] label Jan 10, 2020

mcomella approved these changes Jan 10, 2020

View reviewed changes

mcomella changed the title ~~For #5182: Loading experiments on startup is slow, remove Fretboard!~~ For #5182: Loading experiments on startup is slow, remove Fretboard!! Jan 11, 2020

colintheshots force-pushed the fix5182 branch from 005a79d to fb99387 Compare January 13, 2020 17:30

colintheshots merged commit 7baf54f into mozilla-mobile:master Jan 13, 2020

colintheshots deleted the fix5182 branch January 13, 2020 18:38

mcomella mentioned this pull request Jan 17, 2020

Remove fretboard to improve startup performance #5182

Closed

liuche mentioned this pull request Jan 22, 2020

Releng for 3.2 #7845

Closed

32 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #5182: Loading experiments on startup is slow, remove Fretboard!! #7510

For #5182: Loading experiments on startup is slow, remove Fretboard!! #7510

colintheshots commented Jan 6, 2020 •

edited

codecov-io commented Jan 6, 2020 •

edited

severinrudie left a comment

severinrudie Jan 8, 2020

mcomella commented Jan 10, 2020 •

edited

colintheshots commented Jan 10, 2020 •

edited

colintheshots commented Jan 10, 2020

severinrudie left a comment

colintheshots commented Jan 10, 2020

bors bot commented Jan 10, 2020

colintheshots commented Jan 10, 2020

mcomella commented Jan 10, 2020

mcomella commented Jan 10, 2020

mcomella commented Jan 10, 2020

mcomella commented Jan 10, 2020

mcomella left a comment

mcomella commented Jan 11, 2020

For #5182: Loading experiments on startup is slow, remove Fretboard!! #7510

For #5182: Loading experiments on startup is slow, remove Fretboard!! #7510

Conversation

colintheshots commented Jan 6, 2020 • edited

Pull Request checklist

After merge

To download an APK when reviewing a PR:

codecov-io commented Jan 6, 2020 • edited

Codecov Report

severinrudie left a comment

Choose a reason for hiding this comment

severinrudie Jan 8, 2020

Choose a reason for hiding this comment

mcomella commented Jan 10, 2020 • edited

colintheshots commented Jan 10, 2020 • edited

colintheshots commented Jan 10, 2020

severinrudie left a comment

Choose a reason for hiding this comment

colintheshots commented Jan 10, 2020

bors bot commented Jan 10, 2020

Configuration problem

colintheshots commented Jan 10, 2020

mcomella commented Jan 10, 2020

mcomella commented Jan 10, 2020

mcomella commented Jan 10, 2020

mcomella commented Jan 10, 2020

mcomella left a comment

Choose a reason for hiding this comment

mcomella commented Jan 11, 2020

colintheshots commented Jan 6, 2020 •

edited

codecov-io commented Jan 6, 2020 •

edited

mcomella commented Jan 10, 2020 •

edited

colintheshots commented Jan 10, 2020 •

edited