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

Perf: update CustomerInfo cache before anything else #2865

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Jul 24, 2023

I noticed the order of cache updates on SDK initialization was:

  • Offerings
  • ProductEntitlementMapping
  • CustomerInfo

This is inefficient because fetching CustomerInfo first is the most important. Also, #2860 will pre-warm intro eligibility, so we want to do that after we might have potentially cleared the intro eligibility cache after a CustomerInfo update.
Example log from before:

[eligibility] DEBUG: ℹ️ Warming up intro eligibility cache for packages in paywall: [PackageType.$rc_monthly, PackageType.$rc_annual]
[customer] DEBUG: ℹ️ Sending latest CustomerInfo to delegate.
[customer] DEBUG: 😻 CustomerInfo updated from network.
[eligibility] DEBUG: ℹ️ Detected active subscriptions changed. Clearing trial or intro eligibility cache.

The new order is:

  • CustomerInfo
  • ProductEntitlementMapping
  • Offerings

Note: I recommend reviewing the diff ignoring whitespace.

@NachoSoto NachoSoto added the perf label Jul 24, 2023
@NachoSoto NachoSoto requested a review from a team July 24, 2023 09:27
@@ -1604,6 +1599,12 @@ private extension Purchases {
self.delegate?.purchases?(self, receivedUpdated: info)
}

private func updateOfferingsCache(isAppBackgrounded: Bool) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also extracted this into a method because it's going to grow more in #2860.

@@ -26,6 +26,14 @@ class PurchasesGetOfferingsTests: BasePurchasesTests {
expect(self.mockOfferingsManager.invokedUpdateOfferingsCacheCount).toEventually(equal(1))
}

func testFirstInitializationGetsOfferingsIfAppActiveInCustomEntitlementComputation() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

⚠️ It was important to add this test because a naive re-ordering of updateAllCachesIfNeeded would have meant that this would have now been skipped.

I noticed the order of cache updates on SDK initialization was:
- `Offerings`
- `ProductEntitlementMapping`
- `CustomerInfo`

This is inefficient because fetching `CustomerInfo` first is the most important. Also, #2860 will pre-warm intro eligibility, so we want to do that _after_ we might have potentially cleared the intro eligibility cache after a `CustomerInfo` update.

_ Note: I recommend reviewing the diff ignoring whitespace._
Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

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

Changes look good to me... With the jittering, I imagine these requests can still happen in different order, but I think that's ok. Leaving others to review first though.

Another question I have. To calculate intro-eligibility, don't we need offerings first? So we can know what products to use to calculate intro eligibility.

@NachoSoto
Copy link
Contributor Author

With the jittering, I imagine these requests can still happen in different order, but I think that's ok. Leaving others to review first though.

We only do jitter if updating caches in the background. This method I'm modifying is called when launching the app and it's in the foreground, so the order will be deterministic.

To calculate intro-eligibility, don't we need offerings first? So we can know what products to use to calculate intro eligibility.

Yeah that's why pre-warming that is done last, after fetching offerings. Without this change, that might have been done before updating the CustomerInfo. When that was done (at the end), it could have invalidated the eligibility cache we had just pre-warmed.

@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Attention: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.57%. Comparing base (5ce59d7) to head (a61f663).
Report is 885 commits behind head on main.

Files with missing lines Patch % Lines
Sources/Purchasing/Purchases/Purchases.swift 94.44% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2865      +/-   ##
==========================================
+ Coverage   86.48%   86.57%   +0.09%     
==========================================
  Files         217      217              
  Lines       15513    15511       -2     
==========================================
+ Hits        13416    13429      +13     
+ Misses       2097     2082      -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NachoSoto NachoSoto requested a review from aboedo July 24, 2023 13:13
@NachoSoto NachoSoto changed the title Caching: update CustomerInfo cache before anything else Perf: update CustomerInfo cache before anything else Jul 24, 2023
@NachoSoto NachoSoto merged commit 6c5bed7 into main Jul 24, 2023
2 checks passed
@NachoSoto NachoSoto deleted the customer-info-cache-first branch July 24, 2023 22:06
NachoSoto added a commit that referenced this pull request Jul 26, 2023
**This is an automatic release.**

_This release is compatible with Xcode 15 beta 5 and visionOS beta 2_

### Bugfixes
* `xrOS`: fixed `SubscriptionStoreView` for visionOS beta 2 (#2884) via
Josh Holtz (@joshdholtz)
### Performance Improvements
* `Perf`: update `CustomerInfo` cache before anything else (#2865) via
NachoSoto (@NachoSoto)
### Other Changes
* `SimpleApp`: added support for localization (#2880) via NachoSoto
(@NachoSoto)
* `TestStoreProduct`: made available on release builds (#2861) via
NachoSoto (@NachoSoto)
* `Tests`: increased default logger capacity (#2870) via NachoSoto
(@NachoSoto)
* `CustomEntitlementComputation`: removed `invalidateCustomerInfoCache`
(#2866) via NachoSoto (@NachoSoto)
* `SimpleApp`: updates for TestFlight compatibility (#2862) via
NachoSoto (@NachoSoto)
* `BasePurchasesTests`: consolidate to only initialize one `DeviceCache`
(#2863) via NachoSoto (@NachoSoto)
* `Codable`: debug log entire JSON when decoding fails (#2864) via
NachoSoto (@NachoSoto)
* `IntegrationTests`: replaced `Purchases.shared` with a `throw`ing
property (#2867) via NachoSoto (@NachoSoto)
* `NetworkError`: 2 new tests to ensure underlying error is included in
description (#2843) via NachoSoto (@NachoSoto)
* Add SPM `Package.resolved` for Xcode Cloud (#2844) via NachoSoto
(@NachoSoto)
* `CustomEntitlementComputation`: added integration test for
cancellations (#2849) via NachoSoto (@NachoSoto)
* `CustomEntitlementComputation`: removed
`syncPurchases`/`restorePurchases` (#2854) via NachoSoto (@NachoSoto)

---------

Co-authored-by: NachoSoto <[email protected]>
@vegaro vegaro added pr:other and removed pr:perf labels Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants