From b03b87cd9d089a48058c399186e23653a9ce7847 Mon Sep 17 00:00:00 2001 From: exaby73 Date: Tue, 5 Nov 2024 19:30:29 +0530 Subject: [PATCH 1/6] fix(firebase-perf): Update device cache only if RC value is different from cached value --- .../firebase/perf/config/ConfigResolver.java | 23 +++++++++++++------ .../perf/config/ConfigResolverTest.java | 17 ++++++++++++++ 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/firebase-perf/src/main/java/com/google/firebase/perf/config/ConfigResolver.java b/firebase-perf/src/main/java/com/google/firebase/perf/config/ConfigResolver.java index 762dff42a7e..55337d28213 100644 --- a/firebase-perf/src/main/java/com/google/firebase/perf/config/ConfigResolver.java +++ b/firebase-perf/src/main/java/com/google/firebase/perf/config/ConfigResolver.java @@ -221,6 +221,7 @@ private boolean getIsSdkEnabled() { // 2. If the value exists in device cache, return this value. // 3. Otherwise, return default value. SdkEnabled config = SdkEnabled.getInstance(); + Optional deviceCacheValue = getDeviceCacheBoolean(config); // 1. Reads value from Firebase Remote Config, saves this value in cache layer if fetch status // is not failure. @@ -230,13 +231,17 @@ private boolean getIsSdkEnabled() { if (remoteConfigManager.isLastFetchFailed()) { return false; } - // b. Cache and return this value. - deviceCacheManager.setValue(config.getDeviceCacheFlag(), rcValue.get()); - return rcValue.get(); + + Boolean newValue = rcValue.get(); + // b. Only cache and return this value if it is different from the current value. + if (!deviceCacheValue.isAvailable() || deviceCacheValue.get() != newValue) { + deviceCacheManager.setValue(config.getDeviceCacheFlag(), newValue); + } + + return newValue; } // 2. If the value exists in device cache, return this value. - Optional deviceCacheValue = getDeviceCacheBoolean(config); if (deviceCacheValue.isAvailable()) { return deviceCacheValue.get(); } @@ -257,17 +262,21 @@ private boolean getIsSdkVersionDisabled() { // 2. If the value exists in device cache, return this value. // 3. Otherwise, return default value. SdkDisabledVersions config = SdkDisabledVersions.getInstance(); + Optional deviceCacheValue = getDeviceCacheString(config); // 1. Reads value from Firebase Remote Config, cache and return this value. Optional rcValue = getRemoteConfigString(config); if (rcValue.isAvailable()) { // Do not check FRC last fetch status because it is the most recent value device can get. - deviceCacheManager.setValue(config.getDeviceCacheFlag(), rcValue.get()); - return isFireperfSdkVersionInList(rcValue.get()); + String newValue = rcValue.get(); + // Only cache and return this value if it is different from the current value. + if (!deviceCacheValue.isAvailable() || !deviceCacheValue.get().equals(newValue)) { + deviceCacheManager.setValue(config.getDeviceCacheFlag(), newValue); + } + return isFireperfSdkVersionInList(newValue); } // 2. If the value exists in device cache, return this value. - Optional deviceCacheValue = getDeviceCacheString(config); if (deviceCacheValue.isAvailable()) { return isFireperfSdkVersionInList(deviceCacheValue.get()); } diff --git a/firebase-perf/src/test/java/com/google/firebase/perf/config/ConfigResolverTest.java b/firebase-perf/src/test/java/com/google/firebase/perf/config/ConfigResolverTest.java index 7ccca34a8ff..16151c5ce92 100644 --- a/firebase-perf/src/test/java/com/google/firebase/perf/config/ConfigResolverTest.java +++ b/firebase-perf/src/test/java/com/google/firebase/perf/config/ConfigResolverTest.java @@ -280,6 +280,23 @@ public void getIsServiceCollectionEnabled_sdkDisabledVersionFlagNoFrc_returnDefa verify(mockDeviceCacheManager, never()).setValue(any(), anyString()); } + @Test + public void getIsServiceCollectionEnabled_deviceCacheHasSameValueAsFrc_returnCacheValue() { + when(mockRemoteConfigManager.getBoolean(FIREBASE_PERFORMANCE_SDK_ENABLED_FRC_KEY)) + .thenReturn(Optional.of(true)); + when(mockDeviceCacheManager.getBoolean(FIREBASE_PERFORMANCE_SDK_ENABLED_CACHE_KEY)) + .thenReturn(Optional.of(true)); + + when(mockDeviceCacheManager.getString(FIREBASE_PERFORMANCE_DISABLED_VERSIONS_CACHE_KEY)) + .thenReturn(Optional.of("")); + when(mockRemoteConfigManager.getString(FIREBASE_PERFORMANCE_DISABLED_VERSIONS_FRC_KEY)) + .thenReturn(Optional.of("")); + + assertThat(testConfigResolver.getIsServiceCollectionEnabled()).isTrue(); + verify(mockDeviceCacheManager, never()).setValue(any(), anyBoolean()); + verify(mockDeviceCacheManager, never()).setValue(any(), anyString()); + } + @Test public void getIsPerformanceCollectionConfigValueAvailable_noDeviceCacheNoRemoteConfig_returnsFalse() { From 9a0e1a0e2433b1cd6503525cac3463ddf78a3a0d Mon Sep 17 00:00:00 2001 From: exaby73 Date: Tue, 5 Nov 2024 20:10:24 +0530 Subject: [PATCH 2/6] fix(firebase-perf): Format --- .../google/firebase/perf/config/ConfigResolverTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/firebase-perf/src/test/java/com/google/firebase/perf/config/ConfigResolverTest.java b/firebase-perf/src/test/java/com/google/firebase/perf/config/ConfigResolverTest.java index 16151c5ce92..24dc6597e4a 100644 --- a/firebase-perf/src/test/java/com/google/firebase/perf/config/ConfigResolverTest.java +++ b/firebase-perf/src/test/java/com/google/firebase/perf/config/ConfigResolverTest.java @@ -283,14 +283,14 @@ public void getIsServiceCollectionEnabled_sdkDisabledVersionFlagNoFrc_returnDefa @Test public void getIsServiceCollectionEnabled_deviceCacheHasSameValueAsFrc_returnCacheValue() { when(mockRemoteConfigManager.getBoolean(FIREBASE_PERFORMANCE_SDK_ENABLED_FRC_KEY)) - .thenReturn(Optional.of(true)); + .thenReturn(Optional.of(true)); when(mockDeviceCacheManager.getBoolean(FIREBASE_PERFORMANCE_SDK_ENABLED_CACHE_KEY)) - .thenReturn(Optional.of(true)); + .thenReturn(Optional.of(true)); when(mockDeviceCacheManager.getString(FIREBASE_PERFORMANCE_DISABLED_VERSIONS_CACHE_KEY)) - .thenReturn(Optional.of("")); + .thenReturn(Optional.of("")); when(mockRemoteConfigManager.getString(FIREBASE_PERFORMANCE_DISABLED_VERSIONS_FRC_KEY)) - .thenReturn(Optional.of("")); + .thenReturn(Optional.of("")); assertThat(testConfigResolver.getIsServiceCollectionEnabled()).isTrue(); verify(mockDeviceCacheManager, never()).setValue(any(), anyBoolean()); From 33466cf918797a57127578fed6c0c9a1009b446d Mon Sep 17 00:00:00 2001 From: exaby73 Date: Wed, 6 Nov 2024 14:40:59 +0530 Subject: [PATCH 3/6] fix(firebase-perf): Add null check for cache value --- .../java/com/google/firebase/perf/config/ConfigResolver.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/firebase-perf/src/main/java/com/google/firebase/perf/config/ConfigResolver.java b/firebase-perf/src/main/java/com/google/firebase/perf/config/ConfigResolver.java index 55337d28213..ed8e6d4657d 100644 --- a/firebase-perf/src/main/java/com/google/firebase/perf/config/ConfigResolver.java +++ b/firebase-perf/src/main/java/com/google/firebase/perf/config/ConfigResolver.java @@ -234,7 +234,7 @@ private boolean getIsSdkEnabled() { Boolean newValue = rcValue.get(); // b. Only cache and return this value if it is different from the current value. - if (!deviceCacheValue.isAvailable() || deviceCacheValue.get() != newValue) { + if (deviceCacheValue == null || !deviceCacheValue.isAvailable() || deviceCacheValue.get() != newValue) { deviceCacheManager.setValue(config.getDeviceCacheFlag(), newValue); } @@ -270,7 +270,7 @@ private boolean getIsSdkVersionDisabled() { // Do not check FRC last fetch status because it is the most recent value device can get. String newValue = rcValue.get(); // Only cache and return this value if it is different from the current value. - if (!deviceCacheValue.isAvailable() || !deviceCacheValue.get().equals(newValue)) { + if (deviceCacheValue == null || !deviceCacheValue.isAvailable() || !deviceCacheValue.get().equals(newValue)) { deviceCacheManager.setValue(config.getDeviceCacheFlag(), newValue); } return isFireperfSdkVersionInList(newValue); From 183ca91a07888f9453531ce92d12ed31d6622e7e Mon Sep 17 00:00:00 2001 From: exaby73 Date: Wed, 6 Nov 2024 15:02:12 +0530 Subject: [PATCH 4/6] fix(firebase-perf): Format --- .../com/google/firebase/perf/config/ConfigResolver.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/firebase-perf/src/main/java/com/google/firebase/perf/config/ConfigResolver.java b/firebase-perf/src/main/java/com/google/firebase/perf/config/ConfigResolver.java index ed8e6d4657d..1ee9d395e03 100644 --- a/firebase-perf/src/main/java/com/google/firebase/perf/config/ConfigResolver.java +++ b/firebase-perf/src/main/java/com/google/firebase/perf/config/ConfigResolver.java @@ -234,7 +234,9 @@ private boolean getIsSdkEnabled() { Boolean newValue = rcValue.get(); // b. Only cache and return this value if it is different from the current value. - if (deviceCacheValue == null || !deviceCacheValue.isAvailable() || deviceCacheValue.get() != newValue) { + if (deviceCacheValue == null + || !deviceCacheValue.isAvailable() + || deviceCacheValue.get() != newValue) { deviceCacheManager.setValue(config.getDeviceCacheFlag(), newValue); } @@ -270,7 +272,9 @@ private boolean getIsSdkVersionDisabled() { // Do not check FRC last fetch status because it is the most recent value device can get. String newValue = rcValue.get(); // Only cache and return this value if it is different from the current value. - if (deviceCacheValue == null || !deviceCacheValue.isAvailable() || !deviceCacheValue.get().equals(newValue)) { + if (deviceCacheValue == null + || !deviceCacheValue.isAvailable() + || !deviceCacheValue.get().equals(newValue)) { deviceCacheManager.setValue(config.getDeviceCacheFlag(), newValue); } return isFireperfSdkVersionInList(newValue); From c2cf4663e6634fe8b23e8455374bb97824ce82ed Mon Sep 17 00:00:00 2001 From: exaby73 Date: Wed, 6 Nov 2024 15:39:47 +0530 Subject: [PATCH 5/6] chore(firebase-perf): Added a changelog entry --- firebase-perf/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/firebase-perf/CHANGELOG.md b/firebase-perf/CHANGELOG.md index 10e74519624..a0508f0c41d 100644 --- a/firebase-perf/CHANGELOG.md +++ b/firebase-perf/CHANGELOG.md @@ -2,6 +2,8 @@ # 21.0.2 +* [fixed] Fixed a performance issue with shared preferences + calling `.apply()` every time a value is read from remote config (#6407) * [fixed] Fixed `IllegalStateException` that happened when starting a trace before Firebase initializes. * [changed] Updated protobuf dependency to `3.25.5` to fix From 054811083bc8d4a919b7617967fa555f949e6c7e Mon Sep 17 00:00:00 2001 From: exaby73 Date: Tue, 12 Nov 2024 20:54:47 +0530 Subject: [PATCH 6/6] chore(firebase-perf): Move Changelog entry to Unreleased section --- firebase-perf/CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/firebase-perf/CHANGELOG.md b/firebase-perf/CHANGELOG.md index a0508f0c41d..9278b0ae17f 100644 --- a/firebase-perf/CHANGELOG.md +++ b/firebase-perf/CHANGELOG.md @@ -1,9 +1,9 @@ # Unreleased - -# 21.0.2 * [fixed] Fixed a performance issue with shared preferences calling `.apply()` every time a value is read from remote config (#6407) + +# 21.0.2 * [fixed] Fixed `IllegalStateException` that happened when starting a trace before Firebase initializes. * [changed] Updated protobuf dependency to `3.25.5` to fix