Normative: Cache templates per site, rather than by contents

[littledan]

The previous definition of template caching had a few issues:

• (from @syg) Template strings may live forever due to putting them
  in a WeakMap
• (from @ajklein) Because of this logic, it's rather difficult to
  implement any GC at all of template objects
• (from @erights) The template string facility cannot be extended
  to expose anything about the site, as it's site-independent

This patch makes template caching key off the Parse Node where the
template occurs in source, rather than the List of Strings that the
template evaluates into.

These semantics seem to match SpiderMonkey's implementation of templates.
V8, ChakraCore and JSC, on the other hand, implement the prior semantics.

Resolves #840

Edit: The version of SpiderMonkey I was running is two months old.

[syg]

@littledan The behavior in SM was recently patched to be per-content and live-forever. Though I'd be very, very happy to revert this. :)

[littledan]

FWIW the eshost test

eshost -e '(function() { function id(x) { return x; } for (let i = 0; i < 100000000; i++) { eval("id`"+i+"`")}})()'

hangs on three engines, apparently as they fight over allocating more system memory, but completes cleanly in the older version of SM that I'm running.

[ajklein]

@caitp implemented V8's template literals, she may have thoughts here.

[caitp]

The template object caching landed separately from the main feature (https://codereview.chromium.org/742643003).

Though there's no mention of it on the CL, there had been some discussion at the time about it off of rietveld. I believe the rationalization for the caching was to help some tagged template literal functions do their work faster. It's still unclear to me how caching would help userspace code at all, since the substitution values aren't taken into account.

From v8's POV, I think I would prefer if template objects were not cached, and I'm not sure any actual applications are doing any kind of caching (as noted above, not useful because substitutions not considered).

[shvaikalesh]

@caitp on userspace use cases:

Helper tag that removes comments/whitespaces from regexp patterns, like Ruby's extended flag. It should not care about substitutions, only strings in raw. Implementation is kinda neat. Also, libs like t7 and styled-components may find caching useful (substitutions should not affect their AST's).

EDIT: I've measured 5-6x perf boost in patterns with > 5 substitutions because of caching, V8.

[caitp]

Right, but those are still fairly limited uses. Thanks for mentioning it, though. I appreciate seeing uses in the wild.

[arv]

@caitp Caching was a core feature that @erights and Mike Samuel had in the original proposal. It is useful for memoizing expensive work.

We used this feature effectively in traceur to not have to reparse parse trees. https://github.com/google/traceur-compiler/blob/master/src/codegeneration/PlaceholderParser.js

[littledan]

@arv Do you think this PR leaves enough caching in place to be a benefit to the way Traceur took advantage of it here?

[caitp]

I concede the point, thanks for taking the time to explain the need for the cache, guys.

Based on the examples I've seen, I think the proposed change to the lifetime of cached entries makes sense, and shouldn't hurt code using that style in the wild.

[erights]

I just looked at the pull request itself. What determines when two Parse Nodes are the same? What does Parse Node mean precisely in the context of the rest of the spec?

[erights]

(reentering comment so I can use markdown)

From v8's POV, I think I would prefer if template objects were not
cached, and I'm not sure any actual applications are doing any kind of
caching (as noted above, not useful because substitutions not considered).

Hi @caitp ,

The whole point of the design is to enable the tag to parse and preparation
from the literal parts only once, and to amortize that cost over multiple
substitution values.

See https://github.com/erights/quasiParserGenerator for a good example.

var arith = bnf`
    start ::= expr EOF  ${(v,_) => v};
    expr ::=
      term "+" expr     ${(a,_,b) => (...subs) => a(...subs) + b(...subs)}
    / term;
    term ::=
      NUMBER            ${n => (..._) => JSON.parse(n)}
    / HOLE              ${h => (...subs) => subs[h]}
    / "(" expr ")"      ${(_,v,_2) => v};
   `;
}

Does one parser generation from the literally described grammar, and can
apply the generated parser to multiple values for the action functions.

The tag it generates can be used

function foo(i) {
  return arith`1 + (2 + ${i} + ${i+1}) + 4`;
}

to reuse the parse and preparation from the literal parts of the DSL, and
apply this over multiple substitution values for i.

[littledan]

@erights Parse Node is a concept used around the spec; it was already the input to the modified abstract algorithm. For equality, I want to say that this is by identity, that is, whenever a different string is parsed as ECMAScript source text, it generates new Parse Nodes with a distinct identity that is being tested here. Do you have suggestions for wording to clarify that?

[erights]

Actually, if made explicit, I think that's a great clarification. Each act of parsing endows the resulting parse nodes with unique identities.

To avoid muddying your otherwise great clarification, I would delete the "different" in

whenever a different string is parsed as ECMAScript source text

It does not matter whether the string is different. What matters is each time the spec calls for the act of parsing this string. In particular:

eval(str); eval(str);

causes str to be parsed twice, resulting in parse nodes with distinct identities, even though the string is not different.

With the identity issue made explicit, and perhaps even illustrated with this example in a note, I like this PR. It repairs all the problems you pointed out with the content-based caching and opens the door to the addition of a source map. The loss of memo hits that would result is of no consequence whatever. Tags would still get the memo hit they need --- from successive evaluation of the "same" template literal expression, for an adequate sense of "same".

Thanks!

[allenwb]

The place where the concept of "Parse Node" probably should be clarified is in [5.2]{https://tc39.github.io/ecma262/#sec-algorithm-conventions) which currently says:

Algorithms may be associated with productions of one of the ECMAScript grammars. A production that has multiple alternative definitions will typically have a distinct algorithm for each alternative. When an algorithm is associated with a grammar production, it may reference the terminal and nonterminal symbols of the production alternative as if they were parameters of the algorithm. When used in this manner, nonterminal symbols refer to the actual alternative definition that is matched when parsing the source text.

The term"parse node" is used several times in ECMA-262 but currently does not have a definition.

In the current spec. parsing takes place in step 2 ParseScript and step 2 of ParseModule. Also in step 6 of PerformEval. The two step 2s should probably be refactored into a common ParseSourceText abstract operation parameterized with the goal symbol and PerfomEval step 6 should be rewritten to use that abstract operation.

Then a "parse node" can be defined as a node of the parse tree produced by ParseSourceText. And the above coded paragraph of 5.2 might be rewritten as (added text in italic):

Algorithms may be associated with productions of one of the ECMAScript grammars. A production that has multiple alternative definitions will typically have a distinct algorithm for each alternative. Algorithms associated with grammar productions are evaluated by applying them to an parse node produced by the ParseSourceText abstraction operation. When an algorithm is associated witha grammar production algorithm , it may reference the parse nodes of terminal and nonterminal symbols of the production alternative as if they were parameters of the algorithm. When used in this manner, nonterminal symbols refer to the parse node of the actual alternative definition that is was matched when parsing the source text.

[jmdyck]

The term"parse node" is used several times in ECMA-262 but currently does not have a definition.

There's been a definition (in The Syntactic Grammar) since #804 was merged two months ago.

In the current spec. parsing takes place in step 2 ParseScript and step 2 of ParseModule. Also in step 6 of PerformEval.

Also in:

• CoveredParenthesizedExpression
• CoveredCallExpression
• CoveredFormalsList
• ClassDefinitionEvaluation
• CoveredAsyncArrowHead
• CreateDynamicFunction
• RegExpInitialize
• JSON.parse
• Evaluation of AssignmentExpression, AssignmentElement, AssignmentRestElement
• ForIn/OfBodyEvaluation

(but this is maybe getting a little off-topic)

[littledan]

@erights Great example, I completely agree that those should be separate Parse Nodes. Thanks for the correction.

[littledan]

@jmdyck Thanks for doing the hard work of building the basis for describing parse nodes around the spec. I uploaded a patch which uses that to define the "same parse node", as used in template caching in this PR. What do you think?

[jmdyck]

Rather than "a Parse Node containing a |TemplateLiteral|", it would be more consistent (with usage in the rest of the spec) to say something like:

• "a Parse Node that is a |TemplateLiteral|", or
• "a Parse Node that is an instance of |TemplateLiteral|", or
• "a |TemplateLiteral| (i.e., a Parse Node)".

[littledan]

Done

[bterlson]

FWIW, I think the current text is now better WRT @allenwb's feedback: https://tc39.github.io/ecma262/#sec-syntactic-grammar paragraph 3 defines parse nodes in a way that I think doesn't require further abstraction into an explicit ParseSourceText AO. Let me know if you disagree.

[erights]

Do you think that it makes the freshness requirement explicit? Perhaps I am missing something?

[bterlson]

I find this clarification confusing personally. Suggestions (not sure if I like them, but here they are):

Add to previous graph a note about fresh instances:

Each Parse Node is an instance of a symbol in the grammar; Parse Nodes represent a span of the source text that can be derived from that symbol. New Parse Nodes are newly instantiated for each invocation of the parser and never reused between parses even of identical source text.

Then...

Parse nodes are considered identical if and only if they are the same instance. Thus, two parse nodes are identical if and only if they represent the same span of source text, are instances of the same grammar symbol, and resulted from the same parser invocation.

[jmdyck]

(In "Parse Nodes represent", the plural doesn't make sense, because the rest of the sentence is singular. You could change it to "the Parse Node represents" or just leave it as "it represents".)

"New ... newly" is redundant.

I can imagine an implementer balking at the suggestion they can't reuse the results of a previous parse, so you might want to add a note saying why it's specified that way. Or at least a cross-reference to places in the spec where the identity of Parse Nodes is important.

Have you convinced yourself that no-reuse doesn't have unintended consequences? I.e., that there aren't places in the spec that already implicitly assume 'reuse' of parse nodes?

[allenwb]

I can imagine an implementer balking at the suggestion they can't reuse the results of a previous parse

Implementations are free to do anything that doesn't observably violate the specification.

[jmdyck]

Right, so my imagined implementor would want to know how reusing the result of a previous parse would observably violate the spec.

[bterlson]

I would still prefer my proposed tweak here (with updates from @jmdyck) - thoughts @littledan?

[littledan]

I updated the specification to use a version of @bterlson's tweak (though I kept "the same parse node" dfn to allow the link from the usage, and I incorporated @jmdyck's suggestions).

[bterlson]

@erights I don't think it does - I was only commenting on the need to define Parse Nodes. Just pointing out I've since added a definition (with a dfn and all!) so further abstraction isn't necessary. But if you think explicit ParseSourceText AO is warranted for clarifying "Freshness" I'd be ok with that. I proposed some alternative language above you might find acceptable, too...

[erights]

I proposed some alternative language above you might find acceptable, too...

+1

[littledan]

@bterlson One question about what it means for parse nodes to be fresh (which came up with @caitp's implementation): If a browser caches a Script, then is it allowed to use the same ParseNodes, or does it have to produce different ones?

[littledan]

We discussed this PR at the July TC39 meeting. @allenwb seemed to argue (please correct me if I'm misunderstanding) that it may be OK if not all of this memory is collectable, and that this "leak" is by-design behavior. The memory is collected when the realm dies, which should be enough for applications which consist of many short-lived realms.

Even if it was by design, I still think this issue is worth discussing further. I believe this is the only piece of memory associated with running code which cannot be collected if the function becomes unreachable. For a long-running JavaScript application, which may load various pieces of code and then move onto other pieces of code, without creating new realms for the new code and discarding old realms, this could be a serious issue.

[littledan]

@waldemarhorwat pointed out that we need to clarify how ParseNode behaves in the presence of cover grammars, which trigger reparsing. Modulo that, this proposal reached consensus at the September 2017 TC39 meeting.

[WebReflection]

unless you were explicitly bundling first, then passing a whole bundle to Babel

who on earth would do anything different from that?

I don't distribute multiple Babel patches per file, I bundle all at once and babel once the result.

Babel is not the issue here, Babel tries its best to provide what's defined in here.

[WebReflection]

Also, I don't get to know how 3rd parts use my code so none of you has any solution to the problem I am describing:

• I don't know if they use polyfills upfront for WeakMap, if they do, now my code breaks thanks to this change and broken poklyfill, OR, it will cause leaks (should I put a note in the README? sure thing)
• if they write same DOM string in two different parts they'll have slower results due extra parsing and potential leaks to legacy (should I put a note in the README? sure thing)

None of this has ever been my problem for my 100% code covered library that works in every browser, so thanks again for considering improving the way you break specifications.

[loganfsmyth]

who on earth would do anything different from that?

The majority of people use babel-cli and babel-loader which will run Babel on each file separately.

Babel is not the issue here, Babel tries its best to provide what's defined in here.

Yup! Just wanted to clarify that I'd never expect the average user of Babel to be able to share template instances beyond the file level.

[WebReflection]

The majority of people use babel-cli and babel-loader which will run Babel on each file separately.

• AFAIK the majority use bundlers, bundlers are smarter, bundlers are good ... consider using bundlers
• I don't get to know how users of my library include my library. Before there might have been a couple of extra templates stored in memory, I'll sign for that if the alternative is that now they might wrongly polyfill upfront a WeakMap and see my code break

[ljharb]

Bundlers babel each file separately and then bundle the result - webpack and browserify specifically.

[WebReflection]

Bundler babel each file separately and then bundle the result - webpack and browserify specifically.

I don't know how you configured that, that's not what I do, neither what my WebPack or Rollup do, but on top of that, I don't care.

If interested, we can have a conversation a part where I explain you how to bundle without babel in the middle and then bundle, if needed, only the result at once, not per file (per file is silly)

[jridgewell]

I'd like to remind everyone that we have a code of conduct. We're getting to a point of being aggressive with our responses, and that won't fly.

---

To address a few points:

Sure, so I just ignore the fact now there might be more leaks due this change so I just don't care and keep using Map, right?

Using a WeakMap or a Map is totally orthogonal to this change. Best practices would be to use a WeakMap for GC, as long as you don't need to iterate the map. I'll provide a WeakMap detection sample at the end, but I don't see why it's necessary? Browser support for Map and WeakMap seem to align.

Babel

Discussing Babel's processing of files seems off-topic.

And the worst part, is that few others have been decent enough to recognize breaking changes like this on 3 yo specs are bad, if delivered in this way, but your conclusion is instead

We made a breaking change in an attempt to prevent a pretty easy memory leak. Out intuition was that this was not a going to cause much pain, because any significant length template strings would not be duplicated in source code. It certainly might bite devs who duplicate shorter templates strings.

Additionally, Firefox seems not to have implemented the correct behavior, and Babel/TypeScript didn't. So we really didn't see this as too badly breaking.

---

A pretty simple solution for detecting WeakMap shams is to just use a WeakMap:

let MapConstructor;
try {
  const map = new WeakMap();
  const frozen = Object.freeze({});
  map.add(frozen, 1);
  if (map.get(frozen) !== 1) {
    throw new Error('sham');
  }
  MapConstructor = WeakMap;
} catch (e) {
  MapConstructor = Map;
}

[WebReflection]

A pretty simple solution for detecting WeakMap shams is to just use a WeakMap:

I am already doing shimming the WeakMap but if I fallback to Map, since Babel 7 will not return always the same object, I will cause inevitably leaks, the reason this change was proposed, to legacy browsers, those already slow, those with usually less RAM to spare.

This is not a solution for me, just a work around to not have breaking code. I have leaking code now, while before the leak was contained in an easy to measure amount of template literals.

Now leaks in legacy browsers are unpredictable due new transpiler behavior (that simply follows specs updates)

[esprehn]

@caitp @littledan

You have the same source text for a couple reasons:

1. The same reasons you would with a regex, ex. You might want to match all letters and numbers in multiple parts of your code. You could refactor that into a module, but if it's so short as /[a-z0-9]/ you might as well just type it inline.

As prior art v8 and other engines all cache regexes based on text not source location.

2. Because it's super natural to write markup which is very repetitive in small chunks. Ex. You'd write ```html`

   ```` in lots of places. This is really the same as point (1). For the same reason you write the same CSS selectors repeatedly passed into .querySelector.

Ex. You might write:

element.append(html`<p>${user.name}`);
if (user.email) {
  element.append(html`<p>${user.email}`);
}
// ... And so on with lots of repeated markup.

And it's also natural for multiple components to use the same markup. For example different form widgets would do:

root.append(html`<label for=input>${this.label}</label>`);

(Note with shadow roots ids are not unique, so this is totally fine to reuse the same id inside every component).

It's also natural to create web components this way:

button1 = html`<app-button>${text}`;
// In another part of the program
button2 = html`<app-button>${otherText}`

I'd just caution about making assumptions about there not being lots of repeated tagged templates, that's a pretty reasonable way to use them. :)

[SMotaal]

I think that the discussion should be more about how changes like this one can have very drastic compounding effects downstream. When a spec changes, implementers have all the capabilities they need to manage efficiencies. Then we hit the very visible barriers that are enforced on JavaScript execution in browsers (for everyone's benefit).

So, if an opportunity presented itself to come up with a very reliable caching/mapping mechanism, which was to spec for more than 1 revision of the standard, it obviously cannot be not breaking, or at the very least worthy of a mitigating alternative other than reliance on the notion that polyfills are actually solutions.

The little interaction from the rest of the community is not necessarily approval, this ecosystem is built on the notion of compartmentalization, and people don't often realize what the package they use really does internally or how it relates to specs. In fact, I know of a similar library which was a mainstream buzz for a while, and if anything, at least some of their team had no idea how the spec related to what they were putting inside their blackbox for us users to use "reliably".

The specs are for implementers, but they ultimately and far more importantly define the limitations that are imposed on the rest of the JavaScript community who actually use the features as they are made available.

Let's not just create a new concept with this kind of change, but if we do, I suggest we use the term retrofill which is more appropriate in this case. A good compromise would offer an opt-in platform-managed alternative which would drastically minimize the GC overhead for code that doesn't intentionally choose the raw-string caching behaviour.

For this I think that RegExp's g and y behaviours for same string are good precedences for thinking of a more explicit solution.

[WebReflection]

people don't often realize what the package they use really does internally or how it relates to specs.

just to clarify, it looks like I was the only one out there using Map on purpose, understanding that WeakMap would've been useless for TO as key, accordingly to ES2015, hence I 100% agree with you that changes like this should be delivered definitively more carefully.

[littledan]

@esprehn I don't think anything in the new design makes it impossible or even very difficult to do caching based on source text, and I don't debate that caching based on source text can be useful. This change just doesn't make the default behavior to require this caching and, as such, create a guaranteed memory leak (even if you clear out the not-recently-used elements of your Map, the system has to keep the mapping from strings to template objects eternally).