Skip to content
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.

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

Move app start trace build logic into a non-main thread. #3008

Merged
merged 4 commits into from
Sep 30, 2021

Conversation

jeremyjiang-dev
Copy link
Contributor

@jeremyjiang-dev jeremyjiang-dev commented Sep 28, 2021

Summary:

This effort will cut down the trace build time during app startup from ~30ms to a few ms.

@google-cla google-cla bot added the cla: yes Override cla label Sep 28, 2021
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Sep 28, 2021

Coverage Report

Affected SDKs

No changes between base commit (a692f63) and head commit (2f2938f0).

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (2f2938f0) is created by Prow via merging commits: a692f63 5aefc34.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Sep 28, 2021

Binary Size Report

Affected SDKs

  • firebase-perf

    Type Base (a692f63) Head (2f2938f0) Diff
    aar 300 kB 300 kB +547 B (+0.2%)
    apk (aggressive) 980 kB 980 kB +52 B (+0.0%)
    apk (release) 2.45 MB 2.45 MB +352 B (+0.0%)

Test Logs

Notes

Head 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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I stored SessionManager.getInstance().perfSession() into a class variable.

@visumickey
Copy link
Contributor

visumickey commented Sep 29, 2021

@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.
Sorry for the confusion. I made some code format changes, which made the diff hard to read. I have updated the description and the change should be very straightforward.

new ThreadPoolExecutor(
CORE_POOL_SIZE,
MAX_POOL_SIZE,
/* keepAliveTime= */ 10,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Done.

MAX_POOL_SIZE,
/* keepAliveTime= */ 10,
TimeUnit.SECONDS,
new LinkedBlockingQueue<>()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@visumickey visumickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure to test your changes to see a successful logging of the app start trace.

🚀


if (isRegisteredForLifecycleCallbacks) {
// After AppStart trace is logged, we can unregister this callback.
unregisterActivityLifecycleCallbacks();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jeremyjiang-dev
Copy link
Contributor Author

Please make sure to test your changes to see a successful logging of the app start trace.

🚀

Thanks for reviewing the PR! I have tested and saw the logging of the app start trace.

Copy link
Contributor

@leotianlizhan leotianlizhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jeremyjiang-dev jeremyjiang-dev merged commit 1e58174 into master Sep 30, 2021
@jeremyjiang-dev jeremyjiang-dev deleted the perfTraceBuilder branch September 30, 2021 19:15
@firebase firebase locked and limited conversation to collaborators Oct 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes Override cla size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants