Optimize price formatter by Juanpe · Pull Request #1374 · RevenueCat/purchases-ios

Juanpe · 2022-03-10T13:15:38Z

Resolves #1033

Description

The goal of this PR is to improve the performance when the price of the products are formatted. The solution consists of creating a provider that creates a NumberFormatter the first time a product needs a formatter and caches it. Following times the formatter is reused.

To be prepared for storefront changes, the currency code and locale are set each time the price formatter is used but only if the values have changed.

aboedo

This looks amazing @Juanpe!! Thanks for taking care of it!

aboedo · 2022-03-10T13:25:26Z

+				576C8A8A27CFCB150058FA6E /* AnyEncodable.swift */,
 				57A0FBEF2749C0C2009E2FC3 /* Atomic.swift */,
+				352B7D7827BD919B002A47DD /* DangerousSettings.swift */,
 				37E3567189CF6A746EE3CCC2 /* DateExtensions.swift */,
 				0313FD40268A506400168386 /* DateProvider.swift */,
+				5796A3A827D7C43500653165 /* Deprecations.swift */,
 				2D22BF6426F3CB31001AE2F9 /* FatalErrorUtil.swift */,
 				B33CEA9F268CDCC9008A3144 /* ISOPeriodFormatter.swift */,
 				57EAE526274324C60060EB74 /* Lock.swift */,
 				F530E4FE275646EF001AF6BD /* MacDevice.swift */,
-				5796A3A827D7C43500653165 /* Deprecations.swift */,
 				57EAE52A274332830060EB74 /* Obsoletions.swift */,
 				2DDA3E4624DB0B5400EDFE5B /* OperationDispatcher.swift */,
+				2CB8CF9227BF538F00C34DE3 /* PlatformInfo.swift */,
+				F5FCD3E927DA0D0B003BDC04 /* PriceFormatterProvider.swift */,
 				2DD9F4BD274EADC20031AE2C /* Purchases+async.swift */,
 				57EAE52C274468900060EB74 /* RawDataContainer.swift */,
 				57A0FBF12749CF66009E2FC3 /* SynchronizedUserDefaults.swift */,
 				B3AA6237268B926F00894871 /* SystemInfo.swift */,


thanks for alphabetizing this!

aboedo · 2022-03-10T13:25:49Z

+    func testSk2PriceFormatterUsesCurrentStorefront() async throws {
+        try AvailabilityChecks.iOS15APIAvailableOrSkipTest()
+
+        testSession.locale = Locale(identifier: "es_ES")
+        await changeStorefront("ESP")
+
+        var storeProduct = try await fetchSk2StoreProduct()
+
+        var priceFormatter = try XCTUnwrap(storeProduct.priceFormatter)
+        expect(priceFormatter.currencyCode) == "EUR"
+
+        testSession.locale = Locale(identifier: "en_EN")
+        await changeStorefront("USA")
+
+        storeProduct = try await fetchSk2StoreProduct()
+
+        priceFormatter = try XCTUnwrap(storeProduct.priceFormatter)
+        expect(priceFormatter.currencyCode) == "USD"
+    }


aboedo · 2022-03-10T13:28:15Z

+    }
+
+    @available(iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0, *)
+    func testSk2PriceFormatterUsesCurrentStorefront() async throws {


is there a technical limitation to doing a similar test for SK1?

Oops, I didn't see this comment and I put this comment: #1374 (comment)

Juanpe · 2022-03-10T13:54:06Z

+    /// Updates `SKTestSession.storefront` and waits for `Storefront.current` to reflect the change
+    /// This is necessary because the change is aynchronous within `StoreKit`, and otherwise code that depends
+    /// on the change might not see it in time, resulting in race conditions and flaky tests.
+    func changeStorefront(_ new: String) async {


This method belonged to StoreKitTests class, but I moved it to StoreKitConfigTestCase because I needed it for one of my test case. Now, any test of StoreKit can change the Storefront.

Juanpe · 2022-03-10T13:57:09Z

+    }
+
+    @available(iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0, *)
+    func testSk2PriceFormatterUsesCurrentStorefront() async throws {


I only created a test for SK2 because I saw a comment that said:

ProductsFetcherSK1 does not detect Storefront changes to invalidate the cache like ProductsFetcherSK2 does.

Yeah, that's true, I forgot about that. The fetcher doesn't detect Storefront changes. We should address that (as a separate thing).

But independently of that, if the PriceFormatterProvider is passed a new locale, it does update the formatter, right? So I think we should test that.

That way when we do add automatic updates to the fetcher, we'll know that the PriceFormatterProvider will respond correctly as long as the test passes

Yep, totally agree. We could create a new fetcher after the Storefront has changed and the PriceFormatterProvider should respond correctly. Let me change it.

Thanks!

I've already created the test case to cover Storefront changes for SK1, you can check it out here 🙂

Juanpe · 2022-03-10T14:32:02Z

This looks amazing @Juanpe!! Thanks for taking care of it!

Thank @aboedo for the review and the feedback 🤙

NachoSoto · 2022-03-10T16:03:00Z

+    @available(iOS 15.0, tvOS 15.0, watchOS 8.0, macOS 12.0, *)
+    func priceFormatterForSK2(withCurrencyCode currencyCode: String) -> NumberFormatter {
+        if cachedPriceFormatterForSK2.currencyCode != currencyCode {
+            cachedPriceFormatterForSK2.currencyCode = currencyCode


This would be changing the currency under the rug of every previously returned formatter. I haven't been following the discussion but I feel like this isn't a desired behavior 😬

NSFormatter conforms to NSCopying, we could store a copy instead of the currency changes?

I see your point 🤔

Well, with this approach each product has its own priceFormatterProvider. Therefore, it's true if the currency code is changed, the next time priceFormatter is called, the currency code will be updated, and all previously returned formatters will be updated as well.

My question is if there is a possibility that there are two instances of the same product, one obtained before the currency code changes and another after and we want to show the price using both currency codes, do you know what I mean?

NSFormatter conforms to NSCopying, we could store a copy instead of the currency changes?

About that, would you store a formatter for each currency code?

This would be changing the currency under the rug of every previously returned formatter. I haven't been following the discussion but I feel like this isn't a desired behavior 😬

Yeah, I hadn't thought of that... You could have an autoUpdatingFormatter or just have it in the docs, but things get tricky:

In practice, if the storefront changes, you do want the prices to use the new currency, otherwise you'd be presenting potentially really problematic values.

Furthermore, you actually want to entirely drop the old products, because you'll run into consistency problems otherwise:

For example, say that your storefront is USA and you have a product priced at $9.99.
But you're also selling to Spain, where you're selling it a bit cheaper, at €6.99.

a) If the storefront updates but the formatter does not, and the old product is still around, the old product stays at price $9.99 when formatted, which is good.
b) If the storefront updates and the formatter does too, you get €9.99 for the old product, which is incorrect. Then again, the old product is no longer valid in the first place, since when you try to sell it to the customer it'll be priced €6.99, not $9.99.

So... to truly provide a foolproof experience, we'd have to:

bridge over the storefront changed notification so customers could subscribe to it

listen to it and clear all product caches when it fires

for bonus points, we could try eagerly re-fetch all the same products we'd cached before, since they're likely (although not guaranteed) to be used in the new storefront too.

If we don't do 1 and 2 we'll always have a partially broken experience when the storefront changes, we have to kind of pick which broken experience. It does seem like a) is a slightly less broken experience in that at least you'd have outdated products that are internally consistent, so I think we should go with that for now, but we still have to know that it's not an ideal case.

Storefronts aren't updated often, and chances of it happening in the lifecycle of an app are very low, so I wouldn't call it a huge priority to get this perfect.

In practice, if the storefront changes, you do want the prices to use the new currency, otherwise you'd be presenting potentially really problematic values.

Yes, this is exactly what I thought.

My question is whether this is the responsibility of the library or the users 🤔 If it's the library, it's something that should be clearly stated in the documentation as expected behaviour, otherwise it could be confusing.

Opened up this one for it #1382

But the way this is solved in this PR requires calling the method one more time, which is more of an accident than the framework explicitly listening to and reacting to changes.

So...the idea is to return a copy when the currency code is different. My question is, in this case, we should replace the cached formatter with the new copy, right?

Yeah exactly, store and return the new copy. So basically this is caching formatters as long as the code doesn't change, and if it does, we have a small performance penalty but only initially.

Ok, as we discussed I've modified the logic. Now, we'll store and return a copy, if the currency code is different :)

Here is the commit: d9dda34

NachoSoto

I may be misunderstanding it, but I think my comment about modifying previously returned formatters is a deal breaker. I would solve it by copying on demand when the currency changes.

NachoSoto · 2022-03-10T21:01:56Z

-    init(sk1Product: SK1Product) {
+    init(
+        sk1Product: SK1Product,
+        priceFormatterProvider: PriceFormatterProvider = .init()


This isn't being overridden, so it doesn't need to be a parameter of the constructor right?

yeah, we could initialize the provider when the property is defined 👍🏼

NachoSoto · 2022-03-10T21:02:35Z

-    init?(sk1Discount: SK1ProductDiscount) {
+    init?(
+        sk1Discount: SK1ProductDiscount,
+        priceFormatterProvider: PriceFormatterProvider = .init()


NachoSoto · 2022-03-10T21:02:44Z

-    init(sk2Product: SK2Product) {
+    init(
+        sk2Product: SK2Product,
+        priceFormatterProvider: PriceFormatterProvider = .init()


NachoSoto · 2022-03-10T21:02:59Z

+            Logger.appleError("Can't initialize priceFormatter for SK2 product! Could not find the currency code")
+            return nil
+        }
+        return priceFormatterProvider.priceFormatterForSK2(withCurrencyCode: currencyCode)


I love that at the very least all this code is now abstracted out of these 3 types!

NachoSoto · 2022-03-10T21:05:00Z

+
+}
+
+enum StoreKitTestError: Swift.Error {


Can you put this inside of StoreKitConfigTestCase to avoid putting it in the global namespace? It could get confusing with SKTestError.

Also this could be fileprivate.

yep, much better 👍🏼

…ptimize_priceFormatter

Juanpe · 2022-03-11T10:40:24Z

I may be misunderstanding it, but I think my comment about modifying previously returned formatters is a deal breaker. I would solve it by copying on demand when the currency changes.

Thanks for the review and the feedback 👍🏼

I put a comment. WDYT? 🤔

aboedo · 2022-03-21T14:10:51Z

LGTM. We need to fix conflicts with the changes from #1402. That PR just moves all tests to Tests/, so we'd have to update:

StoreKitUnitTests/StoreProductTests.swift -> Tests/StoreKitUnitTests/StoreProductTests.swift
StoreKitUnitTests/TestHelpers/StoreKitTestHelpers.swift -> Tests/StoreKitUnitTests/TestHelpers/StoreKitTestHelpers.swift

Juanpe · 2022-03-21T17:45:31Z

@NachoSoto @Juanpe is there a significant performance improvement in copying vs just creating a new one whenever locale changes? I'm asking because if we just did a new one when locale updates (which shouldn't happen often anyway), I feel like this code could get simplified a ton, where it'd basically be

if cachedFormatter == nil || locale is different cached formatter = new formatter with locale return cached formatter

(FWIW my suggestion isn't needed for this PR, since the code here works and is already much more efficient than what we have right now)

Yes, the performance is better by copying than by creating a new one, but the difference is not very big:

-----------Tests------------
Test 1 (new one): 107.9789400100708ms
Test 2 (copy): 90.35897254943848ms
Test 3 (cache): 39.42000865936279ms
----------------------------

So I agree with you that we can assume this worse performance to make the code more readable 👍🏼

# Conflicts: # Tests/StoreKitUnitTests/PriceFormatterProviderTests.swift

Juanpe · 2022-03-21T17:54:57Z

I made the changes 💃🏼 I think I don't miss anything 🤔

NachoSoto

Awesome 🎉

NachoSoto · 2022-03-24T16:32:16Z

@aboedo you're good to merge this?

aboedo · 2022-03-24T19:41:10Z

@NachoSoto yep!! Merging now. @Juanpe thanks so much for taking care of this!! 🏆

Juanpe added 3 commits March 10, 2022 12:44

Created PriceFormatterProvider

2482266

Used provider to get the cached price formatter

088481d

Created tests for PriceFormatterProvider

dca644d

aboedo approved these changes Mar 10, 2022

View reviewed changes

Juanpe commented Mar 10, 2022

View reviewed changes

Create a new test case to cover Storefront changes

ffa4d60

NachoSoto reviewed Mar 10, 2022

View reviewed changes

NachoSoto suggested changes Mar 10, 2022

View reviewed changes

Juanpe added 2 commits March 11, 2022 10:39

Merge branch 'main' of https://github.com/Juanpe/purchases-ios into o…

bb162ff

…ptimize_priceFormatter

Changes after CR comments

46ff166

Juanpe requested a review from NachoSoto March 11, 2022 16:18

aboedo mentioned this pull request Mar 11, 2022

Respond to Storefront updates #1382

Closed

Juanpe added 2 commits March 19, 2022 11:14

Merge remote-tracking branch 'origin/main' into optimize_priceFormatter

f9237d3

Update logic of how store and return price formatters

d9dda34

Juanpe added 2 commits March 21, 2022 18:47

Merge branch 'main' into optimize_priceFormatter

45a7af5

# Conflicts: # Tests/StoreKitUnitTests/PriceFormatterProviderTests.swift

Create a new price formatter if the currency code or locale is different

67defac

NachoSoto approved these changes Mar 21, 2022

View reviewed changes

aboedo merged commit 27e49f9 into RevenueCat:main Mar 24, 2022

Uh oh!

Conversation

Juanpe commented Mar 10, 2022

Description

Uh oh!

aboedo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Juanpe commented Mar 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Juanpe Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NachoSoto Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NachoSoto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Juanpe commented Mar 11, 2022

Uh oh!

aboedo commented Mar 21, 2022

Uh oh!

Juanpe commented Mar 21, 2022

Uh oh!

Juanpe commented Mar 21, 2022

Uh oh!

NachoSoto left a comment

Choose a reason for hiding this comment

Juanpe Mar 11, 2022 •

edited

Loading

NachoSoto Mar 18, 2022 •

edited

Loading

NachoSoto left a comment •

edited

Loading