From fcd66ee3321649405cf21bc1d625d71abf3d5819 Mon Sep 17 00:00:00 2001 From: Zachary Hanham Date: Tue, 26 Mar 2024 17:08:40 -0400 Subject: [PATCH] [IMP] point_of_sale: improve OrderLine.findAttribute speed This commit addresses slow performance of the JS `OrderLine.findAttribute` method when `attributes_by_ptal_id` is exceedingly large. Before this commit, `findAttribute` would loop over all the values of `this.pos.attributes_by_ptal_id`, and filter out only the attributes that have at least one of the passed in ptal IDs (from the `values` parameter) within the attribute's "values" list. It would also modify the attribute to include a `"valuesForOrderLine"` key mapping to a list of all of the found ptal IDs from that attribute's values. This method becomes slow when `this.pos.attributes_by_ptal_id` is very large. Since it needs to loop over every single attribute. This commit provides a workaround to this slowness, by caching the search for valid attributes that this method performs in a lookup table. First, we construct the lookup table in `_processData` method of the `PosStore` class. `_add_ptal_ids_by_ptav_id` does this, by looping over all the values of all the attributes, mapping the ptav ID of the attribute to all the ptal IDs we find. Now, inside `findAttribute`, we will instead loop over each of the passed in `values`. For each value, we will retrieve the all the cached ptal IDs from `this.pos.ptal_ids_by_ptav_id` for the given value. Now that we have the ptal IDs, we can get all the corresponding attributes from `this.pos.attributes_by_ptal_id`. For each of those attributes, we can do the same modification from the original method by adding the `"valuesForOrderLine"` key/value. Finally we return all the modified attributes. This new method has a nested for loop, which may seem like a problem. But I believe that both the things being looped over (`values` and `this.pos.ptal_ids_by_ptav_id[value]`) should be very small compared to the potential size of `this.pos.attributes_by_ptal_id`. (43,000 in this customer's DB) `findAttributes` is called many times whenever the POS's numpad buttons are pressed, so this commit has the overall effect of drastically reducing the input latency for a numpad press. However, I do believe that this is a workaround, and the real problem is that the entire `attributes_by_ptal_id` is always passed to the POS, regardless of what products are actually being ordered. `attributes_by_ptal_id` should instead be incrementally fetched as products are ordered (if this is possible). Benchmarks (in customer DB): Before commit, each keypad press took around 1-3 seconds per Order Line present in the POS. For 11 products this was ~15 second latency. After commit, each keypad press is almost instant. The `ptal_ids_by_ptav_id` lookup table will consume additional memory, in this customer's DB I've estimated it to be about 0.3MB more memory. Taking memory snapshots in profiler shows no significant different between versions, however this is inconsistent. Will attach profiler results to PR. opw-3788840 closes odoo/odoo#159402 Signed-off-by: David Monnom (moda) --- .../static/src/app/store/models.js | 21 ++++++++----------- .../static/src/app/store/pos_store.js | 18 ++++++++++++++++ 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/addons/point_of_sale/static/src/app/store/models.js b/addons/point_of_sale/static/src/app/store/models.js index 878a9020eae..d7eed801921 100644 --- a/addons/point_of_sale/static/src/app/store/models.js +++ b/addons/point_of_sale/static/src/app/store/models.js @@ -1095,22 +1095,19 @@ export class Orderline extends PosModel { } findAttribute(values) { const listOfAttributes = []; - Object.values(this.pos.attributes_by_ptal_id).filter( - (attribute) => { + for (const value of values){ + for (const ptal_id of this.pos.ptal_ids_by_ptav_id[value]){ + const attribute = this.pos.attributes_by_ptal_id[ptal_id] const attFound = attribute.values.filter((target) => { return Object.values(values).includes(target.id); }); - if (attFound.length > 0) { - const modifiedAttribute = { - ...attribute, - valuesForOrderLine: attFound, - }; - listOfAttributes.push(modifiedAttribute); - return true; - } - return false; + const modifiedAttribute = { + ...attribute, + valuesForOrderLine: attFound, + }; + listOfAttributes.push(modifiedAttribute); } - ); + } return listOfAttributes; } getDisplayData() { diff --git a/addons/point_of_sale/static/src/app/store/pos_store.js b/addons/point_of_sale/static/src/app/store/pos_store.js index fd54e2d94a6..645cfbe4193 100644 --- a/addons/point_of_sale/static/src/app/store/pos_store.js +++ b/addons/point_of_sale/static/src/app/store/pos_store.js @@ -286,6 +286,7 @@ export class PosStore extends Reactive { this._loadProductProduct(loadedData["product.product"]); this.db.add_packagings(loadedData["product.packaging"]); this.attributes_by_ptal_id = loadedData["attributes_by_ptal_id"]; + this._add_ptal_ids_by_ptav_id(this.attributes_by_ptal_id); this.cash_rounding = loadedData["account.cash.rounding"]; this.payment_methods = loadedData["pos.payment.method"]; this._loadPosPaymentMethod(); @@ -299,6 +300,23 @@ export class PosStore extends Reactive { await this._loadPosPrinters(loadedData["pos.printer"]); this.open_orders_json = loadedData["open_orders"]; } + _add_ptal_ids_by_ptav_id(attributes_by_ptal_id) { + // Create a cache based on attributes_by_ptal_id + // that enables faster lookup for all ptal_ids + // inside a given attribute's "values" list, + // given a specific ptav_id. + this.ptal_ids_by_ptav_id = {}; + for (const [ptal_id, attribute] of Object.entries(attributes_by_ptal_id)) { + for (const value of attribute["values"]) { + const ptav_id = value["id"]; + if (ptav_id in this.ptal_ids_by_ptav_id) { + this.ptal_ids_by_ptav_id[ptav_id].push(ptal_id); + } else { + this.ptal_ids_by_ptav_id[ptav_id] = [ptal_id]; + } + } + } + } _loadPosSession() { // We need to do it here, since only then the local storage has the correct uuid this.db.save("pos_session_id", this.pos_session.id);