[FIX] spreadsheet: improve perf by avoiding useless evaluation

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) <vsc@odoo.com>
Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>

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;
     }

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.<string, any>} */
         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<unknown>} 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");
     }
 

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!");

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();