Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Perf] Memoize static objects in template iterator #2559

Closed
nolanlawson opened this issue Oct 29, 2021 · 2 comments · Fixed by #2589
Closed

[Perf] Memoize static objects in template iterator #2559

nolanlawson opened this issue Oct 29, 2021 · 2 comments · Fixed by #2589
Labels

Comments

@nolanlawson
Copy link
Collaborator

For a component like this:

<template>
    <table>
        <tbody>
            <template for:each={rows} for:item="row">
                <tr key={row.id} class={row.className}>
                    <td>{row.id}</td>
                    <td><a onclick={handleSelect}>{row.label}</a></td>
                    <td><a onclick={handleRemove}>Remove</a></td>
                </tr>
            </template>
        </tbody>
    </table>
</template>

The generated template can be optimized by moving duplicated property bags outside of the iteration:

 function tmpl($api, $cmp, $slotset, $ctx) {
   const {k: api_key, d: api_dynamic_text, t: api_text, h: api_element, b: api_bind, i: api_iterator} = $api;
   const {_m0, _m1} = $ctx;
+  const key3 = { key: 3 }
+  const key4 = { key: 4 }
+  const key5 = {
+    key: 5,
+    on: {
+      "click": _m0 || ($ctx._m0 = api_bind($cmp.handleSelect))
+    }
+  }
+  const key6 = { key: 6 }
+  const key7 = {
+    key: 7,
+    on: {
+      "click": _m1 || ($ctx._m1 = api_bind($cmp.handleRemove))
+    }
+  }
   return [api_element("table", {
     key: 0
   }, [api_element("tbody", {
@@ -9,22 +24,8 @@ function tmpl($api, $cmp, $slotset, $ctx) {
     return api_element("tr", {
       className: row.className,
       key: api_key(2, row.id)
-    }, [api_element("td", {
-      key: 3
-    }, [api_text(api_dynamic_text(row.id))]), api_element("td", {
-      key: 4
-    }, [api_element("a", {
-      key: 5,
-      on: {
-        "click": _m0 || ($ctx._m0 = api_bind($cmp.handleSelect))
-      }
-    }, [api_text(api_dynamic_text(row.label))])]), api_element("td", {
-      key: 6
-    }, [api_element("a", {
-      key: 7,
-      on: {
-        "click": _m1 || ($ctx._m1 = api_bind($cmp.handleRemove))
-      }
-    }, [api_text("Remove")])])]);
+    }, [api_element("td", key3, [api_text(api_dynamic_text(row.id))]),
+      api_element("td", key4, [api_element("a", key5, [api_text(api_dynamic_text(row.label))])]),
+      api_element("td", key6, [api_element("a", key7, [api_text("Remove")])])]);
   }))])];
-}
+}

This is similar to #2300, but it's a bit different because the goal is to avoid creating unnecessary duplicate objects in a loop. For instance, the { key: 4 } object doesn't need to be created once per iteration.

Based on my tests, this would improve the dom-table-create-10k benchmark by 1%-5%.

Screen Shot 2021-10-29 at 10 02 31 AM

The trick would be to recognize property bags that don't reference any local variables in the iteration function (row in this case). E.g. the { className: row.className, key: api_key(2, row.id) } object cannot be moved.

For property bags that reference getters outside of the iteration (e.g. $cmp.handleRemove above), this is an observable change, because the getter won't be called once per iteration. Arguably we can either 1) only optimize truly static data (like { key: 4 }), or 2) permit the observable change, as in #2300.

@uip-robot-zz
Copy link

This issue has been linked to a new work item: W-10113376

@nolanlawson nolanlawson changed the title [Perf] Memoize static property bags in template iterator [Perf] Memoize static objects in template iterator Nov 1, 2021
nolanlawson added a commit that referenced this issue Dec 1, 2021
nolanlawson added a commit that referenced this issue Dec 1, 2021
nolanlawson added a commit that referenced this issue Dec 2, 2021
nolanlawson added a commit that referenced this issue Dec 6, 2021
nolanlawson added a commit that referenced this issue Dec 7, 2021
nolanlawson added a commit that referenced this issue Dec 8, 2021
nolanlawson added a commit that referenced this issue Dec 9, 2021
nolanlawson added a commit that referenced this issue Jan 3, 2022
nolanlawson added a commit that referenced this issue Jan 4, 2022
* perf: hoist static object/arrays in templates

Fixes #2559

W-10113376

* fix: fix license header

* fix: use map size

* fix: update packages/@lwc/template-compiler/src/codegen/optimize.ts

Co-authored-by: Pierre-Marie Dartus <[email protected]>

* fix: remove t.isExpression check

* fix: fix typescript

* fix: fix vnode shared-object issue

* fix: add typescript readonly guard

* fix: fix hydration, add more typescript guards

* fix: mark more props as readonly

* fix: mark another prop as readonly

* fix: actually fix innerHTML

* fix: mark children as read-only

* fix: clone the oldCh array

* test: fix snapshot

Co-authored-by: Pierre-Marie Dartus <[email protected]>
@nolanlawson
Copy link
Collaborator Author

I'm going to consider this fixed by #2589. We can open new issues for new compile-time optimizations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants