Fireperf: [Startup improvement] move expensive code to background thread in FirebasePerformance constructor#3044
Conversation
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (c16ba24d) is created by Prow via merging commits: ddcee71 6efc259. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (c16ba24d) is created by Prow via merging commits: ddcee71 6efc259. |
FirebasePerformance constructor
|
/test check-changed |
I don't think we can put
I am not too clear on this question. |
|
Can you summarize what you have done in the PR description? |
firebase-perf/src/main/java/com/google/firebase/perf/FirebasePerformance.java
Outdated
Show resolved
Hide resolved
firebase-perf/src/main/java/com/google/firebase/perf/FirebasePerformance.java
Outdated
Show resolved
Hide resolved
|
The changes LGTM overall. Please fix the failed checks. |
|
/test check-changed |
|
/test smoke-tests |
visumickey
left a comment
There was a problem hiding this comment.
@leotianlizhan I think we need to re-think the implementation strategy for this change. I'm worried that some of these changes would add more complexity to our SDK.
firebase-perf/src/main/java/com/google/firebase/perf/FirebasePerformance.java
Outdated
Show resolved
Hide resolved
| PerfSession appStartSession = SessionManager.getInstance().perfSession(); | ||
| this.syncInitFuture = | ||
| executor.submit( | ||
| () -> { | ||
| gaugeManager.setApplicationContext(appContext); | ||
| if (appStartSession.isGaugeAndEventCollectionEnabled()) { | ||
| gaugeManager.logGaugeMetadata( | ||
| appStartSession.sessionId(), ApplicationProcessState.FOREGROUND); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
This piece of code in Firebase Performance looks unsafe. Ideally anything to do deal with Session should not happen in Firebaseperformance whose purpose is to bootstrap the SDK. One possible alternate would be to send the applicationContext to gaugeManager and when a context is received, gaugeManager can choose to log the gaugeMetadata.
There was a problem hiding this comment.
Moved to a new method SessionManager.setApplicationContext. Giving responsibility to SessionManager because SessionManager owns both perfSession and gaugeManager. This also removes FirebasePerformance's dependency on GaugeManager, which helps with "de-singletoning" GaugeManager in the future.
firebase-perf/src/main/java/com/google/firebase/perf/session/SessionManager.java
Outdated
Show resolved
Hide resolved
firebase-perf/src/main/java/com/google/firebase/perf/transport/TransportManager.java
Outdated
Show resolved
Hide resolved
|
/test check-changed |
| final PerfSession startUpSession = perfSession; | ||
| ExecutorService executorService = Executors.newSingleThreadExecutor(); | ||
| executorService.execute(() -> gaugeManager.initializeGaugeMetadataManager(appContext)); | ||
| syncInitFuture = | ||
| executorService.submit( | ||
| () -> { | ||
| if (startUpSession.isGaugeAndEventCollectionEnabled()) { | ||
| gaugeManager.logGaugeMetadata( | ||
| startUpSession.sessionId(), ApplicationProcessState.FOREGROUND); | ||
| } | ||
| }); |
There was a problem hiding this comment.
gaugeManager is used to logGaugeMetadata. Why do you need sessionManager's executor to do this? Can you use gaugeManager's executor to do this?
There was a problem hiding this comment.
There are 2 reasons:
- SessionManager owns
perfSession, which is required to know if session is verbose. I can passperfSessionintogaugeManager.initializeGaugeMetadataManagerand let it make decisions there, but it's clearer in my opinion for GaugeManager to not know aboutPerfSession, and let the decision aroundPerfSessionbe made inSessionManager. This code is equivalent toSessionManager.logGaugeMetadataIfCollectionEnabled, actually I will refactor it to use that method to make the responsibilities/ownership more clear. - I wanted to use a new local executor instead of using the existing
Lazy<ScheduledExecutorService>inGaugeManagerbecause in the case that session is not verbose, using the existing lazy-initialized executor inGaugeManagerwould initialize it and it will never shut down even though it doesn't do anything afterwards. This current executor will be shutdown by the garbage collector once it goes out of scope (this method finishes).
|
/retest |
|
/test check-changed |
| * (currently that is before onResume finishes), because {@link #perfSession} can be changed by | ||
| * {@link #onUpdateAppState(ApplicationProcessState)} once {@link AppStateMonitor#isColdStart()} |
There was a problem hiding this comment.
NIT: Just a minor cleanup to avoid the situation where onUpdateAppState can change the perfSession (Ideally PerfSession should get updated only on a genuine application state change). Can you add another condition in onUpdateAppState API (lines 102-103), where we check if the newAppState passed is different from the currently existing state and update the perfSession only if the state changes?
For eg: Default value of appstate (currentAppState) in SessionManager would be Foreground. If we call onUpdateAppState with Foreground, that should not lead towards a new perfSession and in the current code that is possible.
This would also prevent the change in PerfSession when the cold start phase of the application changes.
firebase-perf/src/main/java/com/google/firebase/perf/session/gauges/GaugeManager.java
Show resolved
Hide resolved
firebase-perf/src/main/java/com/google/firebase/perf/session/SessionManager.java
Outdated
Show resolved
Hide resolved
FirebasePerformance constructorFirebasePerformance constructor
b/201549230
Goal
Moving expensive operations in
FirebasePerformance(...)constructor to background worker threads.Result of this PR
Reduces
FirebasePerformance(...)constructor by ~1ms, which is about 40% of the constructor.Detail
Moves
gaugeManager.setApplicationContextto background thread, which is the most expensive part of constructor. This PR also fixes a bug with gaugeMetadata not being sent to backend for the startup session, which is now done right aftergaugeManager.setApplicationContext.