From 69178c9617beca4bc4f162a8e4b3fb738d348c2d Mon Sep 17 00:00:00 2001
From: Yury Selivanov <yury@edgedb.com>
Date: Wed, 12 Feb 2025 23:21:16 -0800
Subject: [PATCH] Optimize codecs context cacheing

We should really copy the cached codex context for cloned
Options instances (unless .withCodecs() was called).
---
 packages/driver/src/options.ts | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/packages/driver/src/options.ts b/packages/driver/src/options.ts
index 021281cac..4ac17adbb 100644
--- a/packages/driver/src/options.ts
+++ b/packages/driver/src/options.ts
@@ -178,8 +178,10 @@ export class Options {
   /** @internal */
   readonly annotations: ReadonlyMap<string, string> = new Map<string, string>();
 
-  private cachedCodecContext: CodecContext | null = null;
-  private cachedCodecContextVer = -1;
+  /** @internal */
+  cachedCodecContext: CodecContext | null = null;
+  /** @internal */
+  cachedCodecContextVer = -1;
 
   get tag(): string | null {
     return this.annotations.get(TAG_ANNOTATION_KEY) ?? null;
@@ -269,7 +271,6 @@ export class Options {
       clone.moduleAliases = this.moduleAliases;
     }
 
-    let originalCodecsMap = false;
     if (mergeOptions.codecs != null) {
       clone.codecs = new Map([
         ...this.codecs,
@@ -277,7 +278,12 @@ export class Options {
       ]) as ReadonlyCodecMap;
     } else {
       clone.codecs = this.codecs;
-      originalCodecsMap = true;
+      // It  makes sense to just inherit the cached codec context
+      // as it's expensive to build. The only case when it can't be
+      // cached is when .withCodecs() was called, but caching will
+      // work great for all other .withSomething() methods we have.
+      clone.cachedCodecContext = this.cachedCodecContext;
+      clone.cachedCodecContextVer = this.cachedCodecContextVer;
     }
 
     if (mergeOptions._dropSQLRowCodec && clone.codecs.has("_private_sql_row")) {
@@ -285,7 +291,7 @@ export class Options {
       // and it's set to "object mode", the we want the codec mapping to be
       // empty instead. Why? Empty codec mapping short circuits a lot of
       // custom codec code, and object is the default behavior anyway.
-      if (originalCodecsMap) {
+      if (clone.codecs === this.codecs) {
         // "codecs" map isn't a full-blown read-only mapping, we're just
         // treating it as such and protecting from the user to mutate
         // directly. This allows for optimizing away pointless copies.
@@ -293,6 +299,8 @@ export class Options {
         // mapping from another options object, we have to clone it
         // before deleting any keys or mutating it in any way.
         clone.codecs = new Map(clone.codecs);
+        clone.cachedCodecContext = null;
+        clone.cachedCodecContextVer = -1;
       }
       (clone.codecs as MutableCodecMap).delete("_private_sql_row");
     }