From 4a761a74037a74837ebd5d0ebe8c9fa6428cb5a5 Mon Sep 17 00:00:00 2001
From: Samuel Degueldre <sad@odoo.com>
Date: Fri, 27 Jan 2023 08:57:16 +0100
Subject: [PATCH] [FIX] reactivity: improve performance for long-lived
 reactives

Previously, when having a long lived reactive object and writing a lot
of keys to it, clearing the reactive subscriptions on that object would
slow down as time went on. This is because when clearing a target's
subsctiptions for a callback, we look at all the keys that are observed, and for all
the observed keys, we remove the callback from the set of callbacks
observing that key. The problem arises from the fact that after doing
that, even if the set of callbacks observing that key is now empty, we
don't remove the key from the observed keys, meaning that any further
clearing of subscriptions will have to iterate over that key to attempt
to clear the callbacks even if there are none.

This commit fixes this problem by simply removing the key from the
observedKeys if there are no longer any callbacks observing it.

This commit also improves performance for bare reactives (reactives
created with no callback). The initial implementation simply creates a
default empty callback when creating a reactive, and treats it like any
other, meaning it can observe keys and be notified of changes even
though we know in advance that it does nothing. This can compound with
the previous issue when you're doing a lot of manipulations on a bare
reactive internally for the sole purpose of notifying outside observers,
as this creates a lot of useless work in the reactivity system.
---
 src/runtime/reactivity.ts | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/src/runtime/reactivity.ts b/src/runtime/reactivity.ts
index 19610535..7d61f97f 100644
--- a/src/runtime/reactivity.ts
+++ b/src/runtime/reactivity.ts
@@ -1,8 +1,13 @@
-import { Callback } from "./utils";
+import type { Callback } from "./utils";
 import { OwlError } from "./error_handling";
 
 // Special key to subscribe to, to be notified of key creation/deletion
 const KEYCHANGES = Symbol("Key changes");
+// Used to specify the absence of a callback, can be used as WeakMap key but
+// should only be used as a sentinel value and never called.
+const NO_CALLBACK = () => {
+  throw new Error("Called NO_CALLBACK. Owl is broken, please report this to the maintainers.");
+};
 
 // The following types only exist to signify places where objects are expected
 // to be reactive or not, they provide no type checking benefit over "object"
@@ -86,6 +91,9 @@ const targetToKeysToCallbacks = new WeakMap<Target, Map<PropertyKey, Set<Callbac
  * @param callback the function to call when the key changes
  */
 function observeTargetKey(target: Target, key: PropertyKey, callback: Callback): void {
+  if (callback === NO_CALLBACK) {
+    return;
+  }
   if (!targetToKeysToCallbacks.get(target)) {
     targetToKeysToCallbacks.set(target, new Map());
   }
@@ -140,8 +148,11 @@ export function clearReactivesForCallback(callback: Callback): void {
     if (!observedKeys) {
       continue;
     }
-    for (const callbacks of observedKeys.values()) {
+    for (const [key, callbacks] of observedKeys.entries()) {
       callbacks.delete(callback);
+      if (!callbacks.size) {
+        observedKeys.delete(key);
+      }
     }
   }
   targetsToClear.clear();
@@ -187,7 +198,7 @@ const reactiveCache = new WeakMap<Target, WeakMap<Callback, Reactive<Target>>>()
  *  reactive has changed
  * @returns a proxy that tracks changes to it
  */
-export function reactive<T extends Target>(target: T, callback: Callback = () => {}): T {
+export function reactive<T extends Target>(target: T, callback: Callback = NO_CALLBACK): T {
   if (!canBeMadeReactive(target)) {
     throw new OwlError(`Cannot make the given value reactive`);
   }