[BUGFIX] Make the custom tag system non-enumerable

Currently, the custom tag system relies on an enumerable "symbol"-like
key, which is then placed on the objects that need to use it. This is an
expensive system, since it means we need to do a brand check to see if
the function exists, which means crawling the prototype chain every time
we look up a tag for a property on an object.

This PR instead places the functions in a WeakMap. This means that in
general, we can check to see if the WeakMap has the value, and if so
then we can use the function directly. WeakMap lookup is generally
faster, as it means we don't have to walk the entire prototype chain of
a megamorphic object, so this should generally be more performant.

It does mean that the functions in question need to be bound to the
instance, which could impact performance, but the functions should still
be inline-able when called (since they are called independent of the
object), and given that usage of tag lookup with non-proxies likely
dominates over usage with proxies, it should matter less than the
savings from using a WeakMap.

Finally, this means that CUSTOM_TAG_FOR is not exposed at all, so there
cannot be any observable side-effects such as the ones causing the bug
in: ember-m3/ember-m3#957

\### Unrelated Changes

In addition, I did a quick refactor of the proxy handlers to extract
them as classes. This is important for performance (POJOs with methods
on the instance do not inline, even as proxy handlers), I just never got
around to it before.

Author: Chris Garrett

packages/@glimmer/manager/index.ts

@@ -12,4 +12,4 @@ export { modifierCapabilities, CustomModifierManager } from './lib/public/modifi
 export { helperCapabilities, hasDestroyable, hasValue, customHelper } from './lib/public/helper';
 export { getComponentTemplate, setComponentTemplate } from './lib/public/template';
 export { capabilityFlagsFrom, hasCapability, managerHasCapability } from './lib/util/capabilities';
-export { CUSTOM_TAG_FOR } from './lib/util/args-proxy';
+export { getCustomTagFor, setCustomTagFor } from './lib/util/args-proxy';

packages/@glimmer/manager/lib/util/args-proxy.ts

@@ -6,10 +6,18 @@ import {
   CapturedPositionalArguments,
 } from '@glimmer/interfaces';
 import { Reference, valueForRef } from '@glimmer/reference';
-import { HAS_NATIVE_PROXY, enumerableSymbol } from '@glimmer/util';
+import { HAS_NATIVE_PROXY } from '@glimmer/util';
 import { Tag, track } from '@glimmer/validator';
 
-export const CUSTOM_TAG_FOR = enumerableSymbol('CUSTOM_TAG_FOR');
+const CUSTOM_TAG_FOR = new WeakMap<object, (obj: object, key: string) => Tag>();
+
+export function getCustomTagFor(obj: object): ((obj: object, key: string) => Tag) | undefined {
+  return CUSTOM_TAG_FOR.get(obj);
+}
+
+export function setCustomTagFor(obj: object, customTagFn: (obj: object, key: string) => Tag) {
+  CUSTOM_TAG_FOR.set(obj, customTagFn);
+}
 
 function convertToInt(prop: number | string | symbol): number | null {
   if (typeof prop === 'symbol') return null;
@@ -50,81 +58,89 @@ export let argsProxyFor: (
   type: 'component' | 'helper' | 'modifier'
 ) => Arguments;
 
-if (HAS_NATIVE_PROXY) {
-  argsProxyFor = (capturedArgs, type) => {
-    const { named, positional } = capturedArgs;
+class NamedArgsProxy implements ProxyHandler<{}> {
+  declare set?: (target: {}, prop: string | number | symbol) => boolean;
 
-    let getNamedTag = (key: string) => tagForNamedArg(named, key);
-    let getPositionalTag = (key: string) => tagForPositionalArg(positional, key);
+  constructor(private named: CapturedNamedArguments) {}
 
-    const namedHandler: ProxyHandler<{}> = {
-      get(_target, prop) {
-        const ref = named[prop as string];
+  get(_target: {}, prop: string | number | symbol) {
+    const ref = this.named[prop as string];
 
-        if (ref !== undefined) {
-          return valueForRef(ref);
-        } else if (prop === CUSTOM_TAG_FOR) {
-          return getNamedTag;
-        }
-      },
-
-      has(_target, prop) {
-        return prop in named;
-      },
-
-      ownKeys(_target) {
-        return Object.keys(named);
-      },
-
-      isExtensible() {
-        return false;
-      },
-
-      getOwnPropertyDescriptor(_target, prop) {
-        if (DEBUG && !(prop in named)) {
-          throw new Error(
-            `args proxies do not have real property descriptors, so you should never need to call getOwnPropertyDescriptor yourself. This code exists for enumerability, such as in for-in loops and Object.keys(). Attempted to get the descriptor for \`${String(
-              prop
-            )}\``
-          );
-        }
-
-        return {
-          enumerable: true,
-          configurable: true,
-        };
-      },
+    if (ref !== undefined) {
+      return valueForRef(ref);
+    }
+  }
+
+  has(_target: {}, prop: string | number | symbol) {
+    return prop in this.named;
+  }
+
+  ownKeys() {
+    return Object.keys(this.named);
+  }
+
+  isExtensible() {
+    return false;
+  }
+
+  getOwnPropertyDescriptor(_target: {}, prop: string | number | symbol) {
+    if (DEBUG && !(prop in this.named)) {
+      throw new Error(
+        `args proxies do not have real property descriptors, so you should never need to call getOwnPropertyDescriptor yourself. This code exists for enumerability, such as in for-in loops and Object.keys(). Attempted to get the descriptor for \`${String(
+          prop
+        )}\``
+      );
+    }
+
+    return {
+      enumerable: true,
+      configurable: true,
     };
+  }
+}
 
-    const positionalHandler: ProxyHandler<[]> = {
-      get(target, prop) {
-        if (prop === 'length') {
-          return positional.length;
-        }
+class PositionalArgsProxy implements ProxyHandler<[]> {
+  declare set?: (target: [], prop: string | number | symbol) => boolean;
+  declare ownKeys?: (target: []) => string[];
 
-        const parsed = convertToInt(prop);
+  constructor(private positional: CapturedPositionalArguments) {}
 
-        if (parsed !== null && parsed < positional.length) {
-          return valueForRef(positional[parsed]);
-        }
+  get(target: [], prop: string | number | symbol) {
+    let { positional } = this;
 
-        if (prop === CUSTOM_TAG_FOR) {
-          return getPositionalTag;
-        }
+    if (prop === 'length') {
+      return positional.length;
+    }
 
-        return (target as any)[prop];
-      },
+    const parsed = convertToInt(prop);
 
-      isExtensible() {
-        return false;
-      },
+    if (parsed !== null && parsed < positional.length) {
+      return valueForRef(positional[parsed]);
+    }
 
-      has(_target, prop) {
-        const parsed = convertToInt(prop);
+    return (target as any)[prop];
+  }
 
-        return parsed !== null && parsed < positional.length;
-      },
-    };
+  isExtensible() {
+    return false;
+  }
+
+  has(_target: [], prop: string | number | symbol) {
+    const parsed = convertToInt(prop);
+
+    return parsed !== null && parsed < this.positional.length;
+  }
+}
+
+if (HAS_NATIVE_PROXY) {
+  argsProxyFor = (capturedArgs, type) => {
+    const { named, positional } = capturedArgs;
+
+    let getNamedTag = (_obj: object, key: string) => tagForNamedArg(named, key);
+    let getPositionalTag = (_obj: object, key: string) => tagForPositionalArg(positional, key);
+
+    const namedHandler = new NamedArgsProxy(named);
+    const positionalHandler = new PositionalArgsProxy(positional);
 
     const namedTarget = Object.create(null);
     const positionalTarget: unknown[] = [];
@@ -149,25 +165,29 @@ if (HAS_NATIVE_PROXY) {
       positionalHandler.ownKeys = forInDebugHandler;
     }
 
+    const namedProxy = new Proxy(namedTarget, namedHandler);
+    const positionalProxy = new Proxy(positionalTarget, positionalHandler);
+
+    setCustomTagFor(namedProxy, getNamedTag);
+    setCustomTagFor(positionalProxy, getPositionalTag);
+
     return {
-      named: new Proxy(namedTarget, namedHandler),
-      positional: new Proxy(positionalTarget, positionalHandler),
+      named: namedProxy,
+      positional: positionalProxy,
     };
   };
 } else {
   argsProxyFor = (capturedArgs, _type) => {
     const { named, positional } = capturedArgs;
 
-    let getNamedTag = (key: string) => tagForNamedArg(named, key);
-    let getPositionalTag = (key: string) => tagForPositionalArg(positional, key);
+    let getNamedTag = (_obj: object, key: string) => tagForNamedArg(named, key);
+    let getPositionalTag = (_obj: object, key: string) => tagForPositionalArg(positional, key);
 
     let namedProxy = {};
+    let positionalProxy: unknown[] = [];
 
-    Object.defineProperty(namedProxy, CUSTOM_TAG_FOR, {
-      configurable: false,
-      enumerable: false,
-      value: getNamedTag,
-    });
+    setCustomTagFor(namedProxy, getNamedTag);
+    setCustomTagFor(positionalProxy, getPositionalTag);
 
     Object.keys(named).forEach((name) => {
       Object.defineProperty(namedProxy, name, {
@@ -179,14 +199,6 @@ if (HAS_NATIVE_PROXY) {
       });
     });
 
-    let positionalProxy: unknown[] = [];
-
-    Object.defineProperty(positionalProxy, CUSTOM_TAG_FOR, {
-      configurable: false,
-      enumerable: false,
-      value: getPositionalTag,
-    });
-
     positional.forEach((ref: Reference, index: number) => {
       Object.defineProperty(positionalProxy, index, {
         enumerable: true,

packages/@glimmer/util/index.ts

@@ -23,6 +23,7 @@ export * from './lib/template';
 export { default as _WeakSet } from './lib/weak-set';
 export { castToSimple, castToBrowser, checkNode } from './lib/simple-cast';
 export * from './lib/present';
+export { default as intern } from './lib/intern';
 
 export { default as debugToString } from './lib/debug-to-string';
 export { beginTestSteps, endTestSteps, logStep, verifySteps } from './lib/debug-steps';

packages/@glimmer/util/lib/intern.ts

@@ -0,0 +1,49 @@
+/**
+  Strongly hint runtimes to intern the provided string.
+
+  When do I need to use this function?
+
+  For the most part, never. Pre-mature optimization is bad, and often the
+  runtime does exactly what you need it to, and more often the trade-off isn't
+  worth it.
+
+  Why?
+
+  Runtimes store strings in at least 2 different representations:
+  Ropes and Symbols (interned strings). The Rope provides a memory efficient
+  data-structure for strings created from concatenation or some other string
+  manipulation like splitting.
+
+  Unfortunately checking equality of different ropes can be quite costly as
+  runtimes must resort to clever string comparison algorithms. These
+  algorithms typically cost in proportion to the length of the string.
+  Luckily, this is where the Symbols (interned strings) shine. As Symbols are
+  unique by their string content, equality checks can be done by pointer
+  comparison.
+
+  How do I know if my string is a rope or symbol?
+
+  Typically (warning general sweeping statement, but truthy in runtimes at
+  present) static strings created as part of the JS source are interned.
+  Strings often used for comparisons can be interned at runtime if some
+  criteria are met.  One of these criteria can be the size of the entire rope.
+  For example, in chrome 38 a rope longer then 12 characters will not
+  intern, nor will segments of that rope.
+
+  Some numbers: http://jsperf.com/eval-vs-keys/8
+
+  Known Trick™
+
+  @private
+  @return {String} interned version of the provided string
+*/
+export default function intern(str: string): string {
+  let obj: Record<string, number> = {};
+  obj[str] = 1;
+  for (let key in obj) {
+    if (key === str) {
+      return key;
+    }
+  }
+  return str;
+}

packages/@glimmer/util/lib/platform-utils.ts

@@ -1,4 +1,5 @@
 import { Maybe } from '@glimmer/interfaces';
+import intern from './intern';
 
 export type Factory<T> = new (...args: unknown[]) => T;
 
@@ -40,7 +41,7 @@ export type Lit = string | number | boolean | undefined | null | void | {};
 export const tuple = <T extends Lit[]>(...args: T) => args;
 
 export function enumerableSymbol(key: string): any {
-  return `__${key}${Math.floor(Math.random() * Date.now())}__`;
+  return intern(`__${key}${Math.floor(Math.random() * Date.now())}__`);
 }
 
 export const symbol = HAS_NATIVE_SYMBOL ? Symbol : enumerableSymbol;