Optimize SDK initialization when requests executed before any activity starts #1204

tonidero · 2023-08-18T12:58:48Z

Description

We discovered that the SDK might take a while to respond to offerings/customer info operations if they are started before any Activity has had the chance to start. This is because the SDK considers the app to be in the background, and we have some optimizations in place where we add some jittering to backend requests performed in the background

The approach in this PR is to make it so:

If a request without a jitter delay is started after a request with jitter (but before it's completed), we move the callbacks to use the response from the request without jittering.
If a request with jittering is started after a request without jitter (but before it's completed), we will not add the request with jitter but just reuse the existing request without jittering.

Note that with this approach, we might be duplicating requests in these situations... But I think it should be ok, lmk if you think otherwise

…have jittering

…s after non-jittered calls

tonidero

I haven't started on tests, since I wanted to gather some feedback first

tonidero · 2023-08-18T13:00:08Z

purchases/src/main/kotlin/com/revenuecat/purchases/common/offerings/OfferingsManager.kt

@@ -94,6 +95,7 @@ internal class OfferingsManager(
            },
            onSuccess = { offerings ->
                offeringsCache.cacheOfferings(offerings, offeringsJSON)
+                offeringsCache.setOfferingsCacheTimestampToNow()


This is another optimization I had to do... If we left it where it was before, we wouldn't use the new mechanisms to optimize the requests trying to use the request that is supposed to finish earlier.

I wonder if these two lines should just be a part of cacheOfferings in offeringsCache and guarantee atomicity there

Feels like it'd be conceptually cleaner and slightly easier to make it synchronized?

Certainly, these 2 could be joined (this is the only place they are both called) I will do so in a different PR.

tonidero · 2023-08-18T13:01:41Z

.../src/main/kotlin/com/revenuecat/purchases/subscriberattributes/SubscriberAttributesPoster.kt

@@ -25,6 +26,7 @@ internal class SubscriberAttributesPoster(
            Endpoint.PostAttributes(appUserID),
            mapOf("attributes" to attributes),
            postFieldsToSign = null,
+            delay = Delay.DEFAULT,


This is another change that I thought was fine but lmk if you have any concerns... Basically, this will add jittering to the subscriber attributes requests, which didn't exist before. I don't think it's critical so it made sense to me to delay it in favor of other requests.

codecov · 2023-08-18T13:11:46Z

Codecov Report

Merging #1204 (088de84) into main (7feda65) will decrease coverage by 0.02%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##             main    #1204      +/-   ##
==========================================
- Coverage   85.88%   85.87%   -0.02%     
==========================================
  Files         183      183              
  Lines        6229     6252      +23     
  Branches      905      910       +5     
==========================================
+ Hits         5350     5369      +19     
- Misses        534      537       +3     
- Partials      345      346       +1

Files Changed	Coverage Δ
...com/revenuecat/purchases/strings/NetworkStrings.kt	`0.00% <ø> (ø)`
.../kotlin/com/revenuecat/purchases/common/Backend.kt	`85.13% <79.31%> (-0.01%)`	⬇️
...n/com/revenuecat/purchases/amazon/AmazonBackend.kt	`86.66% <100.00%> (+0.45%)`	⬆️
...n/com/revenuecat/purchases/common/BackendHelper.kt	`96.15% <100.00%> (-3.85%)`	⬇️
...cat/purchases/common/offerings/OfferingsManager.kt	`83.87% <100.00%> (ø)`
...subscriberattributes/SubscriberAttributesPoster.kt	`100.00% <100.00%> (ø)`

vegaro · 2023-08-18T15:30:13Z

purchases/src/main/kotlin/com/revenuecat/purchases/common/Backend.kt

+        delay: Delay = Delay.NONE,
+    ) {
+        val foregroundCacheKey = Pair(cacheKey.first, false)
+        val cacheKeyToUse = if (cacheKey.second && callbacks.containsKey(foregroundCacheKey)) {


can you create variables for cacheKey.first and cacheKey.second? I think it's a bit hard to follow what's what

Also maybe extract callbacks.containsKey(foregroundCacheKey) into a boolean with a descriptive name, something like foregroundCallAlreadyInPlace

can you create variables for cacheKey.first and cacheKey.second? I think it's a bit hard to follow what's what

Hmm yeah, I might create a data class to have these 2 so it's clearer 👍

I created a data class to make things clearer here: 6aa761f, also extracted those conditions to variables to make the code more legible.

vegaro · 2023-08-18T15:34:00Z

purchases/src/main/kotlin/com/revenuecat/purchases/common/Backend.kt

+            cacheKey
+        }
+        addCallback(call, dispatcher, cacheKeyToUse, functions, delay)
+        moveBackgroundedCallbacksToForegroundedCallbacksIfNeeded(this, cacheKey.first, cacheKey.second)


It's only necessary to do this if this new callback being added is for an unjittered call right?

Yes, that's correct. We do that check inside the method, but we might be able to move it here.

Removed that function and moved it to the main one, so it's clearer in 6aa761f

vegaro · 2023-08-18T15:46:20Z

purchases/src/main/kotlin/com/revenuecat/purchases/common/offerings/OfferingsManager.kt

@@ -61,11 +61,12 @@ internal class OfferingsManager(
        onError: ((PurchasesError) -> Unit)? = null,
        onSuccess: ((Offerings) -> Unit)? = null,
    ) {
-        offeringsCache.setOfferingsCacheTimestampToNow()


I don't recall exactly why this was here before 🤔 But I remember there was a reason

Brain dump:

Before your changes, if there was another request to fetch offerings, the request considers the cache to be recent and it would return that outdated offerings cache instead, but that's incorrect right? I think that's what you optimized.

Also, if there was an issue getting the offerings, we would clear that newly set timestamp, and the following request would retry, with your changes, the behavior is the same

Anyways, I think the optimization makes sense, I am just trying to remember why this was done that way lol

if there was another request to fetch offerings, the request considers the cache to be recent and it would return that outdated offerings cache instead, but that's incorrect right?

Mostly yes. However, the scenario I was trying to fix was the following:

User calls getOfferings BEFOREactivityonStart`. We mark timestamp of the cache to now before the requests starts

Activity onStart happens, however, the offerings cache already has a recent timestamp so we don't even try to get it (since it happens in the background, we don't expect a result, so we don't even check if there is a cached value or not).

We remain only with the request that had the jittering, rendering the main changes in this PR useless.

Also, if there was an issue getting the offerings, we would clear that newly set timestamp, and the following request would retry, with your changes, the behavior is the same

That's correct, and that remains that way for now, though it probably wouldn't be needed now. I've just made it so marking the offering cache timestamp happens only after a success.

The main consequence of this change is that multiple offerings request may happen before we receive a successful response, however, we rely on the request callback cache in the Backend to avoid duplicate requests.

We had a recent conversation about this on iOS as well. Using the "set timestamp to now" workaround is effective but definitely not intuitive and confusing for maintainers.

I think the ultimate solution to it is to have 2 values - "last updated" and "is currently updating", and use the combination of those 2 as a more expressive way of determining state. We're currently encoding both of those into a single boolean value.

The cache key came later, though, so maybe it removes the need for that workaround entirely? Does it de-duplicate requests with the same key? If so it effectively removes the need for the "set timestamp to now" workaround and even having an "is currently updating" value as well, right?

The cache key came later, though, so maybe it removes the need for that workaround entirely? Does it de-duplicate requests with the same key?

Yes, the callback cache mechanism deduplicates requests while the request "is currently updating". However, we still need to store the last time the cache was updated to know whether it's stale, so we still need to store the timestamp on a success.

We had a recent conversation about this on iOS as well.

Yes, iOS did a similar change not too long ago and it relies on the request cache as well. I think that makes the most sense to handle deduplicating requests while it's in progress.

aboedo · 2023-08-23T14:10:54Z

purchases/src/main/kotlin/com/revenuecat/purchases/common/Backend.kt

+        // In case we have a request with a jittered delay queued, and we perform the same request without
+        // jittered delay, we want to call the callback using the unjittered request
+        val backgroundedCacheKey = cacheKey.copy(appInBackground = true)
+        val backgroundCallAlreadyInPlace = containsKey(foregroundCacheKey)
+        if (!cacheKey.appInBackground && backgroundCallAlreadyInPlace) {
+            warnLog(NetworkStrings.SAME_CALL_SCHEDULED_WITH_JITTER.format(foregroundCacheKey))
+            remove(backgroundedCacheKey)?.takeIf { it.isNotEmpty() }?.let { backgroundedCallbacks ->
+                if (containsKey(cacheKey)) {
+                    this[cacheKey]?.addAll(backgroundedCallbacks)


aboedo · 2023-08-23T14:17:19Z

purchases/src/main/kotlin/com/revenuecat/purchases/common/offerings/OfferingsManager.kt

@@ -61,11 +61,12 @@ internal class OfferingsManager(
        onError: ((PurchasesError) -> Unit)? = null,
        onSuccess: ((Offerings) -> Unit)? = null,
    ) {
-        offeringsCache.setOfferingsCacheTimestampToNow()


We had a recent conversation about this on iOS as well. Using the "set timestamp to now" workaround is effective but definitely not intuitive and confusing for maintainers.

I think the ultimate solution to it is to have 2 values - "last updated" and "is currently updating", and use the combination of those 2 as a more expressive way of determining state. We're currently encoding both of those into a single boolean value.

The cache key came later, though, so maybe it removes the need for that workaround entirely? Does it de-duplicate requests with the same key? If so it effectively removes the need for the "set timestamp to now" workaround and even having an "is currently updating" value as well, right?

aboedo · 2023-08-23T14:18:30Z

purchases/src/main/kotlin/com/revenuecat/purchases/common/offerings/OfferingsManager.kt

@@ -94,6 +95,7 @@ internal class OfferingsManager(
            },
            onSuccess = { offerings ->
                offeringsCache.cacheOfferings(offerings, offeringsJSON)
+                offeringsCache.setOfferingsCacheTimestampToNow()


I wonder if these two lines should just be a part of cacheOfferings in offeringsCache and guarantee atomicity there

aboedo · 2023-08-23T14:18:58Z

purchases/src/main/kotlin/com/revenuecat/purchases/common/offerings/OfferingsManager.kt

@@ -94,6 +95,7 @@ internal class OfferingsManager(
            },
            onSuccess = { offerings ->
                offeringsCache.cacheOfferings(offerings, offeringsJSON)
+                offeringsCache.setOfferingsCacheTimestampToNow()


Feels like it'd be conceptually cleaner and slightly easier to make it synchronized?

aboedo · 2023-08-23T14:23:55Z

purchases/src/test/java/com/revenuecat/purchases/common/BackendTest.kt

@@ -335,6 +336,60 @@ class BackendTest {
        }
    }

+    @Test
+    fun `given getCustomerInfo call on foreground, then one in background, only one request without delay is triggered`() {


we have to do this because it'd be tricky to cancel the existing request for the background, right?

Well, in this case I think it makes sense to reuse the existing callback cache, since we would get the result earlier without the jittering.

On the opposite side though (first request in background, then one in foreground), ideally we would cancel the background request, but cancelling exising requests would be very tricky yeah. However, since I think this case shouldn't be that common, I think it's better to just leave the request even if it's duplicated.

### Description Unifies caching offerings and updating the offerings cache timestamp to simplify things, as suggested in #1204 (comment)

@tonidero

**This is an automatic release.** ### Performance Improvements * Optimize SDK initialization when requests executed before any activity starts (#1204) via Toni Rico (@tonidero) * Optimize diagnostics file management (#1194) via Toni Rico (@tonidero) ### Other Changes * Use real debug view dependencies in magic weather compose (#1203) via Toni Rico (@tonidero) --------- Co-authored-by: revenuecat-ops <ops@revenuecat.com> Co-authored-by: Toni Rico <antonio.rico.diez@revenuecat.com> Co-authored-by: Toni Rico <toni.rico.diez@revenuecat.com>

### Description These logs were added in #1204. These logs might happen primarily if the developer calls getCustomerInfo/getOfferings before any activity has started. In those situations, we have to handle it so the request does not wait for the whole jitter. There is nothing much the developer can do about this and it's not actually an error, so lowering the log level from warning to debug for these. Fixes RevenueCat/react-native-purchases#757

tonidero added 2 commits August 18, 2023 13:06

Call callbacks without jittering delay if posterior request does not …

a02e52f

…have jittering

Several optimizations to SDK initialization and support jittered call…

2cb7961

…s after non-jittered calls

tonidero added the perf A code change that improves performance label Aug 18, 2023

tonidero commented Aug 18, 2023

View reviewed changes

vegaro reviewed Aug 18, 2023

View reviewed changes

tonidero added 2 commits August 21, 2023 09:28

Some cleanup from review

6aa761f

Improve logs

c87689b

tonidero requested a review from vegaro August 21, 2023 07:47

tonidero added 2 commits August 21, 2023 10:10

Add tests to verify offerings cache timestamp behavior

6015d47

Add basic tests to verify behavior for background aware callbacks

088de84

tonidero marked this pull request as ready for review August 21, 2023 08:29

tonidero requested a review from a team August 21, 2023 08:29

vegaro approved these changes Aug 21, 2023

View reviewed changes

tonidero merged commit c149125 into main Aug 22, 2023
7 checks passed

tonidero deleted the remove-jitter-delay-callback-if-request-without-it branch August 22, 2023 07:15

aboedo reviewed Aug 23, 2023

View reviewed changes

tonidero mentioned this pull request Aug 23, 2023

Unify caching offerings and updating update timestamp #1206

Merged

RCGitBot mentioned this pull request Aug 23, 2023

[AUTOMATIC] Release/6.9.3 #1207

Merged

tonidero added a commit that referenced this pull request Aug 24, 2023

Unify caching offerings and updating update timestamp (#1206)

8300263

### Description Unifies caching offerings and updating the offerings cache timestamp to simplify things, as suggested in #1204 (comment)

tonidero mentioned this pull request Nov 9, 2023

Lower request jitter log level from warning to debug #1453

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize SDK initialization when requests executed before any activity starts #1204

Optimize SDK initialization when requests executed before any activity starts #1204

tonidero commented Aug 18, 2023 •

edited

Loading

tonidero left a comment

tonidero Aug 18, 2023

aboedo Aug 23, 2023

aboedo Aug 23, 2023

tonidero Aug 23, 2023

tonidero Aug 18, 2023

codecov bot commented Aug 18, 2023 •

edited

Loading

vegaro Aug 18, 2023

vegaro Aug 18, 2023

tonidero Aug 18, 2023

tonidero Aug 21, 2023

vegaro Aug 18, 2023 •

edited

Loading

tonidero Aug 18, 2023

tonidero Aug 21, 2023

vegaro Aug 18, 2023

tonidero Aug 21, 2023

aboedo Aug 23, 2023

tonidero Aug 23, 2023

aboedo Aug 23, 2023

aboedo Aug 23, 2023

aboedo Aug 23, 2023

aboedo Aug 23, 2023

aboedo Aug 23, 2023

tonidero Aug 23, 2023

Optimize SDK initialization when requests executed before any activity starts #1204

Optimize SDK initialization when requests executed before any activity starts #1204

Conversation

tonidero commented Aug 18, 2023 • edited Loading

Description

tonidero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 18, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegaro Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonidero commented Aug 18, 2023 •

edited

Loading

codecov bot commented Aug 18, 2023 •

edited

Loading

vegaro Aug 18, 2023 •

edited

Loading