From ad54be71bafe6dec99ced6793da13126bf56b0ea Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lucas=20Lef=C3=A8vre=20=28lul=29?= <lul@odoo.com>
Date: Fri, 27 Oct 2023 15:37:16 +0200
Subject: [PATCH] [IMP] history: reduce memory allocation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Backport of d8d943bb363638912b86d0684b52ea3a02c364c7

`addChange` is used *a lot*, like *a lot, a lot*, for every mutation in core
plugins. Its performance is key.

Currently `const [root, ...path] = args` creates a new array for `path`,
the array is allocated, and must be garbage collected later.

On the large formula dataset, with 20k rows (520k cells), and adding a column
before A, then reload the spreadsheet:

(average over 5 runs)

	minor GC	addChange
before	1609ms		1168ms
after	1094ms		753ms
	-32%		-35%

The memory usage also decreases: right after loading, memory was ~1725Mb and
~792Mb after running the GC. It's now ~1455Mb right after loading and ~670Mb
after running the GC. (I'm not sure why it goes from 792Mb to 670Mb. Is it
just thanks to the removed "root" key)

closes odoo/o-spreadsheet#3193

X-original-commit: 97ebec0ebe46296576e17d7abcecc5de65795816
Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
---
 src/history/factory.ts        |  6 +++---
 src/state_observer.ts         | 16 ++++++++--------
 src/types/history.ts          |  3 +--
 tests/plugins/history.test.ts |  2 ++
 4 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/src/history/factory.ts b/src/history/factory.ts
index f818546456..5d7fc252a2 100644
--- a/src/history/factory.ts
+++ b/src/history/factory.ts
@@ -59,9 +59,9 @@ function revertChanges(revisions: readonly Revision[]) {
  * Apply the changes of the given HistoryChange to the state
  */
 function applyChange(change: HistoryChange, target: "before" | "after") {
-  let val = change.root as any;
-  let key = change.path[change.path.length - 1];
-  for (let pathIndex = 0; pathIndex < change.path.slice(0, -1).length; pathIndex++) {
+  let val = change.path[0];
+  const key = change.path[change.path.length - 1];
+  for (let pathIndex = 1; pathIndex < change.path.slice(0, -1).length; pathIndex++) {
     const p = change.path[pathIndex];
     if (val[p] === undefined) {
       const nextPath = change.path[pathIndex + 1];
diff --git a/src/state_observer.ts b/src/state_observer.ts
index b049830de9..9eb86bf91d 100644
--- a/src/state_observer.ts
+++ b/src/state_observer.ts
@@ -20,15 +20,16 @@ export class StateObserver {
     this.commands.push(command);
   }
 
-  addChange(...args: any[]) {
+  addChange(...args: [...HistoryChange["path"], any]) {
     const val: any = args.pop();
-    const [root, ...path] = args as [any, string | number];
+    const root = args[0];
     let value = root as any;
-    let key = path[path.length - 1];
-    for (let pathIndex = 0; pathIndex <= path.length - 2; pathIndex++) {
-      const p = path[pathIndex];
+    let key = args[args.length - 1];
+    const pathLength = args.length - 2;
+    for (let pathIndex = 1; pathIndex <= pathLength; pathIndex++) {
+      const p = args[pathIndex];
       if (value[p] === undefined) {
-        const nextPath = path[pathIndex + 1];
+        const nextPath = args[pathIndex + 1];
         value[p] = createEmptyStructure(nextPath);
       }
       value = value[p];
@@ -37,8 +38,7 @@ export class StateObserver {
       return;
     }
     this.changes.push({
-      root,
-      path,
+      path: args,
       before: value[key],
       after: val,
     });
diff --git a/src/types/history.ts b/src/types/history.ts
index 591b7a0780..d66b235a6b 100644
--- a/src/types/history.ts
+++ b/src/types/history.ts
@@ -9,8 +9,7 @@ export interface CreateRevisionOptions {
 }
 
 export interface HistoryChange {
-  root: any;
-  path: (string | number)[];
+  path: [any, ...(number | string)[]];
   before: any;
   after: any;
 }
diff --git a/tests/plugins/history.test.ts b/tests/plugins/history.test.ts
index d85e98bc91..40f08a8609 100644
--- a/tests/plugins/history.test.ts
+++ b/tests/plugins/history.test.ts
@@ -150,6 +150,7 @@ describe("history", () => {
       A: {},
     };
     expect(() => {
+      // @ts-expect-error
       history.addChange(state, "A", "B", true, 5);
     }).toThrow();
   });
@@ -160,6 +161,7 @@ describe("history", () => {
       A: {},
     };
     expect(() => {
+      // @ts-expect-error
       history.addChange(state, "A", "B", true, "C", 5);
     }).toThrow();
   });