perf: memoize values of cmp props when used multiple times in template

[jodarove]

Details

Fixes: #243

This PR memoize repeated property access to the component instance from the template.

Example:

<template>
     <p>{author.name}, {author.lastmane}</p>
</template>

now compiles to:

  function tmpl($api, $cmp, $slotset, $ctx) {
    const {
      d: api_dynamic,
      t: api_text,
      h: api_element
    } = $api;
    const { author: $cv0 } = $cmp;
    return [api_element("p", {
      key: 0
    }, [api_dynamic($cv0.name), api_text(", "), api_dynamic($cv0.lastmane)])];
  }

Implementation details

In LWC, a property does not becomes reactive until is accessed. In the following example, bar does not become reactive until foo === true:

<template>
	<template if:true={foo}>
              <p>{bar.name}, {bar.lastname}</p> <!-- bar does not become reactive until foo === true -->
        </template>
</template>

Because of this, we can't access the used properties from the component once at the root scope (it will become reactive) and reuse its value in the following usages.

This PR changes:

1. (b9258ee) The template compilation to create one scope (function) for the if and for/itetator directives in the same hierarchy as the template html.
2. (8f4ed06) Memoize repeated access to component bound property: For each scope when serialized, any access to component bound property A ($cmp.A(.B...)) will be replaced with a variable A' (A'(.B...)), initialized like const { A: A' } = $cmp; in the current scope or some of the ancestor scopes. if and only if any of the following is true:
   a) If A' is declared in an ancestor scope.
   b) If A is used at least 2 times in the current scope.
   c) If A is used only one time in this scope, and at least another time in one of the children's scopes.

Note to Reviewers: The compiler changes are in these commits: b9258ee and 8f4ed06. The rest is updating the template compiler snapshots and adding new ones.

Todos:

• Handle the case where a component property is accessed only within a foreach See this comment, we may want to do it as a separate PR.

Does this PR introduce breaking changes?

• 🚨 Yes, it does introduce breaking changes.

There may be some case in wich the getter returns different values for each invocation. Ex:

<template>
    <p>Random number 1: {randomNumber}</p>
    <p>Random number 2: {randomNumber}</p>
</template>

.... {
   get randomNumber() {
      return Math.random();
   }
}

[caridy]

Let's prioritize this one since it is going to help locker. /cc @manuel-jasso

[manuel-jasso]

Let's prioritize this one since it is going to help locker. /cc @manuel-jasso

You mean from a perf standpoint @caridy? And is there an action item for Locker?

[caridy]

Can you add some explanation in the description about how this will work? looking at the code is very hard to understand.

[nolanlawson]

I tested this a bit manually, and I noticed that this will call the getter only once:

<template>
  {foo}
  {foo}
  {foo}
</template>

but this will call it multiple times:

<template>
  <template for:each="..." for:items="...">
    {foo}
  </template>
</template>

I guess maybe it's kind of an edge case, though?

[nolanlawson]

FWIW I was able to write a Tachometer benchmark to demonstrate the perf improvement from this PR. The benchmark renders a component with an expensive getter, and that getter is referenced 100 times in the template. (So with this PR, the getter would only be called once rather than 100 times.) It's a bit unrepresentative, but it shows a consistent improvement compared to master:

The existing benchmarks do not seem to show an improvement or regression, but they also aren't really exercising the right code paths to show a difference.

[jodarove]

I tested this a bit manually, and I noticed that this will call the getter only once:

<template>
  {foo}
  {foo}
  {foo}
</template>

but this will call it multiple times:

<template>
  <template for:each="..." for:items="...">
    {foo}
  </template>
</template>

I guess maybe it's kind of an edge case, though?

@nolanlawson this is a great catch! it's an edge case, but I do think is one that can be addressed (when foo is bound to the component).

[jodarove]

@nolanlawson I took a look at the iteration case, and the short answer: is difficult.

Today the foreach and iterators directive generate a function that is called from the engine for each element in the collection:

    return api_iterator($cmp.collection, function (item) {
        return api_element("span", {
          key: api_key(0, item.id)
        }, [api_dynamic($cmp.foo)]);
      });

This PR changes that compilation to:

    function foreach1_0(item) {
      return api_element("span", {
        key: api_key(0, item.id)
      }, [api_dynamic($cmp.foo)]);
    }

    return api_iterator($cmp.collection, foreach1_0);

which is the same. We can't access cmp.foo until we know for sure that the collection has at least one element (otherwise it becomes reactive when it shouldn't).

[nolanlawson]

@jodarove Ah, that is a good point. Well, the iterator is probably an edge case anyway. (Most iterations I imagine would be repeating items from the array, not from the outer scope.)

[nolanlawson]

Can you explain this change?

[pmdartus]

IMHO, the risk of breakage and the complexity introduced in the code gen doesn't worth the potential performance gain. I would expect the potential performance gain to be quite minimal since only the top-level property lookup is cached (in the expression foo.bar.baz, only the foo lookup is cached).

Karma integration test should also be added to validate this change.

[pmdartus]

This PR makes reading the compiled template code way more complex, I would recommend keeping those expressions as close as possible to the original expression.

Suggested change

	const { computed: $cv0_0 } = $cmp;
	const $cv0_0 = $cmp.computed;

[pmdartus]

Nested if block and for each block makes it complex/impossible to understand the actual code flow.

One way to make it more intelligible would be to use a similar approach than Svelte with comments preceding blocks. Example of generated code.

[pmdartus]

I would rename most of the references from scope to block. While an iteration creates a new lexical scope with the item and the index, the if block doesn't. This is why I think the term block makes more sense in this context.

[pmdartus]

To make this symmetrical with the popScope method.

Suggested change

	createScope() {
	pushScope() {

[pmdartus]

Why making the identifier more complex by adding the childScopes.length?

[pmdartus]

An assertion should be added here to make sure the setFn should be invoked before serializeInto

[nolanlawson]

Just my two cents: I think caching getters is probably a good perf improvement on the average LWC app, since component authors end up writing lots of getters, and those getters can be 1) frequently called, and 2) expensive.

That said, PM's point about not caching deep.getters.like.this is valid. I also wonder if we could get even bigger perf improvements by caching getters very aggressively outside of templates.

For instance, I've seen a perf issue in the wild that was due to getters, but it would not be solved by this PR, because it was due to repeated re-renders, where each render was calling several expensive getters. Trivial example:

  renderedCallback() {
    console.log(
      this.foo, // expensive
      this.bar, // also expensive
      this.baz  // you get the idea
    )
  }

[AllanOricil]

@nolanlawson it is better if you also run these benchmarks and ask the repo owner to add LWC on the list https://github.com/krausest/js-framework-benchmark

[caridy]

That said, PM's point about not caching deep.getters.like.this is valid. I also wonder if we could get even bigger perf improvements by caching getters very aggressively outside of templates.

I want to hear more about this. Keep in mind that in the first few versions of LWC (raptor at the time), we did some of this, basically, the template never hit the getter directly, but a function that calls the getter under the hood, and can make optimizations, basically, we don't give the component to the template function, but a wrapper around it, so when the template tries to access those getters, you could cache the result, the problem is when to invalidate the result.

[nolanlawson]

@caridy I was thinking along the lines of computed properties (#2437). If we have an explicit API for this, then we know when to cache and when to invalidate. For instance, if sum is defined as a + b, then we invalidate whenever a or b are set.

I suppose we don't always need the explicit signal for invalidation though? We are a compiler, so we could theoretically parse:

get () sum {
  return this.a + this.b
}

...and understand that we can cache the sum getter and only invalidate when a or b are set. We would need this in the JS compiler, though, and we would need to bail out if we don't have the full information:

get () sum {
  return somethingOutsideOfImmediateScope()
}

[nolanlawson]

@AllanOricil It is a fair point, that sometimes you might want caching even without computed properties. E.g.:

get foo () {
  return somethingSuperExpensive()
}

I guess the question is whether the framework should proactively try to cache getters (as in this PR), or if we should rely on an explicit signal from developers (such as @cached).

[caridy]

@AllanOricil @nolanlawson this can't be about new decorators, this has to be about existing code, and how to optimize it without developers intervention.

I think this PR was trying to move the caching to the compiled code, but it seems like it is too complex, and covers too little. An alternative solution is to add a layer in the engine itself which has access to the lifecycle mechanism and instance level metadata. That might be a far better option, or maybe a combination of both, saying: {foo} will need caching if it appears more than once in the template, but it is only used once, then why bothering to add more complexity?

[nolanlawson]

Since this is an observable change, it may only be possible to merge after API versioning is in.

@jodarove Do you still want to proceed with this PR?