Cache template literal callSiteObj

src/harmony-templates.js

Index: src/harmony-templates.js
diff --git a/src/harmony-templates.js b/src/harmony-templates.js
index dd122cce85f28ae5e478af25fb9a1a8c8bbb2c9c..d01faadd76ab4f4c9d00e5eb4d8ffde338eb8237 100644
--- a/src/harmony-templates.js
+++ b/src/harmony-templates.js
@@ -4,11 +4,65 @@
 
 'use strict';
 
-function GetTemplateCallSite(siteObj, rawStrings) {
-  // TODO(caitp): ensure same template callsite is used for subsequent tag calls
+var callSiteCache = new $Map;
+
+function SameCallSiteElements(rawStrings, other) {
+  var length = rawStrings.length;
+  var other = other.raw;
+
+  if (length !== other.length) return false;
+
+  for (var i = 0; i < length; ++i) {
+    if (rawStrings[i] !== other[i]) return false;
+  }
+
+  return true;
+}
+
+
+function GetCachedCallSite(siteObj, hash) {
+  var obj = callSiteCache.get(hash);

[arv (Not doing code reviews), 2014/11/19 06:09:11]

This needs to be be %MapGet(callSiteCache, hash) or someone can hijack this.

[caitp (gmail), 2014/11/19 07:15:05]

Done.

+
+  if (IS_UNDEFINED(obj)) return;
+
+  if ("raw" in obj) {

[caitp (gmail), 2014/11/19 03:50:33]

I know this is broken if Array.prototype is messed with. Maybe a private symbol
to identify these would make sense?

[arv (Not doing code reviews), 2014/11/19 06:09:11]

Why is this needed. Can we not make sure we only store valid call site objects
in the cache?

[caitp (gmail), 2014/11/19 07:15:05]

In an unlikely event where we have hash collisions, the value is a List of some
kind, rather than a single callSiteObj. The lone object is a valid call site,
it's just to determine if it's an array of callsites, or an array of strings
(single callsite).

Less-complex alternative: Always put cached values in an array, even when
they're alone (wastes a small bit of memory for the common no-collision case).

+    // Common case
+    if (SameCallSiteElements(siteObj, obj)) return obj;
+  } else if (IS_ARRAY(obj)) {
+    // Rare case
+    for (var i = 0; i < obj.length; ++i) {
+      if (SameCallSiteElements(siteObj, obj[i])) return obj[i];
+    }
+  } else {
+    throw "UNREACHABLE";
+  }
+}
+
+
+function SetCachedCallSite(siteObj, hash) {
+  var obj = callSiteCache.get(hash);
+
+  if (IS_UNDEFINED(obj)) {
+    callSiteCache.set(hash, siteObj);

[arv (Not doing code reviews), 2014/11/19 06:09:11]

%MapSet

+  } else if ("raw" in obj) {

[caitp (gmail), 2014/11/19 03:50:33]

Same here... what about using a Set of an array?

[arv (Not doing code reviews), 2014/11/19 06:09:11]

I'm not sure why this is needed? Don't we only get here if we have a real call
site object?

+    callSiteCache.set(hash, [obj, siteObj]);
+  } else if (IS_ARRAY(obj)) {

[arv (Not doing code reviews), 2014/11/19 06:09:11]

How can obj be anything but an array?

[caitp (gmail), 2014/11/19 07:15:05]

Explained above --- it can be a callSiteObj, or an array of callSiteObjs ---
callSiteObj is an array, so it's a bit confusing and bad. Alternative was
already proposed, if it sounds good I don't see why not.

+    callSiteCache.set(hash, obj.concat(siteObj));
+  } else {
+    throw "UNREACHABLE";
+  }
+
+  return siteObj;
+}
+
+
+function GetTemplateCallSite(siteObj, rawStrings, hash) {
+  var cached = GetCachedCallSite(rawStrings, hash);
+
+  if (!IS_UNDEFINED(cached)) return cached;
 
   %AddNamedProperty(siteObj, "raw", %ObjectFreeze(rawStrings),
       READ_ONLY | DONT_ENUM | DONT_DELETE);
 
-  return %ObjectFreeze(siteObj);
+  return SetCachedCallSite(%ObjectFreeze(siteObj), hash);
 }