From 8b5efc1346bbbb62719d032e2e37c48610380032 Mon Sep 17 00:00:00 2001
From: shirakaba <14055146+shirakaba@users.noreply.github.com>
Date: Wed, 21 Dec 2022 16:10:04 +0900
Subject: [PATCH] chore: audit usages of bind()

---
 packages/core/data/dom-events/dom-event.ts    | 23 +++++++++-----
 .../data/mutation-sensitive-array/index.ts    | 30 +++++++------------
 2 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/packages/core/data/dom-events/dom-event.ts b/packages/core/data/dom-events/dom-event.ts
index 8d59b7ffa..b5cc7fad2 100644
--- a/packages/core/data/dom-events/dom-event.ts
+++ b/packages/core/data/dom-events/dom-event.ts
@@ -369,11 +369,11 @@ export class DOMEvent implements Event {
 	//
 	// Creating it on the prototype and binding the context instead saves a
 	// further 30 nanoseconds per run of dispatchTo().
-	private onCurrentListenersMutation() {
+	private beforeCurrentListenersMutation() {
 		// Cloning the array via spread syntax is up to 180 nanoseconds
 		// faster per run than using Array.prototype.slice().
 		this.listenersLazyCopy = [...this.listenersLive];
-		this.listenersLive.onMutation = null;
+		this.listenersLive.beforeMutation = null;
 	}
 
 	// Taking multiple params instead of a single property bag saves 250
@@ -381,10 +381,10 @@ export class DOMEvent implements Event {
 	private handleEvent(data: EventData, isGlobal: boolean, phase: 0 | 1 | 2 | 3, removeEventListener: (eventName: string, callback?: any, thisArg?: any, capture?: boolean) => void, removeEventListenerContext: unknown) {
 		// Set a listener to clone the array just before any mutations.
 		//
-		// Lazy-binding this (binding it just before being called, rather than
-		// up-front) unexpectedly seems to slow things down - v8 may be
-		// optimising it for us or something.
-		this.listenersLive.onMutation = this.onCurrentListenersMutation.bind(this);
+		// Lazy-binding this (binding it at the time of calling, rather than
+		// eagerly) unexpectedly seems to slow things down - v8 may be applying
+		// some sort of optimisation or something.
+		this.listenersLive.beforeMutation = this.beforeCurrentListenersMutation.bind(this);
 
 		for (let i = this.listenersLazyCopy.length - 1; i >= 0; i--) {
 			const listener = this.listenersLazyCopy[i];
@@ -403,6 +403,13 @@ export class DOMEvent implements Event {
 			// We simply use a strict equality check here because we trust that
 			// the listeners provider will never allow two deeply-equal
 			// listeners into the array.
+			//
+			// This check costs 150 ns per dispatchTo(). I experimented with
+			// optimising this by building a Set of ListenerEntries that got
+			// removed during this handleEvent() (by introducing a method to
+			// MutationSensitiveArray called afterRemoval, similar to
+			// beforeMutation) to allow O(1) lookup, but it went 1000 ns slower
+			// in practice, so it stays!
 			if (!this.listenersLive.includes(listener)) {
 				continue;
 			}
@@ -415,6 +422,8 @@ export class DOMEvent implements Event {
 			}
 
 			if (once) {
+				// Calling with the context (rather than eagerly pre-binding it)
+				// saves about 100 nanoseconds per dispatchTo() call.
 				removeEventListener.call(removeEventListenerContext, this.type, callback, thisArg, capture);
 			}
 
@@ -443,7 +452,7 @@ export class DOMEvent implements Event {
 
 		// Make sure we clear the callback before we exit the function,
 		// otherwise we may wastefully clone the array on future mutations.
-		this.listenersLive.onMutation = null;
+		this.listenersLive.beforeMutation = null;
 	}
 
 	/**
diff --git a/packages/core/data/mutation-sensitive-array/index.ts b/packages/core/data/mutation-sensitive-array/index.ts
index f94d9bf4d..995b09546 100644
--- a/packages/core/data/mutation-sensitive-array/index.ts
+++ b/packages/core/data/mutation-sensitive-array/index.ts
@@ -1,57 +1,49 @@
 /**
  * A lightweight extension of Array that calls listeners just before any
  * mutations. This allows you to lazily take a clone of an array (i.e. use the
- * array as-is until such time as it mutates).
+ * array as-is until such time as it mutates). In other worse, "copy on write".
  *
  * This could equally be implemented by adding pre-mutation events into
  * ObservableArray, but the whole point is to be as lightweight as possible as
  * its entire purpose is to be used for performance-sensitive tasks.
  */
 export class MutationSensitiveArray<T> extends Array<T> {
-	onMutation: (() => void) | null = null;
-
-	private invalidate(): void {
-		if (this.onMutation) {
-			this.onMutation();
-		}
-	}
-
-	// Override each mutating Array method so that it invalidates our snapshot.
+	beforeMutation: (() => void) | null = null;
 
 	pop(): T | undefined {
-		this.invalidate();
+		this.beforeMutation?.();
 		return super.pop();
 	}
 	push(...items: T[]): number {
-		this.invalidate();
+		this.beforeMutation?.();
 		return super.push(...items);
 	}
 	reverse(): T[] {
-		this.invalidate();
+		this.beforeMutation?.();
 		return super.reverse();
 	}
 	shift(): T | undefined {
-		this.invalidate();
+		this.beforeMutation?.();
 		return super.shift();
 	}
 	sort(compareFn?: (a: T, b: T) => number): this {
-		this.invalidate();
+		this.beforeMutation?.();
 		return super.sort(compareFn);
 	}
 	splice(start: number, deleteCount: number, ...rest: T[]): T[] {
-		this.invalidate();
+		this.beforeMutation?.();
 		return super.splice(start, deleteCount, ...rest);
 	}
 	unshift(...items: T[]): number {
-		this.invalidate();
+		this.beforeMutation?.();
 		return super.unshift(...items);
 	}
 	fill(value: T, start?: number, end?: number): this {
-		this.invalidate();
+		this.beforeMutation?.();
 		return super.fill(value, start, end);
 	}
 	copyWithin(target: number, start: number, end?: number): this {
-		this.invalidate();
+		this.beforeMutation?.();
 		return super.copyWithin(target, start, end);
 	}
 }