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

Improve startup performance of the Fenix wrapper around Glean (~130ms perfTest) #7847

Closed
mcomella opened this issue Jan 22, 2020 · 4 comments
Assignees
Labels
needs:triage Issue needs triage performance Possible performance wins

Comments

@mcomella
Copy link
Contributor

mcomella commented Jan 22, 2020

We're trying to move everything non-essential until after visual completeness in order to improve startup time. GleanMetricsService runs code that may be fine to run after visual completeness: find out and if so, move it!

We should ask the Glean team to review the PR to ensure we're using it correctly.


Old comment 0:

The Glean SDK has performance issues on startup to be addressed in #7746. However, the way Fenix wraps Glean also has performance issues to be addressed by this ticket. In particular, the following methods run (times on Pixel 2, forPerformanceTest build):

  • 14.35ms GleanMetricsService.start
  • Coroutines launched in GleanMetricsService
    • 3.7ms Pings.<clint>
    • 78.28ms setStartupMetrics
      • ? ActivationPing.checkAndSend
  • 3.01ms x3 GleanMetricsSerivce.shouldTrack
  • 9.65ms x3 GleanMetricsService.track (called in other methods)

For a total of 134.31ms.

Questions

  • Can all of these operations wait until after visual completeness? In particular...
    • Each track call
    • setStartupMetrics?
    • the ActivationPing?
  • If not, can these operations be moved to a background thread during init? Glean has mentioned all of their SDK methods should be thread safe.

Reach out to the Fenix team (boek?) to determine which methods need to be called when. We can also consider delegating some product requirements (e.g. ActivityPing, StartupMetrics needed right away?) to ecsmyth.

┆Issue is synchronized with this Jira Task

@mcomella mcomella added the performance Possible performance wins label Jan 22, 2020
@github-actions github-actions bot added the needs:triage Issue needs triage label Jan 22, 2020
@mcomella mcomella changed the title Improve startup performance of Fenix's use of Glean Improve startup performance of Fenix's use of Glean (~130ms perfTest) Jan 22, 2020
@Dexterp37
Copy link
Contributor

I wonder if this abstraction is still necessary: given that Leanplum only has a few metrics and most of the metrics use Glean, what about using the Glean APIs at the call sites directly? Then there would not be any need for touching multiple files when adding a metric and, more importantly, there will be no performance bottleneck.

@mcomella mcomella changed the title Improve startup performance of Fenix's use of Glean (~130ms perfTest) Improve startup performance of the Fenix wrapper around Glean (~130ms perfTest) Feb 6, 2020
@jimporter jimporter self-assigned this Feb 7, 2020
@jimporter
Copy link

@ekager: @mcomella suggested you might have thoughts about how to refactor our usage of GleanMetricsService (possibly even removing it entirely and just using Glean directly as @Dexterp37 suggested). Any thoughts here? I think our main goal should probably be deferring as much of the Glean code as possible to after startup... though maybe just removing GleanMetricsService would cut back on some overhead here?

@jimporter
Copy link

jimporter commented Feb 25, 2020

Not filing a PR just yet since my patch is based on the OnVisualCompleteness patch that hasn't landed, but with this commit, I'm seeing a 21ms improvement to startup time (on a G5 with 100 runs each and a standard deviation of 10ms). Not as big as the issue title suggested we might be able to get, but it's a simple patch. We might be able to eke out a bit more if we defer other bits...

@jimporter
Copy link

Resolved in #8923

gmierz pushed a commit to gmierz/fenix that referenced this issue Mar 5, 2020
@liuche liuche mentioned this issue Mar 12, 2020
32 tasks
severinrudie pushed a commit to severinrudie/fenix that referenced this issue Mar 18, 2020
@liuche liuche mentioned this issue Mar 24, 2020
32 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs:triage Issue needs triage performance Possible performance wins
Projects
None yet
Development

No branches or pull requests

3 participants