Rework repeat directive for better performance. Fixes #242. #501

kevinpschaaf · 2018-09-13T17:01:31Z

Rework repeat directive to use a more efficient algorithm to avoid unnecessary DOM detachment/attachment during common array operations (add, remove, swap, reverse, etc.).

A few TODO's in the implementation to discuss:

The algorithm iterates in-order arrays of (old) Parts and (new) results; however the algorithm also requires in-order arrays of their keys also; as such I found it cleanest to assign the key to the part/result so only one array each for old/new is required. Should keying be promoted to the first-class interface for these objects?
I made helper API's for the part operations needed in repeat (create/update/move/remove); these should probably somehow be folded into the Part API, but wanted to discuss first since creation/ownership of the markers seems to be a case-by-case basis right now?
I did experiment with an implementation of the LIS algorithm mentioned in the comments, which slotted into this implementation fairly cleanly. I recommend holding off adding that until there is a clear use case that would benefit from the extra code+time complexity.
I'm now eagerly generating the key-to-index map (and storing for use in the next turn), as the optimization to only create it in the final else clause (over the subset of old items remaining) didn't show up in benchmarks as significant. This made it straightforward to warn in the case of duplicate keys, which we treat as an error and warn as "undefined behavior", and gives more flexibility to change the algorithm in the future.
Let's also discuss benchmarks; I made a parameterized repeat benchmark that I ran using a stripped-down/upgraded version of polyperf; I didn't check it in, pending discussion about direction for benchmarking.

Fixes #242.

aadamsx · 2018-09-13T17:10:34Z

Really great PR, thank you!

kevinpschaaf · 2018-09-13T17:29:03Z

src/directives/repeat.ts

      } else {
-        // else part is in the correct position already
-        currentMarker = itemPart.endNode.nextSibling!;
+        const result = newResults[index] = template !(item, index);


Arr, if duplicate keys are detected the index passed here should still reflect the input array items index (as is it reflects the final result/part index...). It's an error condition either way; worth fixing?

how would this change if fixed? template would be called twice with the same item and different index, and the index already is the input array items index, no?

The current behavior for duplicates is that only the first item for a key with duplicates will be included in the results; the rest will be skipped. But to provide the correct index to the template function, we need to increment index for each item regardless; however since we also store the index in the key map, that one needs to reflect the index taking into account skips -- which is just newKeyToIndexMap.size

justinfagnani · 2018-09-13T17:33:03Z

Very exciting :)

Should keying be promoted to the first-class interface for these objects?

I had a keyValue property on Part for a little bit when I was refactoring the Part API. I think it's likely that this will be generally useful. unsafeHTML already stores a separate value on parts, and #458 adds a _promise field to NodePart to check Promise identity. The big question for me is whether having a general value composes - ie, what happens when we have a repeat of promises of unsafeHTML values. Naively it seems like they'd all compete for the key field.

Even without a common keyValue field, we should check that Promises work, to address #193

I made helper API's for the part operations needed in repeat (create/update/move/remove); these should probably somehow be folded into the Part API, but wanted to discuss first since creation/ownership of the markers seems to be a case-by-case basis right now?

Yeah, the current insert/append methods are somewhat ad hoc, and have a restriction that the parts are empty. Now's the time to figure this out and lock it down before 1.0. The PR can be the forcing function for that.

duplicate keys, which we treat as an error and warn as "undefined behavior",

I like that. It fixes #362

Let's also discuss benchmarks

I'm working on this again right now. So far I'm mostly on the launcher / record results side of things, and not the specific benchmark implementation, or client-runner (so much at least).

kevinpschaaf · 2018-09-13T18:18:16Z

The big question for me is whether having a general value composes

Hmm, in my mind key is not an type of value, rather it goes along with a value to give it an identity (so that it can be later diffed) -- seems cleaner to keep those two things as separate, first-class properties.

But I think I see your point; since the same NodePart is passed to potentially multiple nested directives, there's a risk of directives stomping on keys (I think I had naively assumed directives spawned new nested NodeParts, but clearly not; they're just run on the NodePart they were set into, which makes sense).

It also points out a mistake I think I made, assuming that the ItemTemplate fn had to return a TemplateResult, when it can actually return any value. So assuming .key can be assigned to results here isn't good, since those may be e.g. directives waiting to be run (actually that would probably work ok, but e.g. assigning key to a primitive value would not work; but maybe that's super unlikely?).

src/directives/repeat.ts

sorvell · 2018-09-13T17:42:51Z

src/directives/repeat.ts

+
+// Helper functions for manipulating parts
+// TODO(kschaaf): Refactor into Part API?
+function createPart(


This does "create" and "update" and "insert" so the name is misleading. What about preparePart?

We could add insertIntoPart to the Part API and a lot of the mucking with nodes would go away

For this and all the other comments about the part helpers, I'm assuming this is throwaway and will be refactored into a better Part API, but want to have an IRL conversation about that...

sorvell · 2018-09-13T18:13:12Z

src/directives/repeat.ts

+  part.commit();
+  return part;
+}
+function movePart(


This would also use insertIntoPart(on the child) or could be insertBefore or insertBeforePart (on the parent to match the DOM api). It'd have to work on a "live" part though.

sorvell · 2018-09-13T18:18:47Z

src/directives/repeat.ts

      } else {
-        // else part is in the correct position already
-        currentMarker = itemPart.endNode.nextSibling!;
+        const result = newResults[index] = template !(item, index);


format (npm run format). The space after template is weird and hopefully goes away.

This is the result of npm run format. 🤷‍♂️

you shouldn't need the ! though, template isn't typed to be nullable.

It was there in the original source. I think it's needed because of the varargs shenanigans. The template symbol is the 3rd arg and it's optional, and TypeScript isn't smart enough to follow the arguments.length condition for its assignment?

src/directives/repeat.ts

sorvell · 2018-09-13T19:10:48Z

src/directives/repeat.ts

+      } else if (oldStartPart.key == newEndResult.key) {
+        // Old head matches new tail; update and move to new tail
+        newParts[newEndIndex] = updatePart(oldStartPart, newEndResult);
+        movePart(directivePart, oldStartPart, newParts[newEndIndex + 1]);


oldStartPart === newParts[newEndIndex]. Since the old part is irrelevant, moving the new one would be easier to understand (similar in next condition as well).

Hmm. My mental model was "update and (possibly) move the old part to the new spot", so the operations are all about making old parts correct in the new order.

src/directives/repeat.ts

sorvell · 2018-09-13T19:24:03Z

src/directives/repeat.ts

+      } else {
+        // Any mismatches at this point are due to additions or moves; see if
+        // we have an old part we can reuse and move into place
+        const oldIndex = oldKeyToIndexMap.get(newEndResult.key);


Is it important to go bottom-up from here? If not, changing to top-down is simpler.

Yeah, I can flip it back to being top-down. Experimenting with the LIS algorithm made bottom-up easier, but I pulled that out.

src/directives/repeat.ts

justinfagnani

First pass...

src/directives/repeat.ts

justinfagnani · 2018-09-13T23:58:33Z

src/directives/repeat.ts

+    //   worth the code at this point, but could consider if a compelling case
+    //   arises.
+    while (oldStartIndex <= oldEndIndex && newStartIndex <= newEndIndex) {
+      if (oldStartPart == null) {


Is this check equivalent to if (oldStartIndex > oldEndIndex)? Or is there a later assignment to oldStartPart to null it out?

Right, spots in the old array are nulled out as they are moved in the last else clause. Steve had a similar suggestion to add a comment that that's what's going on, since the moves come at the end and a new reader sees them out of order.

src/directives/repeat.ts

kevinpschaaf · 2018-09-15T06:37:12Z

Covered most of the feedback in latest update.

TODOs:

Haven't gotten around to rationalizing the NodePart API with the various helper methods here, but still plan to take a whack at that
Add more tests:
- Test different types returned from item template, including Promises (as suggested)
- Test objects as items rather than just primitives (especially if we decide to change default key as discussed above))
Perf test A/B between before this change and after; the code actually reads cleaner having a separate array of keys and not keeping head/tail references (only indices), but concerned there's more array dereferencing going on
The build failure is because unknown (introduced as suggested) is a new type in TypeScript 3.0 and the devDependencies is still on 2.8.3. @justinfagnani any concern upgrading to 3?

kenchris · 2018-09-17T14:01:11Z

src/directives/repeat.ts

+    let newTail = newResults.length - 1;
+
+    // Overview of O(n) reconciliation algorithm (general approach based on
+    // ideas found in ivi, vue, snabdom, etc.):


its snabbdom with two b's

kenchris · 2018-09-17T14:17:31Z

src/directives/repeat.ts

+    //                    newHead ^  ^ newTail
+    //
+    // * If either the new or old pointers move past each other then all we have
+    //   left is additions (if old list exhsusted) or removals (if new list


kenchris · 2018-09-17T14:18:04Z

src/directives/repeat.ts

+    //
+    // * Note that for moves/insertions, a part inserted at the head pointer
+    //   is inserted before the current `oldParts[oldHead]`, and a part inserted
+    //   at the tail pointer is inserved before `newParts[newHead+1]`.


kenchris · 2018-09-17T14:18:28Z

src/directives/repeat.ts

+    // * Note that for moves/insertions, a part inserted at the head pointer
+    //   is inserted before the current `oldParts[oldHead]`, and a part inserted
+    //   at the tail pointer is inserved before `newParts[newHead+1]`.
+    //   The seeming asymetry lies in the fact that new parts are moved into


asymmetry* (two m's)

kenchris · 2018-09-17T14:18:58Z

src/directives/repeat.ts

+    //   algorithm, as long as the null checks come first (to ensure we're
+    //   always working on valid old parts) and that the final else clause
+    //   comes last (since that's where the expensive moves occur). The
+    //   order of remaining clauses is is just a simple guess at wich cases


kenchris · 2018-09-17T14:22:31Z

Would something like this work with a virtual list where only a part of the data is visible on screen (DOM nodes exist)

kevinpschaaf · 2018-09-17T16:46:14Z

@kenchris It all depends on what the strategy of the virtual list is.

Typically "keyed" updates mean "always keep this DOM associated with this key" (corollary: never reuse with another key). In the virtual lists we've done in the past, usually the cost of reusing (aka updating the bindings with a different item) for a small number of (visible) items is less than the cost of maintaining a strict key-to-DOM contract which at the limit means throwing out and re-creating the visible DOM, possibly per frame when scrolling quickly. If you eliminate the key-to-DOM contract, the algorithm can be much simpler (simply re-assign visible items to the physical rows in order), and this is usually best for scrolling speed, at the cost of more constraints on the user (e.g. list components must be stateless). But yeah, if you were to do a virtual list where you wanted to maintain strict key-to-DOM contract, an algorithm like this would probably be good.

Also, thanks for the spelling fixes; I just installed a comment spellchecker for my vscode!

Also rename result->value, update comments.

kevinpschaaf · 2018-09-20T17:09:57Z

@justinfagnani @sorvell As discussed, I will start on the NodePart API refactor in a separate PR. Can you take another look at this?

justinfagnani

This looks really amazing!

A few things before merging:

Update the changelog
Update the repeat docs as necessary. I think mentioning duplicate keys, and the non-keyed behavior would be good.
Add a test for duplicate keys, if only so we know what we expect to happen now?

justinfagnani · 2018-09-20T17:23:21Z

src/directives/repeat.ts

+      newPart.insertAfterNode(startNode);
+      return newPart;
+    };
+const updatePart = (part: NodePart, value: unknown) => {


style nit: add a newline between functions

justinfagnani · 2018-09-20T17:27:30Z

src/directives/repeat.ts


+// Stores previous ordered list of  parts and map of key to index


two spaces between "of" and "parts"

justinfagnani · 2018-09-20T17:29:26Z

src/directives/repeat.ts

+ * DOM will always be created for new keys).
+ *
+ * If no `keyFn` is provided, this directive will perform similar to mapping
+ * items to values, and DOM will be reused against potentially different items.


cc @katejeffreys and @arthurevans for future work to describe this in a user-understandable way :)

justinfagnani · 2018-09-20T17:51:09Z

src/directives/repeat.ts

+    //   pointer range and never visited again.
+    //
+    // * Example below: Here the old tail key matches the new head key, so
+    //   the part at the `oldTail` position and move its dom to the new


justinfagnani · 2018-09-20T17:53:40Z

src/directives/repeat.ts

+        oldTail--;
+        newHead++;
+      } else {
+        if (newKeyToIndexMap! === undefined) {


Do you need the !?

justinfagnani · 2018-09-20T17:56:04Z

src/directives/repeat.ts

+          // we have an old part we can reuse and move into place
+          const oldIndex = oldKeyToIndexMap!.get(newKeys[newHead]);
+          const oldPart = oldIndex !== undefined ? oldParts[oldIndex] : null;
+          if (oldPart == null) {


justinfagnani · 2018-09-20T17:57:01Z

src/directives/repeat.ts

+    let newHead = 0;
+    let newTail = newValues.length - 1;
+
+    // Overview of O(n) reconciliation algorithm (general approach based on


this comment is all kinds of awesome

sorvell

The docs are awesome. Just a couple nits.

sorvell · 2018-09-20T17:25:12Z

src/directives/repeat.ts

 export function repeat<T>(
-    items: T[], keyFn: KeyFn<T>, template: ItemTemplate<T>):
+    items: Iterable<T>, keyFn: KeyFn<T>, template: ItemTemplate<T>):


If we keep keyFn optional, it's simpler if it's the last argument.

I'd rather do that as a separate change, maybe release with any other breaking changes we have at the time. Let's get this repeat() in the hands of current users and make sure it's a drop-in replacement.

src/directives/repeat.ts

sorvell · 2018-09-20T18:05:58Z

src/directives/repeat.ts

+          // we have an old part we can reuse and move into place
+          const oldIndex = oldKeyToIndexMap!.get(newKeys[newHead]);
+          const oldPart = oldIndex !== undefined ? oldParts[oldIndex] : null;
+          if (oldPart == null) {


sorvell · 2018-09-20T18:10:01Z

src/directives/repeat.ts

-      keyMap.forEach(cleanMap);
+    // Add parts for any remaining new values
+    while (newHead <= newTail) {
+      const newPart = createAndInsertPart(containerPart, oldParts[oldHead]!);


Can you insert before newParts[newTail] here? Using the oldParts as anchors is confusing so it'd be nice to only doing it when we have.

The newTail has already been advanced and so it would actually be newParts[newTail+1].

But, I already described the rules for where to insert in both cases here: https://github.com/Polymer/lit-html/pull/501/files#diff-fa22dc099475a72fdfd85c634894b896R202

Even though we're inserting at the "head", the above is only possible since the old parts are out of the picture at this point in the algorithm, so it would be a special difference that would need to be explained, and I think I'd prefer to keep the rule simple as documented: head goes at oldParts[oldHead], tail goes at newParts[newTail+1]

ok, sounds reasonable, but aren't we concerned that oldParts[oldHead] might be nulled out? That would make the items out of order. It seems like newParts[newTail+1] is always safe.

sorvell

Needs changelog but otherwise LTGM

stramel · 2018-09-28T04:13:30Z

I know you guys mentioned some perf benchmarking was being done. Do you have any number you can show for this PR compared to existing?

Great work @kevinpschaaf 👏

CaptainCodeman · 2018-09-28T15:31:04Z

Here's one based a this branch a few commits ago:

before

after

That's using js-framework-benchmark

stramel · 2018-09-28T20:18:34Z

Wow, that's fantastic! Excellent work everyone! Thanks for the benchmark @CaptainCodeman

Rework repeat directive for better performance. Fixes #242.

7b444db

kevinpschaaf requested a review from justinfagnani as a code owner September 13, 2018 17:01

kevinpschaaf added 3 commits September 13, 2018 10:05

Merge branch 'master' into 242-repeat-perf

814828d

Revert change to varargs handling (vestige of playing with options arg)

2404a15

Fix merge conflict.

9844371

Format.

040f3ee

kevinpschaaf requested a review from sorvell September 13, 2018 17:26

kevinpschaaf commented Sep 13, 2018

View reviewed changes

sorvell requested changes Sep 13, 2018

View reviewed changes

justinfagnani requested changes Sep 14, 2018

View reviewed changes

kevinpschaaf added 3 commits September 14, 2018 22:48

Updates based on review feedback.

9039908

Format.

0ffcbb3

Remove unused import.

e73c340

kenchris reviewed Sep 17, 2018

View reviewed changes

kevinpschaaf added 6 commits September 17, 2018 18:03

Fix typos and beef up comments more.

3bbe9ff

Upgrade to latest TypeScript

e6a5e1e

Format.

c587764

Typo.

0294fef

Add tests for repeating objects and directives.

1c4b216

Fix typo in docs.

ef109c9

Add back lazy generation of key maps for perf.

e6b5ec5

Also rename result->value, update comments.

justinfagnani requested changes Sep 20, 2018

View reviewed changes

sorvell requested changes Sep 20, 2018

View reviewed changes

kevinpschaaf added 5 commits September 20, 2018 11:39

Minor updates based on review.

c49a0d6

Add note about importance of unique keys.

eef6604

Format.

94637fa

Fix insertion point for final adds and add a test.

96dba60

Fix nits. Add tests for duplicate keys.

e93ec16

sorvell approved these changes Sep 25, 2018

View reviewed changes

kevinpschaaf added 2 commits September 25, 2018 17:07

Merge branch 'master' into 242-repeat-perf

981e791

Update changelog

c179b30

justinfagnani approved these changes Oct 1, 2018

View reviewed changes

justinfagnani merged commit 72ba655 into master Oct 1, 2018

justinfagnani deleted the 242-repeat-perf branch October 1, 2018 19:39

stramel mentioned this pull request Oct 4, 2018

Performance #536

Closed

kevinpschaaf mentioned this pull request Nov 27, 2018

repeat Should warn on duplicate ids #362

Closed

kevinpschaaf mentioned this pull request Aug 9, 2019

Adds restamp mode to dom-repeat. Polymer/polymer#4363

Merged

neuronetio pushed a commit to neuronetio/lit-html that referenced this pull request Dec 2, 2019

Rework repeat directive for better performance. Fixes lit#242. (lit#501)

cb5a3ee

terrorizer1980 mentioned this pull request Sep 9, 2024

[Snyk] Upgrade: lit-html, approximate-number, twitter-text terrorizer1980/twitter-user#4

Open


		// Stores previous ordered list of parts and map of key to index

Rework repeat directive for better performance. Fixes #242. #501

Rework repeat directive for better performance. Fixes #242. #501

Conversation

kevinpschaaf commented Sep 13, 2018 • edited Loading

aadamsx commented Sep 13, 2018

kevinpschaaf Sep 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinpschaaf Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

justinfagnani commented Sep 13, 2018

kevinpschaaf commented Sep 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinpschaaf Sep 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinfagnani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinpschaaf commented Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenchris commented Sep 17, 2018

kevinpschaaf commented Sep 17, 2018 • edited Loading

kevinpschaaf commented Sep 20, 2018

justinfagnani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorvell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorvell Sep 20, 2018 • edited Loading

Choose a reason for hiding this comment

sorvell left a comment

Choose a reason for hiding this comment

stramel commented Sep 28, 2018 • edited Loading

CaptainCodeman commented Sep 28, 2018

stramel commented Sep 28, 2018

kevinpschaaf commented Sep 13, 2018 •

edited

Loading

kevinpschaaf Sep 13, 2018 •

edited

Loading

kevinpschaaf Sep 15, 2018 •

edited

Loading

kevinpschaaf Sep 13, 2018 •

edited

Loading

kevinpschaaf commented Sep 15, 2018 •

edited

Loading

kevinpschaaf commented Sep 17, 2018 •

edited

Loading

sorvell Sep 20, 2018 •

edited

Loading

stramel commented Sep 28, 2018 •

edited

Loading