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

FNPRMS fully drawn time does not take into account top sites (M4) #7781

Closed
mcomella opened this issue Jan 17, 2020 · 6 comments
Closed

FNPRMS fully drawn time does not take into account top sites (M4) #7781

mcomella opened this issue Jan 17, 2020 · 6 comments
Assignees
Labels
eng:qa:not-needed Added by QA to issues that cannot be tested needs:triage Issue needs triage performance Possible performance wins
Milestone

Comments

@mcomella
Copy link
Contributor

mcomella commented Jan 17, 2020

When the user has tabs or collections, the RecyclerView draws once for "No tabs" and draws again when the tabs arrive. The current reportFullyDrawn implementation does not take this second draw into account and thus is not entirely accurate.

┆Issue is synchronized with this Jira Task

@mcomella mcomella added the performance Possible performance wins label Jan 17, 2020
@mcomella mcomella added this to Needs prioritization in Performance, front-end roadmap via automation Jan 17, 2020
@github-actions github-actions bot added the needs:triage Issue needs triage label Jan 17, 2020
@mcomella
Copy link
Contributor Author

This behavior may change in #7740 because the RecyclerView takes a long time to inflate.

@hawkinsw
Copy link
Contributor

After investigation today with @MarcLeclair , I believe that this needs to be a very high priority. After a recent change to the application (during the nightly migration), there is a new intent dispatcher that has been inserted into startup. This impacts when the application's first layout traversal happens -- it moves it from after the home fragment is inflated to before the home fragment is created. This means that the Displayed time (upon which we rely for our FNPRMS numbers) will now no longer reflect the time that it takes to inflate the home fragment.

@MarcLeclair and I confirmed this from a theoretical and an empirical perspective. In order to test it empirically, we added a Thread.sleep(5000) in the HomeActivity.onCreate() method and watched that our Displayed time did not skyrocket. We compared that to adding a Thread.sleep(5000) to the GleanMetricsService.setStartupMetrics() method (which executes before the first layout traversal) where that addition did affect the Displayed time by an appropriate amount.

Not only does this reinforce the fact that we need to add reportFullyDrawn and use that for FNPRMs, but it also confirms the investigation into when the Displayed flag.

@mcomella mcomella moved this from Needs prioritization to Backlog (prioritized) in Performance, front-end roadmap Jan 21, 2020
@mcomella mcomella self-assigned this Jan 22, 2020
@mcomella
Copy link
Contributor Author

I landed #7780 and I believe it already addresses the case hawkinsw is concerned about above #7781 (comment). Addressing this ticket would fix the case where we have open tabs (and thus we redraw the RV again) but that isn't one of our current requirements. Sending this back to triage...

@mcomella mcomella moved this from Backlog (prioritized) to Needs prioritization in Performance, front-end roadmap Jan 22, 2020
@mcomella mcomella moved this from Needs prioritization to Waiting in Performance, front-end roadmap Feb 6, 2020
@mcomella mcomella changed the title Instrument 2nd RecyclerView update in reportFullyDrawn Fully drawn time does not take into account top sites or tabs loaded Feb 18, 2020
@mcomella mcomella moved this from Waiting to Needs prioritization in Performance, front-end roadmap Feb 18, 2020
@mcomella mcomella removed their assignment Feb 18, 2020
@mcomella
Copy link
Contributor Author

Visual completeness is strange here: we display an animation to show the async-loaded tabs but we probably don't want to include the animation in our time to visual completeness. Theoretically, we could instrument the first frame of that animation though.

@mcomella mcomella moved this from Needs prioritization to Backlog (prioritized) in Performance, front-end roadmap Feb 19, 2020
@mcomella mcomella self-assigned this Feb 19, 2020
@mcomella mcomella moved this from Backlog (prioritized) to In progress in Performance, front-end roadmap Feb 20, 2020
mcomella added a commit to mcomella/fenix that referenced this issue Feb 20, 2020
Eyeballing my output in *Debug builds, this adds approximately 100ms or
slightly less from first frame drawn to visually complete time.
@mcomella mcomella changed the title Fully drawn time does not take into account top sites or tabs loaded FNPRMS fully drawn time does not take into account top sites (M4) Feb 24, 2020
mcomella added a commit to mcomella/fenix that referenced this issue Feb 24, 2020
Eyeballing my output in *Debug builds on my P2, this adds approximately 115ms
or slightly less from first frame drawn to visually complete time.
mcomella added a commit to mcomella/fenix that referenced this issue Feb 27, 2020
Eyeballing my output in *Debug builds on my P2, this adds approximately 115ms
or slightly less from first frame drawn to visually complete time.
mcomella added a commit to mcomella/fenix that referenced this issue Feb 27, 2020
mcomella added a commit to mcomella/fenix that referenced this issue Feb 28, 2020
Eyeballing my output in *Debug builds on my P2, this adds approximately 115ms
or slightly less from first frame drawn to visually complete time.
mcomella added a commit to mcomella/fenix that referenced this issue Feb 28, 2020
Eyeballing my output in *Debug builds on my P2, this adds approximately 115ms
or slightly less from first frame drawn to visually complete time.
mcomella added a commit to mcomella/fenix that referenced this issue Feb 28, 2020
Eyeballing my output in *Debug builds on my P2, this adds approximately 115ms
or slightly less from first frame drawn to visually complete time.
mcomella added a commit to mcomella/fenix that referenced this issue Feb 28, 2020
Eyeballing my output in *Debug builds on my P2, this adds approximately 115ms
or slightly less from first frame drawn to visually complete time.
mcomella added a commit that referenced this issue Feb 28, 2020
Eyeballing my output in *Debug builds on my P2, this adds approximately 115ms
or slightly less from first frame drawn to visually complete time.
@mcomella
Copy link
Contributor Author

mcomella commented Mar 2, 2020

The PR #8615 landed. Unfortunately, I don't see the consistent regression we expect in the FNPRMS results: it landed on Feb 28 and Feb 29 is 8ms regression while Mar 1 is additional 9ms regression. I wonder if another improvement landed.

@mcomella
Copy link
Contributor Author

mcomella commented Mar 2, 2020

This does not look like it will impact stability nor can it be tested by QA so not requesting QA and can close.

@mcomella mcomella added the eng:qa:not-needed Added by QA to issues that cannot be tested label Mar 2, 2020
@mcomella mcomella added this to the v4.0 milestone Mar 2, 2020
@mcomella mcomella closed this as completed Mar 2, 2020
Performance, front-end roadmap automation moved this from In progress to Done Mar 2, 2020
gmierz pushed a commit to gmierz/fenix that referenced this issue Mar 5, 2020
Eyeballing my output in *Debug builds on my P2, this adds approximately 115ms
or slightly less from first frame drawn to visually complete time.
@liuche liuche mentioned this issue Mar 12, 2020
32 tasks
@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
eng:qa:not-needed Added by QA to issues that cannot be tested needs:triage Issue needs triage performance Possible performance wins
Development

No branches or pull requests

2 participants