From 64e475dc7bddd536ad9d439a62f9bd555d3554b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Lef=C3=A8vre=20=28lul=29?= Date: Wed, 17 Jan 2024 13:55:25 +0100 Subject: [PATCH] [FIX] spreadsheet: improve perf by avoiding useless evaluation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In a spreadsheet with multiple data sources (2 pivots), each data source initially loads and triggers a new evaluation upon loading. This results in two evaluations, even if both data sources resolve in less than 10ms apart. In such cases, the first re-evaluation becomes redundant, as a new one is immediately triggered. The issue is worse when more than 6 RPCs are required, as most browsers limit network calls to 6 in parallel. Consequently, the 7th RPC will unnecessarily wait after the evaluation triggered by the first RPC to resolve. For spreadsheets with many many data sources, the accumulation of these pointless evaluations significantly impacts performance. In a real-life scenario with 18 data sources from our production database, the spreadsheet took approximately ~33s to fully load and become reactive. With this commit, the loading time is reduced to ~7s (only one evaluation instead of 18) (tested in 17.0). Note that this testing was conducted locally, with minimal latency, and with a limited amount of data. One consequence of this commit is that cells won't load incrementally as each data source loads. Instead, all cells will display "Loading..." until all data sources are loaded. Given the substantial speed improvement, we consider this trade-off worthwhile. This fix only impacts loadable datasources (pivot, lists, graphs), it could also include data sources using individual RPCs (currency, accounting). Maybe for master. closes odoo/odoo#150015 X-original-commit: 70877d29cc2368298f8716f64c248a86ed0416ce Signed-off-by: Vincent Schippefilt (vsc) Signed-off-by: Lucas Lefèvre (lul) --- .../static/src/data_sources/data_source.js | 12 ++-- .../static/src/data_sources/data_sources.js | 33 ++++++++++ .../tests/data_fetching/data_source_test.js | 12 +++- .../tests/pivots/model/pivot_plugin_test.js | 64 +++++++++++++++++++ 4 files changed, 114 insertions(+), 7 deletions(-) diff --git a/addons/spreadsheet/static/src/data_sources/data_source.js b/addons/spreadsheet/static/src/data_sources/data_source.js index adc580f80d2..68dfe64175f 100644 --- a/addons/spreadsheet/static/src/data_sources/data_source.js +++ b/addons/spreadsheet/static/src/data_sources/data_source.js @@ -15,10 +15,11 @@ import { KeepLast } from "@web/core/utils/concurrency"; * particular data. */ export class LoadableDataSource { - constructor(services) { - this._orm = services.orm; - this._metadataRepository = services.metadataRepository; - this._notify = services.notify; + constructor(params) { + this._orm = params.orm; + this._metadataRepository = params.metadataRepository; + this._notifyWhenPromiseResolves = params.notifyWhenPromiseResolves; + this._cancelPromise = params.cancelPromise; /** * Last time that this dataSource has been updated @@ -44,6 +45,7 @@ export class LoadableDataSource { */ async load(params) { if (params && params.reload) { + this._cancelPromise(this._loadPromise); this._loadPromise = undefined; } if (!this._loadPromise) { @@ -59,8 +61,8 @@ export class LoadableDataSource { .finally(() => { this._lastUpdate = Date.now(); this._isFullyLoaded = true; - this._notify(); }); + await this._notifyWhenPromiseResolves(this._loadPromise); } return this._loadPromise; } diff --git a/addons/spreadsheet/static/src/data_sources/data_sources.js b/addons/spreadsheet/static/src/data_sources/data_sources.js index a98de615c12..b9d0633adb9 100644 --- a/addons/spreadsheet/static/src/data_sources/data_sources.js +++ b/addons/spreadsheet/static/src/data_sources/data_sources.js @@ -25,6 +25,7 @@ export class DataSources extends EventBus { this._metadataRepository.addEventListener("labels-fetched", () => this.notify()); /** @type {Object.} */ this._dataSources = {}; + this.pendingPromises = new Set(); } /** @@ -41,6 +42,8 @@ export class DataSources extends EventBus { orm: this._orm, metadataRepository: this._metadataRepository, notify: () => this.notify(), + notifyWhenPromiseResolves: this.notifyWhenPromiseResolves.bind(this), + cancelPromise: (promise) => this.pendingPromises.delete(promise), }, params ); @@ -89,11 +92,41 @@ export class DataSources extends EventBus { return id in this._dataSources; } + /** + * @private + * @param {Promise} promise + */ + async notifyWhenPromiseResolves(promise) { + this.pendingPromises.add(promise); + await promise + .then(() => { + this.pendingPromises.delete(promise); + this.notify(); + }) + .catch(() => { + this.pendingPromises.delete(promise); + this.notify(); + }); + } + /** * Notify that a data source has been updated. Could be useful to * request a re-evaluation. */ notify() { + if (this.pendingPromises.size) { + if (!this.nextTriggerTimeOutId) { + // evaluates at least every 10 seconds, even if there are pending promises + // to avoid blocking everything if there is a really long request + this.nextTriggerTimeOutId = setTimeout(() => { + this.nextTriggerTimeOutId = undefined; + if (this.pendingPromises.size) { + this.trigger("data-source-updated"); + } + }, 10000); + } + return; + } this.trigger("data-source-updated"); } diff --git a/addons/spreadsheet/static/tests/data_fetching/data_source_test.js b/addons/spreadsheet/static/tests/data_fetching/data_source_test.js index 9e44f3babf2..32adb9e8af8 100644 --- a/addons/spreadsheet/static/tests/data_fetching/data_source_test.js +++ b/addons/spreadsheet/static/tests/data_fetching/data_source_test.js @@ -31,17 +31,22 @@ QUnit.module("spreadsheet data source", {}, () => { } } const dataSource = new TestDataSource({ - notify: () => {}, + notify: () => assert.step("notify"), + notifyWhenPromiseResolves: () => assert.step("notify-from-promise"), + cancelPromise: () => assert.step("cancel-promise"), }); dataSource.load(); + assert.verifySteps(["notify-from-promise"]); dataSource.load({ reload: true }); assert.strictEqual(dataSource.isReady(), false); def1.resolve(); await nextTick(); + assert.verifySteps(["cancel-promise", "notify-from-promise"]); assert.strictEqual(dataSource.isReady(), false); def2.resolve(); await nextTick(); assert.strictEqual(dataSource.isReady(), true); + assert.verifySteps([]); } ); @@ -57,7 +62,9 @@ QUnit.module("spreadsheet data source", {}, () => { } const dataSource = new TestDataSource({ - notify: () => {}, + notify: () => assert.step("notify"), + notifyWhenPromiseResolves: () => assert.step("notify-from-promise"), + cancelPromise: () => assert.step("cancel-promise"), orm: { call: () => { throw makeServerError({ description: "Ya done!" }); @@ -65,6 +72,7 @@ QUnit.module("spreadsheet data source", {}, () => { }, }); await dataSource.load(); + assert.verifySteps(["notify-from-promise"]); assert.ok(dataSource._isFullyLoaded); assert.notOk(dataSource._isValid); assert.equal(dataSource._loadErrorMessage, "Ya done!"); diff --git a/addons/spreadsheet/static/tests/pivots/model/pivot_plugin_test.js b/addons/spreadsheet/static/tests/pivots/model/pivot_plugin_test.js index c483f664d44..a53534711ae 100644 --- a/addons/spreadsheet/static/tests/pivots/model/pivot_plugin_test.js +++ b/addons/spreadsheet/static/tests/pivots/model/pivot_plugin_test.js @@ -388,6 +388,70 @@ QUnit.module("spreadsheet > pivot plugin", {}, () => { assert.equal(getCellValue(model, "A1"), 131); }); + QUnit.test("evaluates only once when two pivots are loading", async function (assert) { + const spreadsheetData = { + sheets: [{ id: "sheet1" }], + pivots: { + 1: { + id: 1, + colGroupBys: ["foo"], + domain: [], + measures: [{ field: "probability", operator: "avg" }], + model: "partner", + rowGroupBys: ["bar"], + }, + 2: { + id: 2, + colGroupBys: ["foo"], + domain: [], + measures: [{ field: "probability", operator: "avg" }], + model: "partner", + rowGroupBys: ["bar"], + }, + }, + }; + const model = await createModelWithDataSource({ + spreadsheetData, + }); + model.config.custom.dataSources.addEventListener("data-source-updated", () => + assert.step("data-source-notified") + ); + setCellContent(model, "A1", '=ODOO.PIVOT("1", "probability")'); + setCellContent(model, "A2", '=ODOO.PIVOT("2", "probability")'); + assert.equal(getCellValue(model, "A1"), "Loading..."); + assert.equal(getCellValue(model, "A2"), "Loading..."); + await nextTick(); + assert.equal(getCellValue(model, "A1"), 131); + assert.equal(getCellValue(model, "A2"), 131); + assert.verifySteps(["data-source-notified"], "evaluation after both pivots are loaded"); + }); + + QUnit.test("concurrently load the same pivot twice", async function (assert) { + const spreadsheetData = { + sheets: [{ id: "sheet1" }], + pivots: { + 1: { + id: 1, + colGroupBys: ["foo"], + domain: [], + measures: [{ field: "probability", operator: "avg" }], + model: "partner", + rowGroupBys: ["bar"], + }, + }, + }; + const model = await createModelWithDataSource({ + spreadsheetData, + }); + // the data loads first here, when we insert the first pivot function + setCellContent(model, "A1", '=ODOO.PIVOT("1", "probability")'); + assert.equal(getCellValue(model, "A1"), "Loading..."); + // concurrently reload the same pivot + model.dispatch("REFRESH_PIVOT", { id: 1 }); + await nextTick(); + assert.equal(getCellValue(model, "A1"), 131); + }); + QUnit.test("display loading while data is not fully available", async function (assert) { const metadataPromise = makeDeferred(); const dataPromise = makeDeferred();