Skip to content

Commit

Permalink
[IMP] history: reduce memory allocation
Browse files Browse the repository at this point in the history
Backport of d8d943b

`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 #3193

X-original-commit: 97ebec0
Signed-off-by: Rémi Rahir (rar) <[email protected]>
Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
  • Loading branch information
LucasLefevre committed Nov 17, 2023
1 parent e73505e commit ad54be7
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 13 deletions.
6 changes: 3 additions & 3 deletions src/history/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
16 changes: 8 additions & 8 deletions src/state_observer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand All @@ -37,8 +38,7 @@ export class StateObserver {
return;
}
this.changes.push({
root,
path,
path: args,
before: value[key],
after: val,
});
Expand Down
3 changes: 1 addition & 2 deletions src/types/history.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,7 @@ export interface CreateRevisionOptions {
}

export interface HistoryChange {
root: any;
path: (string | number)[];
path: [any, ...(number | string)[]];
before: any;
after: any;
}
Expand Down
2 changes: 2 additions & 0 deletions tests/plugins/history.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ describe("history", () => {
A: {},
};
expect(() => {
// @ts-expect-error
history.addChange(state, "A", "B", true, 5);
}).toThrow();
});
Expand All @@ -160,6 +161,7 @@ describe("history", () => {
A: {},
};
expect(() => {
// @ts-expect-error
history.addChange(state, "A", "B", true, "C", 5);
}).toThrow();
});
Expand Down

0 comments on commit ad54be7

Please sign in to comment.