Skip to content

Fireperf: fix screen trace in multi-activity apps #3311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Mar 7, 2022
Merged

Fireperf: fix screen trace in multi-activity apps #3311

merged 5 commits into from
Mar 7, 2022

Conversation

leotianlizhan
Copy link
Contributor

@leotianlizhan leotianlizhan commented Jan 13, 2022 •
edited
Loading

b/210055697

Cause:

In multi-activity apps, when Activity 1 starts Activity 2, FrameMetricsAggregator does not work when frameMetricsAggregator.add(activity2) is called before reset() of activity1. That is the current behaviour because we use onStart and onStop. The order of these lifecycle callbacks are well defined in the activity lifecycle documentation, "Coordinating activities" section.

Fix:

Move the "stopping of screen traces" from onActivityStopped to onActivityPostPaused. According to the same documentation, the order of these callbacks are well defined, and it is guaranteed that activity 1's onPause will happen before activity 2's onStart.

The reason onActivityPostPaused is picked over onActivityPaused is because onActivityPaused is called at the beginning of onPause, and onPause then calls each fragment's onPause. If reset() is called in onActivityPaused, then the last fragment in the activity won't have a chance to read its frame metrics.

Consequence:

Fragment screen traces must use onPause instead of onStop because frameMetricsAggregator.reset() is moved from onStop to onPause.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jan 13, 2022 •
edited
Loading

Coverage Report 1

Affected Products

  • firebase-perf

    Overall coverage changed from 70.80% (46b3dfd) to 70.82% (91cd6cd) by +0.02%.

    FilenameBase (46b3dfd)Merge (91cd6cd)Diff
    AppStateMonitor.java86.63%87.01%+0.38%

Test Logs

Notes

  • Commit (91cd6cd) is created by Prow via merging PR base commit (46b3dfd) and head commit (b9926ae).
  • Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/GmtPtEcEsi.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jan 13, 2022 •
edited
Loading

Size Report 1

Affected Products

  • firebase-perf

    TypeBase (46b3dfd)Merge (91cd6cd)Diff
    aar301 kB301 kB+188 B (+0.1%)
    apk (aggressive)981 kB981 kB+144 B (+0.0%)
    apk (release)2.45 MB2.45 MB+120 B (+0.0%)

Test Logs

Notes

  • Commit (91cd6cd) is created by Prow via merging PR base commit (46b3dfd) and head commit (b9926ae).

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/Y18I4ltjji.html

@leotianlizhan leotianlizhan marked this pull request as draft January 13, 2022 16:54
@leotianlizhan leotianlizhan marked this pull request as ready for review January 13, 2022 19:14
@leotianlizhan
Copy link
Contributor Author

/retest

@leotianlizhan
Copy link
Contributor Author

/run binary-size
/test check-changed

@leotianlizhan
Copy link
Contributor Author

/test check-changed

@leotianlizhan leotianlizhan changed the title Perf: fix screen trace in multi-activity apps Fireperf: fix screen trace in multi-activity apps Jan 14, 2022
@leotianlizhan leotianlizhan merged commit 5b10051 into master Mar 7, 2022
@leotianlizhan leotianlizhan deleted the multiActivityScreenTraceFix branch March 7, 2022 18:23
jeremyjiang-dev pushed a commit that referenced this pull request Mar 9, 2022
@leotianlizhan @jeremyjiang-dev
Fireperf: fix screen trace in multi-activity apps (#3311)
4f6e68a 
* mstop screen trace in onPause

* onActivityPostPaused

* fix test

* fix test typo

* add debug message
@leotianlizhan leotianlizhan mentioned this pull request Mar 21, 2022
Merged
@firebase firebase locked and limited conversation to collaborators Apr 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@visumickey visumickey visumickey approved these changes

@jeremyjiang-dev jeremyjiang-dev jeremyjiang-dev approved these changes

Assignees
No one assigned
Labels
size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@leotianlizhan @google-oss-bot @visumickey @jeremyjiang-dev