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

Fireperf: [Startup improvement] lazy initialize expensive objects in GaugeManager #3011

Merged
merged 6 commits into from
Oct 5, 2021

Conversation

leotianlizhan
Copy link
Contributor

@leotianlizhan leotianlizhan commented Sep 29, 2021

As per b/201001743, the goal is to "Optimize SessionManager getInstance() to check of session enabled flags before initializing gaugeManager".

Why we can't lazy initialize GaugeManager

Currently no matter if verbose session is turned on or off, a method in GaugeManager is guaranteed to be called in FirebasePerfProvider.attachInfo(). Specifically, FirebasePerfProvider.attachInfo() calls SessionManager.updatePerfSession which calls SessionManager.startOrStopCollectingGauges, which then checks if verbose session is on, if yes it calls gaugeManager. startCollectingGauges, otherwise it calls gaugeManager.stopCollectingGauges.
Therefore GaugeManager must be initialized.

But we can lazy initialize expensive objects in GaugeManager

There are 3 relatively expensive objects in GaugeManager that are not used when verbose session is off, but they are currently eagerly initialized. This PR just makes these 3 objects lazy, using a helper class from firebase-components.

Additional optimization

Also swapped a String.replaceAll to String.replace for very slight improvement.

Other changes

Changed CpuGaugeCollector and MemoryGaugeCollector from singletons to normal objects (since they are only owned by GaugeManager which is a singleton already)

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Sep 29, 2021

Coverage Report

Affected SDKs

  • firebase-perf

    SDK overall coverage changed from 70.74% (79975f0) to 70.72% (396a33c0) by -0.02%.

    Filename Base (79975f0) Head (396a33c0) Diff
    CpuGaugeCollector.java 93.10% 92.77% -0.33%
    GaugeManager.java 98.39% 98.43% +0.04%
    MemoryGaugeCollector.java 90.00% 89.66% -0.34%
    PerfSession.java 93.33% 93.22% -0.11%

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 (396a33c0) is created by Prow via merging commits: 79975f0 3aa1bb7.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Sep 29, 2021

Binary Size Report

Affected SDKs

  • firebase-perf

    Type Base (79975f0) Head (396a33c0) Diff
    aar 300 kB 301 kB +147 B (+0.0%)
    apk (aggressive) 980 kB 980 kB +312 B (+0.0%)
    apk (release) 2.45 MB 2.45 MB +364 B (+0.0%)

Test Logs

Notes

Head commit (396a33c0) is created by Prow via merging commits: 79975f0 3aa1bb7.

@leotianlizhan leotianlizhan marked this pull request as ready for review September 30, 2021 20:35
@leotianlizhan leotianlizhan changed the title Lazy initialize ScheduledExecutorService in GaugeManager Lazy initialize executor and cpu/memory gauge collectors in GaugeManager Sep 30, 2021
@leotianlizhan leotianlizhan changed the title Lazy initialize executor and cpu/memory gauge collectors in GaugeManager Fireperf: lazy initialize executor and cpu/memory gauge collectors in GaugeManager Sep 30, 2021
@leotianlizhan leotianlizhan changed the title Fireperf: lazy initialize executor and cpu/memory gauge collectors in GaugeManager Fireperf: lazy initialize expensive objects in GaugeManager Oct 1, 2021
Copy link
Contributor

@jeremyjiang-dev jeremyjiang-dev left a comment

Choose a reason for hiding this comment

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

Good work!

@jeremyjiang-dev
Copy link
Contributor

Is there a point in Cpu and Memory gauge collectors being singletons? They are only owned by GaugeManager which is already a singleton.

I think at this point, it will work if Cpu and Memory gauge collectors are not singletons. The point of making these two being singletons is to make sure there is only one instance of each in the environment if other classes want to use them.

@visumickey
Copy link
Contributor

Question

Is there a point in Cpu and Memory gauge collectors being singletons? They are only owned by GaugeManager which is already a singleton.

Honestly, the only reason they are singletons is not to have redundant collectors of such metrics. But, given the fact they are being used only by the gauge manager, they don't have to be singleton. I would prefer for us to remove them to be singletons. Do you know the complexity in making them non-singleton?

@leotianlizhan
Copy link
Contributor Author

Question

Is there a point in Cpu and Memory gauge collectors being singletons? They are only owned by GaugeManager which is already a singleton.

Honestly, the only reason they are singletons is not to have redundant collectors of such metrics. But, given the fact they are being used only by the gauge manager, they don't have to be singleton. I would prefer for us to remove them to be singletons. Do you know the complexity in making them non-singleton?

It should be quick and easy, unless it breaks a lot of tests which I don't expect to happen. Just updated PR with removal of singleton

@leotianlizhan
Copy link
Contributor Author

/test smoke-tests

1 similar comment
@leotianlizhan
Copy link
Contributor Author

/test smoke-tests

@leotianlizhan leotianlizhan merged commit c91adbe into master Oct 5, 2021
@leotianlizhan leotianlizhan deleted the fireperf-gauge-lazy branch October 5, 2021 20:50
@firebase firebase locked and limited conversation to collaborators Nov 5, 2021
@leotianlizhan leotianlizhan changed the title Fireperf: lazy initialize expensive objects in GaugeManager Fireperf: [Startup improvement] lazy initialize expensive objects in GaugeManager Feb 22, 2022
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.

4 participants