Skip to content

Commit 6a1a64e

Browse files
committed
[FIX] stores: bye bye reactivity 👋
Reactivity is very very (very) slow, with potential overhead exceeding 90%. This commit completely revisit the way store updates trigger app rendering. Previously, this process relied entirely on Owl's reactivity system. With a reactive store, every `this.something` call traverses the reactive proxy, incurring significant cost, regardless of whether `something` was a method, a property, called publicly or privately. Instead, it introduces a new approach where store updates are triggered by explicitly declared public methods (mutators) Since stores follow the Command-Query Separation (CQS) principle, all public methods strictly mutate state in a store, they cannot have any other effect since they cannot return anything. The new approach also relies on proxies to track mutators calls, but the proxy is only on the surface. A mutator is usually only called once in the component. A property is also usually accessed only once for rendering. The internal working of the store is not affected and can run at full speed without any overhead. Note the loss of fined-grained rendering. However, as observed, bypassing reactivity might result in faster overall app rendering compared to relying on reactivity for fine-grained rendering, even if it means rendering the entire app rather than a few components. Reactivity had another unexpected error-prone issue: since everything was proxied, we could never compare objects references without using `toRaw` to retrieve the underlying un-proxied object. It was very error prone and added noise in the code. On the large number data set, open the find & replace panel and search for "1" It currently takes 1080ms (search + rendering) With this commit, that time is reduced to just 111ms, nearly a tenfold improvement! closes #4244 Task: 3829125 X-original-commit: 894216f Signed-off-by: Rémi Rahir (rar) <[email protected]> Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
1 parent d43a3bf commit 6a1a64e

28 files changed

+204
-97
lines changed

‎src/components/composer/autocomplete_dropdown/autocomplete_dropdown_store.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { AutoCompleteProposal, AutoCompleteProvider } from "../../../registries"
22
import { SpreadsheetStore } from "../../../stores";
33

44
export class AutoCompleteStore extends SpreadsheetStore {
5+
mutators = ["useProvider", "moveSelection", "hide", "selectIndex"] as const;
56
selectedIndex: number | undefined = undefined;
67
provider: AutoCompleteProvider | undefined;
78

‎src/components/composer/composer/composer.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -680,7 +680,9 @@ export class Composer extends Component<ComposerProps, SpreadsheetChildEnv> {
680680
*/
681681
private processTokenAtCursor(): void {
682682
let content = this.composerStore.currentContent;
683-
this.autoCompleteState.hide();
683+
if (this.autoCompleteState.provider) {
684+
this.autoCompleteState.hide();
685+
}
684686
this.functionDescriptionState.showDescription = false;
685687
const autoCompleteProvider = this.composerStore.autocompleteProvider;
686688
if (autoCompleteProvider) {

‎src/components/composer/composer/composer_store.ts

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import { toRaw } from "@odoo/owl";
21
import { EnrichedToken, composerTokenize } from "../../../formulas/composer_tokenizer";
32
import { POSTFIX_UNARY_OPERATORS } from "../../../formulas/tokenizer";
43
import { parseLiteral } from "../../../helpers/cells";
@@ -68,6 +67,16 @@ export interface ComposerSelection {
6867
}
6968

7069
export class ComposerStore extends SpreadsheetStore {
70+
mutators = [
71+
"startEdition",
72+
"setCurrentContent",
73+
"stopEdition",
74+
"stopComposerRangeSelection",
75+
"cancelEdition",
76+
"cycleReferences",
77+
"changeComposerCursorSelection",
78+
"replaceComposerCursorSelection",
79+
] as const;
7180
private col: HeaderIndex = 0;
7281
private row: HeaderIndex = 0;
7382
editionMode: EditionMode = "inactive";
@@ -84,9 +93,9 @@ export class ComposerStore extends SpreadsheetStore {
8493

8594
constructor(get: Get) {
8695
super(get);
87-
this.highlightStore.register(toRaw(this));
96+
this.highlightStore.register(this);
8897
this.onDispose(() => {
89-
this.highlightStore.unRegister(toRaw(this));
98+
this.highlightStore.unRegister(this);
9099
});
91100
}
92101

@@ -229,7 +238,7 @@ export class ComposerStore extends SpreadsheetStore {
229238
if (this.isSelectingRange) {
230239
this.editionMode = "editing";
231240
}
232-
this.model.selection.resetAnchor(toRaw(this), {
241+
this.model.selection.resetAnchor(this, {
233242
cell: { col: left, row: top },
234243
zone: cmd.zone,
235244
});
@@ -247,7 +256,7 @@ export class ComposerStore extends SpreadsheetStore {
247256
row: activePosition.row,
248257
});
249258
const zone = this.getters.expandZone(cmd.sheetIdTo, positionToZone({ col, row }));
250-
this.model.selection.resetAnchor(toRaw(this), { cell: { col, row }, zone });
259+
this.model.selection.resetAnchor(this, { cell: { col, row }, zone });
251260
}
252261
break;
253262
case "DELETE_SHEET":
@@ -373,7 +382,7 @@ export class ComposerStore extends SpreadsheetStore {
373382
private startComposerRangeSelection() {
374383
if (this.sheetId === this.getters.getActiveSheetId()) {
375384
const zone = positionToZone({ col: this.col, row: this.row });
376-
this.model.selection.resetAnchor(toRaw(this), {
385+
this.model.selection.resetAnchor(this, {
377386
cell: { col: this.col, row: this.row },
378387
zone,
379388
});
@@ -404,7 +413,7 @@ export class ComposerStore extends SpreadsheetStore {
404413
this.colorIndexByRange = {};
405414
const zone = positionToZone({ col: this.col, row: this.row });
406415
this.model.selection.capture(
407-
toRaw(this),
416+
this,
408417
{ cell: { col: this.col, row: this.row }, zone },
409418
{
410419
handleEvent: this.handleEvent.bind(this),
@@ -519,7 +528,7 @@ export class ComposerStore extends SpreadsheetStore {
519528
return;
520529
}
521530
this.editionMode = "inactive";
522-
this.model.selection.release(toRaw(this));
531+
this.model.selection.release(this);
523532
}
524533

525534
/**

‎src/components/composer/composer_focus_store.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { ComposerSelection, ComposerStore } from "./composer/composer_store";
44
export type ComposerFocusType = "inactive" | "cellFocus" | "contentFocus";
55

66
export class ComposerFocusStore extends SpreadsheetStore {
7+
mutators = ["focusTopBarComposer", "focusGridComposerContent", "focusGridComposerCell"] as const;
78
private composerStore = this.get(ComposerStore);
89

910
private topBarFocus: Exclude<ComposerFocusType, "cellFocus"> = "inactive";

‎src/components/focus_store.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,14 @@
1-
import { toRaw } from "@odoo/owl";
2-
31
// The name is misleading and can be confused with the DOM focus.
42
export class FocusStore {
3+
mutators = ["focus", "unfocus"] as const;
54
public focusedElement: object | null = null;
65

76
focus(element: object) {
87
this.focusedElement = element;
98
}
109

1110
unfocus(element: object) {
12-
if (this.focusedElement && toRaw(this.focusedElement) === toRaw(element)) {
11+
if (this.focusedElement && this.focusedElement === element) {
1312
this.focusedElement = null;
1413
}
1514
}

‎src/components/grid/hovered_cell_store.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { SpreadsheetStore } from "../../stores";
22
import { Command, Position } from "../../types";
33

44
export class HoveredCellStore extends SpreadsheetStore {
5+
mutators = ["clear", "hover"] as const;
56
col: number | undefined;
67
row: number | undefined;
78

‎src/components/helpers/draw_grid_hook.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { DOMDimension, OrderedLayers } from "../../types";
99
export function useGridDrawing(refName: string, model: Model, canvasSize: () => DOMDimension) {
1010
const canvasRef = useRef(refName);
1111
useEffect(drawGrid);
12-
const rendererManager = useStore(RendererStore);
12+
const rendererStore = useStore(RendererStore);
1313
useStore(GridRenderer);
1414

1515
function drawGrid() {
@@ -39,7 +39,11 @@ export function useGridDrawing(refName: string, model: Model, canvasSize: () =>
3939

4040
for (const layer of OrderedLayers()) {
4141
model.drawLayer(renderingContext, layer);
42-
rendererManager.drawLayer(renderingContext, layer);
42+
// @ts-ignore 'drawLayer' is not declated as a mutator because:
43+
// it does not mutate anything. Most importantly it's used
44+
// during rendering. Invoking a mutator during rendering would
45+
// trigger another rendering, ultimately resulting in an infinite loop.
46+
rendererStore.drawLayer(renderingContext, layer);
4347
}
4448
}
4549
}

‎src/components/helpers/highlight_hook.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
1-
import { onMounted, useEffect, useEnv } from "@odoo/owl";
2-
import { useLocalStore } from "../../store_engine";
1+
import { onMounted, useEffect } from "@odoo/owl";
2+
import { useLocalStore, useStoreProvider } from "../../store_engine";
33
import { HighlightProvider, HighlightStore } from "../../stores/highlight_store";
4-
import { Ref, SpreadsheetChildEnv } from "../../types";
4+
import { Ref } from "../../types";
55
import { useHoveredElement } from "./listener_hook";
66

77
export function useHighlightsOnHover(ref: Ref<HTMLElement>, highlightProvider: HighlightProvider) {
88
const hoverState = useHoveredElement(ref);
9-
const env = useEnv() as SpreadsheetChildEnv;
9+
const stores = useStoreProvider();
1010

1111
useHighlights({
1212
get highlights() {
@@ -15,7 +15,7 @@ export function useHighlightsOnHover(ref: Ref<HTMLElement>, highlightProvider: H
1515
});
1616
useEffect(
1717
() => {
18-
env.model.dispatch("RENDER_CANVAS");
18+
stores.trigger("store-updated");
1919
},
2020
() => [hoverState.hovered]
2121
);

‎src/components/popover/cell_popover_store.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ import {
1111
import { HoveredCellStore } from "../grid/hovered_cell_store";
1212

1313
export class CellPopoverStore extends SpreadsheetStore {
14+
mutators = ["open", "close"] as const;
15+
1416
private persistentPopover?: CellPosition & { type: CellPopoverType };
1517

1618
protected hoveredCell = this.get(HoveredCellStore);

‎src/components/selection_input/selection_input_store.ts

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import { toRaw } from "@odoo/owl";
21
import { colors, isEqual, positionToZone, splitReference } from "../../helpers/index";
32
import { Get } from "../../store_engine";
43
import { SpreadsheetStore } from "../../stores";
@@ -21,6 +20,16 @@ export interface RangeInputValue {
2120
* This plugin handles this internal state.
2221
*/
2322
export class SelectionInputStore extends SpreadsheetStore {
23+
mutators = [
24+
"resetWithRanges",
25+
"focusById",
26+
"unfocus",
27+
"addEmptyRange",
28+
"removeRange",
29+
"changeRange",
30+
"reset",
31+
"confirm",
32+
] as const;
2433
ranges: RangeInputValue[] = [];
2534
focusedRangeIndex: number | null = null;
2635
private inputSheetId: UID;
@@ -90,7 +99,7 @@ export class SelectionInputStore extends SpreadsheetStore {
9099
row: 0,
91100
});
92101
const zone = this.getters.expandZone(cmd.sheetIdTo, positionToZone({ col, row }));
93-
this.model.selection.resetAnchor(toRaw(this), { cell: { col, row }, zone });
102+
this.model.selection.resetAnchor(this, { cell: { col, row }, zone });
94103
}
95104
break;
96105
}
@@ -110,7 +119,7 @@ export class SelectionInputStore extends SpreadsheetStore {
110119
if (focusIndex !== -1) {
111120
this.focus(focusIndex);
112121
const { left, top } = newZone;
113-
this.model.selection.resetAnchor(toRaw(this), {
122+
this.model.selection.resetAnchor(this, {
114123
cell: { col: left, row: top },
115124
zone: newZone,
116125
});
@@ -216,7 +225,7 @@ export class SelectionInputStore extends SpreadsheetStore {
216225

217226
private get hasMainFocus() {
218227
const focusedElement = this.focusStore.focusedElement;
219-
return !!focusedElement && toRaw(focusedElement) === toRaw(this);
228+
return !!focusedElement && focusedElement === this;
220229
}
221230

222231
get highlights(): Highlight[] {
@@ -251,7 +260,7 @@ export class SelectionInputStore extends SpreadsheetStore {
251260
unfocus() {
252261
this.focusedRangeIndex = null;
253262
this.focusStore.unfocus(this);
254-
this.model.selection.release(toRaw(this));
263+
this.model.selection.release(this);
255264
}
256265

257266
private captureSelection() {
@@ -262,7 +271,7 @@ export class SelectionInputStore extends SpreadsheetStore {
262271
const sheetId = this.getters.getActiveSheetId();
263272
const zone = this.getters.getRangeFromSheetXC(sheetId, range?.xc || "A1").zone;
264273
this.model.selection.capture(
265-
toRaw(this),
274+
this,
266275
{ cell: { col: zone.left, row: zone.top }, zone },
267276
{
268277
handleEvent: this.handleEvent.bind(this),

‎src/components/side_panel/chart/main_chart_panel/main_chart_panel_store.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { SpreadsheetStore } from "../../../../stores";
33
import { ChartCreationContext, ChartType, UID } from "../../../../types";
44

55
export class MainChartPanelStore extends SpreadsheetStore {
6+
mutators = ["activatePanel", "changeChartType"] as const;
67
panel: "configuration" | "design" = "configuration";
78
private creationContext: ChartCreationContext = {};
89

‎src/components/side_panel/find_and_replace/find_and_replace_store.ts

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ import { debounce, getSearchRegex, isInside, positionToZone } from "../../../hel
22
import { HighlightProvider, HighlightStore } from "../../../stores/highlight_store";
33
import { CellPosition, Color, Command, Highlight } from "../../../types";
44

5-
import { toRaw } from "@odoo/owl";
65
import { Get } from "../../../store_engine";
76
import { SpreadsheetStore } from "../../../stores";
87
import { SearchOptions } from "../../../types/find_and_replace";
@@ -16,6 +15,14 @@ enum Direction {
1615
}
1716

1817
export class FindAndReplaceStore extends SpreadsheetStore implements HighlightProvider {
18+
mutators = [
19+
"updateSearchOptions",
20+
"updateSearchContent",
21+
"searchFormulas",
22+
"selectPreviousMatch",
23+
"selectNextMatch",
24+
"replace",
25+
] as const;
1926
private allSheetsMatches: CellPosition[] = [];
2027
private activeSheetMatches: CellPosition[] = [];
2128
private specificRangeMatches: CellPosition[] = [];
@@ -44,11 +51,11 @@ export class FindAndReplaceStore extends SpreadsheetStore implements HighlightPr
4451
this.searchOptions.searchFormulas = this.initialShowFormulaState;
4552

4653
const highlightStore = get(HighlightStore);
47-
highlightStore.register(toRaw(this));
54+
highlightStore.register(this);
4855
this.onDispose(() => {
4956
this.model.dispatch("SET_FORMULA_VISIBILITY", { show: this.initialShowFormulaState });
5057
this.updateSearchContent.stopDebounce();
51-
highlightStore.unRegister(toRaw(this));
58+
highlightStore.unRegister(this);
5259
});
5360
}
5461

‎src/components/side_panel/pivot/pivot_side_panel/pivot_side_panel_store.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
} from "../../../../types/pivot";
1515

1616
export class PivotSidePanelStore extends SpreadsheetStore {
17+
mutators = ["applyUpdate", "renamePivot", "update"] as const;
1718
private updatesAreDeferred: boolean = true;
1819
private draft: PivotCoreDefinition | null = null;
1920
constructor(get: Get, private pivotId: UID) {

‎src/components/side_panel/side_panel/side_panel_store.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ interface ClosedSidePanel {
1919
export type SidePanelState = OpenSidePanel | ClosedSidePanel;
2020

2121
export class SidePanelStore extends SpreadsheetStore {
22+
mutators = ["open", "toggle", "close"] as const;
2223
initialPanelProps: SidePanelProps = {};
2324
componentTag: string = "";
2425

‎src/components/spreadsheet/spreadsheet.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import {
2828
SEPARATOR_COLOR,
2929
TOPBAR_HEIGHT,
3030
} from "../../constants";
31+
import { batched } from "../../helpers";
3132
import { ImageProvider } from "../../helpers/figures/images/image_provider";
3233
import { Model } from "../../model";
3334
import { Store, useStore, useStoreProvider } from "../../store_engine";
@@ -336,10 +337,15 @@ export class Spreadsheet extends Component<SpreadsheetProps, SpreadsheetChildEnv
336337
}
337338
});
338339

340+
const render = batched(this.render.bind(this, true));
339341
onMounted(() => {
340342
this.checkViewportSize();
343+
stores.on("store-updated", this, render);
344+
});
345+
onWillUnmount(() => {
346+
this.unbindModelEvents();
347+
stores.off("store-updated", this);
341348
});
342-
onWillUnmount(() => this.unbindModelEvents());
343349
onPatched(() => {
344350
this.checkViewportSize();
345351
});

‎src/helpers/misc.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,6 +313,27 @@ export function debounce<T extends (...args: any) => void>(
313313
return debounced as DebouncedFunction<T>;
314314
}
315315

316+
/**
317+
* Creates a batched version of a callback so that all calls to it in the same
318+
* microtick will only call the original callback once.
319+
*
320+
* @param callback the callback to batch
321+
* @returns a batched version of the original callback
322+
*
323+
* Copied from odoo/owl repo.
324+
*/
325+
export function batched(callback: () => void): () => void {
326+
let scheduled = false;
327+
return async (...args) => {
328+
if (!scheduled) {
329+
scheduled = true;
330+
await Promise.resolve();
331+
scheduled = false;
332+
callback(...args);
333+
}
334+
};
335+
}
336+
316337
/*
317338
* Concatenate an array of strings.
318339
*/

‎src/store_engine/dependency_container.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,14 @@
1+
import { EventBus } from "../helpers/event_bus";
12
import { Get, StoreConstructor, StoreParams } from "./store";
23

4+
interface StoreUpdateEvent {
5+
type: "store-updated";
6+
}
7+
38
/**
49
* A type-safe dependency container
510
*/
6-
export class DependencyContainer {
11+
export class DependencyContainer extends EventBus<StoreUpdateEvent> {
712
private dependencies: Map<StoreConstructor, any> = new Map();
813
private factory = new StoreFactory(this.get.bind(this));
914

0 commit comments

Comments
 (0)