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.

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

fix(firebase-perf): Update device cache only if RC value is different from cached value #6431

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix(firebase-perf): Update device cache only if RC value is different from cached value #6431

wants to merge 8 commits into from

Conversation

exaby73
Copy link

@exaby73 exaby73 commented Nov 5, 2024

Fixes #6407

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024 •
edited
Loading

Release note changes

The following release notes were modified. Please ensure they look correct.

Release Notes
firebase-perf 
### {{perfmon}} version 21.0.3 {: #performance_v21-0-3}

* {{fixed}} Fixed a performance issue with shared preferences
  calling `.apply()` every time a value is read from remote config GitHub [#6407](//github.com/firebase/firebase-android-sdk/issues/6407){: .external}

#### {{perfmon}} Kotlin extensions version 21.0.3 {: #performance-ktx_v21-0-3}

The Kotlin extensions library transitively includes the updated
`firebase-perf` library. The Kotlin extensions library has no additional
updates.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024

Vertex AI Mock Responses Check ⚠️

A newer major version of the mock responses for Vertex AI unit tests is available. update_responses.sh should be updated to clone the latest version of the responses: v5.1

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024 •
edited
Loading

Test Results

  110 files  + 56    110 suites  +56   2m 17s ⏱️ +4s
  969 tests +454    969 ✅ +455  0 💤  - 1  0 ❌ ±0 
1 946 runs  +916  1 946 ✅ +918  0 💤  - 2  0 ❌ ±0 

Results for commit cc34082. ± Comparison against base commit 57e401c.

This pull request removes 515 and adds 969 tests. Note that renamed tests count towards both. 
com.google.firebase.dataconnect.AnyValueSerializerUnitTest ‑ descriptor should have expected values
com.google.firebase.dataconnect.AnyValueSerializerUnitTest ‑ deserialize() should throw UnsupportedOperationException
com.google.firebase.dataconnect.AnyValueSerializerUnitTest ‑ serialize() should throw UnsupportedOperationException
com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(Boolean) creates an object with the expected value
com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(Double) creates an object with the expected value (edge cases)
com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(Double) creates an object with the expected value (normal cases)
com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(List) creates an object with the expected value (edge cases)
com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(List) creates an object with the expected value (normal cases)
com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(Map) creates an object with the expected value (edge cases)
com.google.firebase.dataconnect.AnyValueUnitTest ‑ constructor(Map) creates an object with the expected value (normal cases)
…

com.google.firebase.perf.FirebasePerfRegistrarTest ‑ testGetComponents
com.google.firebase.perf.FirebasePerformanceTest ‑ firebasePerformanceInitialization_providesRcProvider_remoteConfigManagerIsSet
com.google.firebase.perf.FirebasePerformanceTest ‑ initFirebasePerformance_injectsMetadataIntoConfigResolver
com.google.firebase.perf.FirebasePerformanceTest ‑ initializeFirebasePerformance_emptyMetadataAndCache_metadataAndContextInjected
com.google.firebase.perf.FirebasePerformanceTest ‑ setDataCollectionDefaultEnabled_whenForceDisabledThenCleared_respectsGlobalFlag
com.google.firebase.perf.FirebasePerformanceTest ‑ setDataCollectionDefaultEnabled_whenForceDisabledThenCleared_respectsManifestTrue
com.google.firebase.perf.FirebasePerformanceTest ‑ setDataCollectionDefaultEnabled_whenForceEnabledThenCleared_respectsGlobalFlag
com.google.firebase.perf.FirebasePerformanceTest ‑ setDataCollectionDefaultEnabled_whenForceEnabledThenCleared_respectsManifestFalse
com.google.firebase.perf.FirebasePerformanceTest ‑ testAddingMoreThanMaxLocalAttributes
com.google.firebase.perf.FirebasePerformanceTest ‑ testBothManifestsAgree
…

♻️ This comment has been updated with latest results.

koflox
koflox reviewed Nov 7, 2024
View reviewed changes
firebase-perf/CHANGELOG.md Outdated Show resolved Hide resolved
@hakanbagci hakanbagci mentioned this pull request Nov 8, 2024
Open
visumickey
visumickey approved these changes Nov 11, 2024
View reviewed changes
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.

Thanks for making the change. I'm just curious as to why just these 2 flags and not other values? Is this because we are trying to optimize only the startup time performance?

firebase-perf/CHANGELOG.md Show resolved Hide resolved
@exaby73
Copy link
Author

exaby73 commented Nov 12, 2024

@visumickey The issue is every time isPerformanceMonitoringEnabled is called, if a value in RC is available, SharedPreferenced.apply() was called twice. Since this method can be called for every trace, this queued up many calls to apply() which caused the main thread to wait for a long time when it finally flushed the queue causing ANRs in many cases.

This PR is aimed to fix that specific issue, but I can't see a reason why we can't do it for all values

@visumickey
Copy link
Contributor

@visumickey The issue is every time isPerformanceMonitoringEnabled is called, if a value in RC is available, SharedPreferenced.apply() was called twice. Since this method can be called for every trace, this queued up many calls to apply() which caused the main thread to wait for a long time when it finally flushed the queue causing ANRs in many cases.

This PR is aimed to fix that specific issue, but I can't see a reason why we can't do it for all values

Thanks for clarifying. Let's keep this PR narrow down to this issue. We can make the refactor to the rest of the code later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koflox koflox koflox left review comments

@mehdiyari mehdiyari mehdiyari left review comments

@visumickey visumickey visumickey approved these changes

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

Successfully merging this pull request may close these issues.

[firebase-perf] com.google.firebase.perf.trace function has significant impact on performance and causing ANRs
5 participants
@exaby73 @visumickey @mehdiyari @koflox @google-oss-bot