perf: experiment with an alternate binary search implementation (#9852)

* Optimize the binary search for assets

Optimizations:
- remove recursion to use a while loop instead
- do not allocate unnecessary array (to reduce GC)
- convert the manifest to a Uint8Array once upfront
- added a faster vesrion of compare (compareSearchValueWithEntry). The compare implementatiopn was moved to the tests where it is used

Other changes
- BS: use indexes instead of offest (IMO easier to read/debug)
- BS: return the content hash instead of the whole entry

* Add an experiment for the update search algo

* fixup! fallback to the current implementation on error

Author: vicb

packages/workers-shared/asset-worker/src/assets-manifest.2.ts

@@ -0,0 +1,122 @@
+import {
+	CONTENT_HASH_OFFSET,
+	CONTENT_HASH_SIZE,
+	ENTRY_SIZE,
+	HEADER_SIZE,
+	PATH_HASH_SIZE,
+} from "../../utils/constants";
+
+export class AssetsManifest {
+	private data: Uint8Array;
+
+	constructor(data: ArrayBuffer) {
+		this.data = new Uint8Array(data);
+	}
+
+	async get(pathname: string) {
+		const pathHash = await hashPath(pathname);
+		const entry = binarySearch(this.data, pathHash);
+		return entry ? Uint8ToHexString(entry) : null;
+	}
+}
+
+export const hashPath = async (path: string) => {
+	const encoder = new TextEncoder();
+	const data = encoder.encode(path);
+	const hashBuffer = await crypto.subtle.digest(
+		"SHA-256",
+		data.buffer as ArrayBuffer
+	);
+	return new Uint8Array(hashBuffer, 0, PATH_HASH_SIZE);
+};
+
+/**
+ * Search for an entry with the given hash path.
+ *
+ * @param manifest the manifest bytes
+ * @param pathHash the path hash to find in the manifest
+ * @returns The content hash when the entry is found and `false` otherwise
+ */
+export const binarySearch = (
+	manifest: Uint8Array,
+	pathHash: Uint8Array
+): Uint8Array | false => {
+	if (pathHash.byteLength !== PATH_HASH_SIZE) {
+		throw new TypeError(
+			`Search value should have a length of ${PATH_HASH_SIZE}`
+		);
+	}
+
+	const numberOfEntries = (manifest.byteLength - HEADER_SIZE) / ENTRY_SIZE;
+
+	if (numberOfEntries === 0) {
+		return false;
+	}
+
+	let lowIndex = 0;
+	let highIndex = numberOfEntries - 1;
+
+	while (lowIndex <= highIndex) {
+		const middleIndex = (lowIndex + highIndex) >> 1;
+
+		const cmp = comparePathHashWithEntry(pathHash, manifest, middleIndex);
+
+		if (cmp < 0) {
+			highIndex = middleIndex - 1;
+			continue;
+		}
+
+		if (cmp > 0) {
+			lowIndex = middleIndex + 1;
+			continue;
+		}
+
+		return new Uint8Array(
+			manifest.buffer,
+			HEADER_SIZE + middleIndex * ENTRY_SIZE + CONTENT_HASH_OFFSET,
+			CONTENT_HASH_SIZE
+		);
+	}
+
+	return false;
+};
+
+/**
+ * Compares a search value with an entry in the manifest
+ *
+ * @param searchValue a `Uint8Array` of size `PATH_HASH_SIZE`
+ * @param manifest the manifest bytes
+ * @param entryIndex the index in the manifest of the entry to compare
+ */
+function comparePathHashWithEntry(
+	searchValue: Uint8Array,
+	manifest: Uint8Array,
+	entryIndex: number
+) {
+	let entryOffset = HEADER_SIZE + entryIndex * ENTRY_SIZE;
+	for (let offset = 0; offset < PATH_HASH_SIZE; offset++, entryOffset++) {
+		// We know that both values could not be undefined
+		// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+		const s = searchValue[offset]!;
+		// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+		const e = manifest[entryOffset]!;
+		if (s < e) {
+			return -1;
+		}
+		if (s > e) {
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * Converts an Uint8Array to an hex string
+ *
+ * @param array The content hash
+ * @returns padded hex string
+ */
+const Uint8ToHexString = (array: Uint8Array) => {
+	return [...array].map((b) => b.toString(16).padStart(2, "0")).join("");
+};

packages/workers-shared/asset-worker/src/experiment-analytics.ts

@@ -14,6 +14,8 @@ type Data = {
 	manifestReadTime?: number;
 
 	// -- Blobs --
+	// blob1 current or perfTest version of binary search
+	binarySearchVersion?: "current" | "perfTest" | "current-fallback";
 };
 
 export class ExperimentAnalytics {
@@ -44,7 +46,9 @@ export class ExperimentAnalytics {
 			doubles: [
 				this.data.manifestReadTime ?? -1, // double1
 			],
-			blobs: [],
+			blobs: [
+				this.data.binarySearchVersion, // blob1
+			],
 		});
 	}
 }

packages/workers-shared/asset-worker/src/worker.ts

@@ -4,6 +4,7 @@ import { setupSentry } from "../../utils/sentry";
 import { mockJaegerBinding } from "../../utils/tracing";
 import { Analytics } from "./analytics";
 import { AssetsManifest } from "./assets-manifest";
+import { AssetsManifest as AssetsManifest2 } from "./assets-manifest.2";
 import { normalizeConfiguration } from "./configuration";
 import { ExperimentAnalytics } from "./experiment-analytics";
 import { canFetch, handleRequest } from "./handler";
@@ -231,6 +232,12 @@ export default class<TEnv extends Env = Env> extends WorkerEntrypoint<TEnv> {
 		pathname: string,
 		_request?: Request
 	): Promise<string | null> {
+		const BINARY_SEARCH_EXPERIMENT_SAMPLE_RATE = 0.05;
+		const binarySearchVersion =
+			Math.random() < BINARY_SEARCH_EXPERIMENT_SAMPLE_RATE
+				? "current"
+				: "perfTest";
+
 		const analytics = new ExperimentAnalytics(this.env.EXPERIMENT_ANALYTICS);
 		const performance = new PerformanceTimer(this.env.UNSAFE_PERFORMANCE);
 		const jaeger = this.env.JAEGER ?? mockJaegerBinding();
@@ -243,13 +250,31 @@ export default class<TEnv extends Env = Env> extends WorkerEntrypoint<TEnv> {
 				analytics.setData({
 					accountId: this.env.CONFIG.account_id,
 					experimentName: "manifest-read-timing",
+					binarySearchVersion,
 				});
 			}
 
 			const startTimeMs = performance.now();
 			try {
-				const assetsManifest = new AssetsManifest(this.env.ASSETS_MANIFEST);
-				const eTag = await assetsManifest.get(pathname);
+				let eTag: string | null;
+
+				if (binarySearchVersion === "perfTest") {
+					try {
+						const assetsManifest = new AssetsManifest2(
+							this.env.ASSETS_MANIFEST
+						);
+						eTag = await assetsManifest.get(pathname);
+					} catch {
+						// Fallback to the "current" impl if the new one throws.
+						// We use "current-fallback" to surface errors in the analytics data
+						analytics.setData({ binarySearchVersion: "current-fallback" });
+						const assetsManifest = new AssetsManifest(this.env.ASSETS_MANIFEST);
+						eTag = await assetsManifest.get(pathname);
+					}
+				} else {
+					const assetsManifest = new AssetsManifest(this.env.ASSETS_MANIFEST);
+					eTag = await assetsManifest.get(pathname);
+				}
 
 				span.setTags({
 					path: pathname,