Move app start trace build logic into a non-main thread. #3008
Conversation
Coverage ReportAffected SDKsNo changes between base commit (a692f63) and head commit (2f2938f0). Test Logs
NotesHTML coverage reports can be produced locally with Head commit (2f2938f0) is created by Prow via merging commits: a692f63 5aefc34. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (2f2938f0) is created by Prow via merging commits: a692f63 5aefc34. |
| .setClientStartTimeUs(onStartTime.getMicros()) | ||
| .setDurationUs(onStartTime.getDurationMicros(onResumeTime)); | ||
| subtraces.add(temp.build()); | ||
| subtraces.add(traceMetricBuilder.build()); | ||
|
|
||
| metric | ||
| .addAllSubtraces(subtraces) | ||
| .addPerfSessions(SessionManager.getInstance().perfSession().build()); |
There was a problem hiding this comment.
is this thread-safe? Would it make sense to add synchronized to SessionManager.getInstance()? But transportManager.log 2 lines down also calls SessionManager.getInstance() in a different thread already, so I'm not sure if it's already thread-safe.
There was a problem hiding this comment.
Good catch! I stored SessionManager.getInstance().perfSession() into a class variable.
|
@jeremyjiang-dev Can you add some details on what your PR does? The description of the PR shows the outcomes and not so much on what has changed. |
| new ThreadPoolExecutor( | ||
| CORE_POOL_SIZE, | ||
| MAX_POOL_SIZE, | ||
| /* keepAliveTime= */ 10, |
There was a problem hiding this comment.
I would suggest to have the keepAliveTime of the executor to be 60 seconds or more. Eg. 60+10 seconds. This is since the max allowed time for us to log the app start time is 1 minute.
There was a problem hiding this comment.
Good catch! Done.
| MAX_POOL_SIZE, | ||
| /* keepAliveTime= */ 10, | ||
| TimeUnit.SECONDS, | ||
| new LinkedBlockingQueue<>())); |
There was a problem hiding this comment.
Since we are using a blockingQueue, should we have a capacity set?
Reference: go/appengine-java-threading#blockingqueue
Reference 2: https://developer.android.com/reference/java/util/concurrent/LinkedBlockingQueue
The optional capacity bound constructor argument serves as a way to prevent excessive queue expansion. The capacity, if unspecified, is equal to Integer#MAX_VALUE. Linked nodes are dynamically created upon each insertion unless this would bring the queue above capacity.
There was a problem hiding this comment.
Great suggestions! A bounded LinkedBlockingQueue could save some memory since the capacity would be Integer#MAX_VALUE if not specified. I set the capacity to 1 because we will log _as once for an application run.
|
|
||
| if (isRegisteredForLifecycleCallbacks) { | ||
| // After AppStart trace is logged, we can unregister this callback. | ||
| unregisterActivityLifecycleCallbacks(); |
There was a problem hiding this comment.
Just out of curiousity - Can this happen on non-main thread? Just thinking if we can delay this for later on a non-main thread.
There was a problem hiding this comment.
We cannot. If we delay this, it is possible that the app enters onActivityResumed multiple times before the executor executes so that we might log the startup time multiple times.
Thanks for reviewing the PR! I have tested and saw the logging of the app start trace. |
Summary:
This effort will cut down the trace build time during app startup from ~30ms to a few ms.