[FIX] stores: bye bye reactivity 👋 #4104

LucasLefevre · 2024-04-19T15:05:48Z

Description:

Reactivity is very very (very) slow, with potential overhead exceeding 90%.

With a reactive store, every this.something call traverses the reactive
proxy, incurring significant cost.

This commit completely revisit the way store updates trigger app rendering.
Previously, this process relied entirely on Owl's reactivity system.

Instead, it introduces a new approach where store updates are triggered by
explicitly declared public methods (mutators)
Since stores follow the CQS principle all public methods actually
mutate state in a store, they cannot have any other effect since they
can't 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 properties 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!

Task: : 3829125

review checklist

robodoo · 2024-04-19T15:06:16Z

Pull request status dashboard.

src/components/composer/composer/composer_store.ts

LucasLefevre · 2024-05-14T07:15:19Z

src/components/helpers/draw_grid_hook.ts

+      // @ts-ignore
      rendererManager.drawLayer(renderingContext, layer);


drawLayer is a method used during rendering. It's not a mutator.

src/store_engine/dependency_container.ts

rrahir · 2024-05-14T10:25:02Z

src/store_engine/store_hooks.ts

@@ -15,8 +15,8 @@ export function useStoreProvider() {
    __spreadsheet_stores__: container,
    getStore: <T extends StoreConstructor>(Store: T) => {
      const store = container.get(Store);
-      const mutators: readonly string[] = store.mutators;
-      return proxifyToRender(env, store, mutators);
+      // TODO check this, it shouldn't be/use a hook here


because useStoreRenderProxy uses useCompent which is only available in the setup of components. (right click on a cell, Insert link => boom)

ah right :'(

rrahir · 2024-05-14T10:52:25Z

src/stores/grid_renderer_store.ts

@@ -57,20 +56,14 @@ import { RendererStore } from "./renderer_store";
 export const CELL_BACKGROUND_GRIDLINE_STROKE_STYLE = "#111";

 export class GridRenderer {
+  mutators = [] as const;


is it mandatory to declare for stores? the typing seems to miss this.

Does a store makes sense without mutators ?

Does a store makes sense without mutators ?

currently yes because other stores need to register themselves to this store

pro-odoo

Very nice 🥳 Bye bye toRaw and thanks for nothing

pro-odoo · 2024-05-14T12:07:04Z

src/components/composer/composer/composer_store.ts

@@ -84,9 +93,9 @@ export class ComposerStore extends SpreadsheetStore {

  constructor(get: Get) {
    super(get);
-    this.highlightStore.register(toRaw(this));
+    this.highlightStore.register(this);


pro-odoo · 2024-05-14T12:09:53Z

src/stores/grid_renderer_store.ts

@@ -57,20 +56,14 @@ import { RendererStore } from "./renderer_store";
 export const CELL_BACKGROUND_GRIDLINE_STROKE_STYLE = "#111";

 export class GridRenderer {
+  mutators = [] as const;


Does a store makes sense without mutators ?

rrahir · 2024-05-21T08:14:19Z

Could be the occasion to get rid of "RENDER_CANVAS" command then since we have a new bus to work with?

diff --git a/src/components/helpers/highlight_hook.ts b/src/components/helpers/highlight_hook.ts
index 7c0edb7e0..4fb2116f5 100644
--- a/src/components/helpers/highlight_hook.ts
+++ b/src/components/helpers/highlight_hook.ts
@@ -1,12 +1,12 @@
-import { onMounted, useEffect, useEnv } from "@odoo/owl";
-import { useLocalStore } from "../../store_engine";
+import { onMounted, useEffect } from "@odoo/owl";
+import { useLocalStore, useStoreProvider } from "../../store_engine";
 import { HighlightProvider, HighlightStore } from "../../stores/highlight_store";
-import { Ref, SpreadsheetChildEnv } from "../../types";
+import { Ref } from "../../types";
 import { useHoveredElement } from "./listener_hook";
 
 export function useHighlightsOnHover(ref: Ref<HTMLElement>, highlightProvider: HighlightProvider) {
   const hoverState = useHoveredElement(ref);
-  const env = useEnv() as SpreadsheetChildEnv;
+  const stores = useStoreProvider();
 
   useHighlights({
     get highlights() {
@@ -15,7 +15,7 @@ export function useHighlightsOnHover(ref: Ref<HTMLElement>, highlightProvider: H
   });
   useEffect(
     () => {
-      env.model.dispatch("RENDER_CANVAS");
+      stores.trigger("store-updated");
     },
     () => [hoverState.hovered]
   );
``` @LucasLefevre WDYT?

LucasLefevre · 2024-05-21T08:28:17Z

@rrahir true. I pushed a commit :)

rrahir

Ok for me :)

pro-odoo

👍

VincentSchippefilt · 2024-05-21T12:57:44Z

src/helpers/misc.ts

+/**
+ * Creates a batched version of a callback so that all calls to it in the same
+ * microtick will only call the original callback once.
+ *
+ * @param callback the callback to batch
+ * @returns a batched version of the original callback
+ *
+ * Copied from odoo/owl repo.
+ */
+export function batched(callback: () => void): () => void {
+  let scheduled = false;
+  return async (...args) => {
+    if (!scheduled) {
+      scheduled = true;
+      await Promise.resolve();
+      scheduled = false;
+      callback(...args);
+    }
+  };
+}
+


shouldn't batched be linked to the nextanimationframe instead of the microtick ?

I pushed a commit but it's 🤢 just because we don't have a direct access to owl's Scheduler (same trick used in the test).
I'll go and talk to the framework-js team tomorrow to check if they can export Scheduler from owl (or an updated version of their batched using Scheduler.requestAnimationFrame)

VincentSchippefilt · 2024-05-21T12:59:21Z

I think it is a fantastic idea. good job !

VincentSchippefilt · 2024-05-21T14:34:42Z

src/helpers/misc.ts

 * Creates a batched version of a callback so that all calls to it in the same
 * microtick will only call the original callback once.


THIS IS A LIE :)

VincentSchippefilt · 2024-05-22T07:02:30Z

what would be less error prone:

class MyStore extends Store {
 constructor() {...}
 static mutators = ["myMytator","myOtherMutator"]
 myMutator(value) { ... }
 myOtherMutator(value) { ... }
 myGetter() { return value; }
}

or

class MyStore extends Store {
 constructor() {...}
 mutators : {
    myMutator = function (value) { ... },
    myOtherMutator = function (value) { ... }
 }
 myGetter() { return value; } // if I write this.mutators.myMutator here, it's obviously a bug
}

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! Task: 3829125

rrahir · 2024-05-22T08:36:23Z

@robodoo r+

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 #4104 Task: 3829125 Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>

LucasLefevre force-pushed the saas-17.2-stores-bye-reactivity-lul branch from bbaf339 to 472403b Compare April 19, 2024 18:43

LucasLefevre force-pushed the saas-17.2-stores-bye-reactivity-lul branch 7 times, most recently from f8b0ca3 to f4b34aa Compare May 14, 2024 07:19

LucasLefevre commented May 14, 2024

View reviewed changes

rrahir reviewed May 14, 2024

View reviewed changes

pro-odoo reviewed May 14, 2024

View reviewed changes

LucasLefevre force-pushed the saas-17.2-stores-bye-reactivity-lul branch 3 times, most recently from 082434b to 80dac25 Compare May 17, 2024 12:01

LucasLefevre changed the title ~~[POC] stores: bye bye reactivity 👋~~ [FIX] stores: bye bye reactivity 👋 May 17, 2024

LucasLefevre force-pushed the saas-17.2-stores-bye-reactivity-lul branch 3 times, most recently from 5a8d43e to 88bbda7 Compare May 17, 2024 12:25

rrahir approved these changes May 21, 2024

View reviewed changes

pro-odoo approved these changes May 21, 2024

View reviewed changes

VincentSchippefilt reviewed May 21, 2024

View reviewed changes

LucasLefevre force-pushed the saas-17.2-stores-bye-reactivity-lul branch from edd3c0d to 5f4bcfc Compare May 22, 2024 08:35

robodoo temporarily deployed to merge May 22, 2024 08:50 Inactive

robodoo closed this May 22, 2024

fw-bot mentioned this pull request May 22, 2024

[FW][FIX] stores: bye bye reactivity 👋 #4244

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] stores: bye bye reactivity 👋 #4104

[FIX] stores: bye bye reactivity 👋 #4104

LucasLefevre commented Apr 19, 2024 •

edited

robodoo commented Apr 19, 2024

LucasLefevre May 14, 2024

rrahir May 14, 2024

LucasLefevre May 14, 2024

rrahir May 14, 2024

rrahir May 14, 2024

pro-odoo May 14, 2024

LucasLefevre May 17, 2024

pro-odoo left a comment

pro-odoo May 14, 2024

pro-odoo May 14, 2024

rrahir commented May 21, 2024

LucasLefevre commented May 21, 2024

rrahir left a comment

pro-odoo left a comment

VincentSchippefilt May 21, 2024

LucasLefevre May 21, 2024 •

edited

VincentSchippefilt commented May 21, 2024

VincentSchippefilt May 21, 2024

VincentSchippefilt commented May 22, 2024

rrahir commented May 22, 2024

		// @ts-ignore
		rendererManager.drawLayer(renderingContext, layer);

		* Creates a batched version of a callback so that all calls to it in the same
		* microtick will only call the original callback once.

[FIX] stores: bye bye reactivity 👋 #4104

[FIX] stores: bye bye reactivity 👋 #4104

Conversation

LucasLefevre commented Apr 19, 2024 • edited

Description:

review checklist

robodoo commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pro-odoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrahir commented May 21, 2024

LucasLefevre commented May 21, 2024

rrahir left a comment

Choose a reason for hiding this comment

pro-odoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasLefevre May 21, 2024 • edited

Choose a reason for hiding this comment

VincentSchippefilt commented May 21, 2024

Choose a reason for hiding this comment

VincentSchippefilt commented May 22, 2024

rrahir commented May 22, 2024

LucasLefevre commented Apr 19, 2024 •

edited

LucasLefevre May 21, 2024 •

edited