Skip to content

Conversation

@toupper
Copy link
Contributor

@toupper toupper commented Nov 4, 2025

Description

⚙️ Context

This Draft PR is not intended to be merged — it serves as a space for collaboration to find the best long-term solution.

During testing, we noticed that the orders cache was not displaying results immediately. After investigation, we found that mapping the order models from the backend to the corresponding view models in the UI was taking a considerable amount of time:

replaceOrders: buildItemsMap() took 5955ms for 25 orders

🔍 Findings

The main bottleneck was identified in WooPosFormatPrice, where site settings were being fetched on every single call.
After introducing currency-code caching, the mapping time improved significantly:

replaceOrders: buildItemsMap() took 2568ms for 25 orders

💡 Discussion

Caching the currency code introduces a potential downside — if the site settings change (for example, switching to another site), the cached value could become outdated.

However, since WooPosFormatPrice is not a singleton, it will be deallocated together with its parent object, so the cache should remain consistent within the object’s lifecycle.

An alternative could be to inject the currency code directly, though that comes with its own disadvantages (e.g., tighter coupling and less flexibility).

❓ Questions

@kidinov — what do you think about this caching approach?
Do you see other opportunities to reduce the mapping time based on our measurements?

@toupper toupper requested a review from kidinov November 4, 2025 11:25
@toupper toupper marked this pull request as draft November 4, 2025 11:25
@dangermattic
Copy link
Collaborator

1 Message
📖 This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App NameWooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit46cee61
Direct Downloadwoocommerce-wear-prototype-build-pr14892-46cee61.apk

@toupper toupper added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Nov 4, 2025
@wpmobilebot
Copy link
Collaborator

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App NameWooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit46cee61
Direct Downloadwoocommerce-prototype-build-pr14892-46cee61.apk

@wpmobilebot
Copy link
Collaborator

🤖 Test Failure Analysis

Your tests failed. Claude has analyzed the failures - check the annotation for details.

@kidinov
Copy link
Contributor

kidinov commented Nov 4, 2025

@toupper Thank you for your invistigation!

I think the fundamental issue here is that we calculate all the order details even if we may never show them. So maybe the simplest solution for now will be to perform the lazy calculation. Having said that, in-memory cache of the currency is something that we should do as well, so perhaps a ticket to a backlog? Another thing to remember is that with the local catalog, we are going to hit the DB for every order, so lazy calculation is needed anyway.

I also added mapping in parallel.

Wdyt?

Performance_optimisations.patch

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

Labels

status: do not merge Dependent on another PR, ready for review but not ready for merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants