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

Fix updateNodes() (mostly vdom diffing logic). #2021

Merged
merged 10 commits into from Dec 4, 2017

Conversation

Projects
None yet
6 participants
@pygy
Copy link
Member

pygy commented Nov 21, 2017

Edit: this is done AFAICT

Fix for #1990, #1991 and #2003

The logic in updateNodes will still need some love for fragments (the last commit is actually sub-optimal, skipping insertNode when the vnode is a fragment would be better since the children of the fragment will have been inserted at that point. That's a change for tomorrow though.

Few lines of updateNodes are left untouched. Hopefully the individual commits will make the review easier.

No formal review requested now since there will be future changes. If @starsolaris and @purplecode want to test the waters, there's a build at https://github.com/pygy/mithril.js/blob/fix-updateNodes-build/mithril.js

How Has This Been Tested?

So far, mostly manually in Flems, I'll have to cook up test cases. There were quite a few bugs.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md

@pygy pygy requested a review from isiahmeadows as a code owner Nov 21, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 22, 2017

The logic in updateNodes will still need some love for fragments (the last commit is actually sub-optimal, skipping insertNode when the vnode is a fragment would be better since the children of the fragment will have been inserted at that point.

Actually, the insertNode() business will be part of another PR that will address #1999. I'll add tests here later this evening, but you can start the code review, the six commits that have been pushed won't change.

@pygy pygy requested review from tivac and lhorie Nov 22, 2017

@pygy pygy force-pushed the pygy:fix-updateNodes branch from 80ce1b4 to 58e5466 Nov 22, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 22, 2017

More tests tomorrow (I had to rephrase some commit messages that referenced #1992 instead of #1991). I'll keep on adding tests to that last commit (I'm stuck trying to create a minimal repro for the leftover 4 in my version of #1991 and it's getting a bit late).

@pygy pygy force-pushed the pygy:fix-updateNodes branch from c32c2ed to df027f4 Nov 22, 2017

o(onupdate.callCount).equals(0)
})

o("uneyed cached elements are re-initialized when brought back from the pool", function () {

This comment has been minimized.

@StephanHoyer

This comment has been minimized.

@pygy

pygy Nov 23, 2017

Member

thanks!

@pygy pygy force-pushed the pygy:fix-updateNodes branch from df027f4 to 3a3f748 Nov 23, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 23, 2017

The tests are in. You can also watch them fail in v1.1.5.

@pygy pygy force-pushed the pygy:fix-updateNodes branch 3 times, most recently from d6f5191 to e1624b7 Nov 23, 2017

@pygy pygy changed the title Fix updateNodes() (WIP, still needs a few tests and some more tweaks) Fix updateNodes() Nov 23, 2017

@pygy pygy changed the title Fix updateNodes() Fix updateNodes() (vdom diffing logic). Nov 23, 2017

@spacejack

This comment has been minimized.

Copy link
Contributor

spacejack commented Nov 23, 2017

Just a thought, and I don't want to pile more tasks on top of the great work you're already doing, but I think it would be great if you could sprinkle a few more comments/notes in the code, especially now that you've got a map of it in your head and it's still fresh.

I know that when I'm writing some tricky procedural logic I need to make notes otherwise it'll fade from memory very quickly. Plus I think it would make it more approachable for others to get involved.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Nov 23, 2017

@pygy Yeah, I'm with @spacejack here: if you could put some comments in that code, it'd be perfect. That way, we don't run into quite the mess we had when we started.

(Or, in other words, let's try not to repeat history again, while we still remember it. 😄 )

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 23, 2017

@spacejack @isiahmeadows That was actually part of the plan, I'll add those later this evening. Just thinking about this made me realize that the way we recycle nodes clashes with the keyed reconciler optimizations... I'll document how as well.

@pygy pygy changed the title Fix updateNodes() (vdom diffing logic). Fix updateNodes() (mostly vdom diffing logic). Nov 23, 2017

pygy added a commit to pygy/mithril.js that referenced this pull request Nov 23, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 23, 2017

There you have it! Rendered here:


This function diffs and patches lists of vnodes, both keyed and unkeyed.

We will:

  1. describe its general structure
  2. focus on the diff algorithm optimizations
  3. describe how the recycling pool meshes into this
  4. discuss DOM node operations.

Overview:

The updateNodes() function:

  • deals with trivial cases
  • determines whether the lists are keyed or unkeyed
    (we take advantage of the fact that mixed diff is not supported and settle on the
    keyedness of the first vnode we find)
  • diffs them and patches the DOM if needed (that's the brunt of the code)
  • manages the leftovers: after diffing, are there:
    • old nodes left to remove?
    • new nodes to insert?
    • nodes left in the recycling pool?
      deal with them!

The lists are only iterated over once, with an exception for the nodes in old that
are visited in the fourth part of the diff and in the removeNodes loop.

Diffing

There's first a simple diff for unkeyed lists of equal length that eschews the pool.

It is followed by a small section that activates the recycling pool if present, we'll
ignore it for now.

Then comes the main diff algorithm that is split in four parts (simplifying a bit).

The first part goes through both lists top-down as long as the nodes at each level have
the same key. This is always true for unkeyed lists that are entirely processed by this
step.

The second part deals with lists reversals, and traverses one list top-down and the other
bottom-up (as long as the keys match).

The third part goes through both lists bottom up as long as the keys match.

The first and third sections allow us to deal efficiently with situations where one or
more contiguous nodes were either inserted into, removed from or re-ordered in an otherwise
sorted list. They may reduce the number of nodes to be processed in the fourth section.

The fourth section does keyed diff for the situations not covered by the other three. It
builds a {key: oldIndex} dictionary and uses it to find old nodes that match the keys of
new ones.
The nodes from the old array that have a match in the new vnodes one are marked as
vnode.skip: true.

If there are still nodes in the new vnodes array that haven't been matched to old ones,
they are created.
The range of old nodes that wasn't covered by the first three sections is passed to
removeNodes(). Those nodes are removed unless marked as .skip: true.

Then some pool business happens.

It should be noted that the description of the four sections above is not perfect, because those
parts are actually implemented as only two loops, one for the first two parts, and one for
the other two. I'm not sure it wins us anything except maybe a few bytes of file size.

The pool

old.pool is an optional array that holds the vnodes that have been previously removed
from the DOM at this level (provided they met the pool eligibility criteria).

If the old, old.pool and vnodes meet some criteria described in isRecyclable, the
elements of the pool are appended to the old array, which enables the reconciler to find
them.

While this is optimal for unkeyed diff and map-based keyed diff (the fourth diff part),
that strategy clashes with the second and third parts of the main diff algo, because
the end of the old list is now filled with the nodes of the pool.

To determine if a vnode was brought back from the pool, we look at its position in the
old array (see the various oFromPool definitions). That information is important
in three circumstances:

  • If the old and the new vnodes are the same object (===), diff is not performed unless
    the old node comes from the pool (since it must be recycled/re-created).
  • The value of oFromPool is passed as the recycling parameter of upateNode() (whether
    the parent is being recycled is also factred in here)
  • It is used in the DOM node insertion logic (see below)

At the very end of updateNodes(), the nodes in the pool that haven't been picked back
are put in the new pool for the next render phase.

The pool eligibility and isRecyclable() criteria are to be updated as part of #1675.

DOM node operations

In most cases updateNode() and createNode() perform the DOM operations. However,
this is not the case if the node moved (second and fourth part of the diff algo), or
if the node was brough back from the pool and both the old and new nodes have the same
.tag value (when the .tag differ, updateNode() does the insertion).

The fourth part of the diff currently inserts nodes unconditionally, leading to issues
like #1791 and #1999. We need to be smarter about those situations where adjascent old
nodes remain together in the new list in a way that isn't covered by parts one and
three of the diff algo.

@isiahmeadows
Copy link
Collaborator

isiahmeadows left a comment

So far, so good. I like the look of the updateNodes function now - it's much easier to comprehend now.

@spacejack

This comment has been minimized.

Copy link
Contributor

spacejack commented Nov 24, 2017

@pygy if this is an up to date build: https://github.com/pygy/mithril.js/tree/fix-updateNodes-build I can test it in some apps I'm working on.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 24, 2017

@spacejack I just updated it, it was missing e1624b7 ("render: remove check that may hide bugs").

Edit: Thanks for the offer BTW, please note that this is a v2 build, with different mount/route/redraw timings. I'll backport the changes to the v1 branch once we merge this (and add #1675) and the new events thing to release v1.2, it would probably be less risky to base your testing on that branch.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 24, 2017

The Flems env wasn't properly set up (I wasn't removing root.vnodes between runs), leading to crashes in the last four tests rather than actual assertion errors (tests for mixing the pool and userland-cached nodes). The problem cropped up in the flems I cooked up for review here, but the tests are correct (i.e. failing in v1.1.5 for the reason I expected and fixed after this PR).

Fixed here.

@StephanHoyer

This comment has been minimized.

Copy link
Member

StephanHoyer commented Nov 27, 2017

just tried to use you newest version for the FLIP example here and it does not quite work, where with v 1.1.3 it works

@StephanHoyer

This comment has been minimized.

Copy link
Member

StephanHoyer commented Nov 27, 2017

sorted version seems to work with you fixes, random one still has issues

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Nov 28, 2017

@StephanHoyer @pygy Could you see if you all could reduce the FLIP issues to a test case? That might make it easier to spot. (Also, having false negatives in the tests is really, really bad... 😟)

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Nov 28, 2017

@StephanHoyer what's the "random" version? It should be sorted out with the ivi-like diffing I'll add in the coming days anyway (mostly, ivi doesn't support fragments, minimizes the DOM operation based on the sequence of vnodes, that all map to one DOM element... I think I'll ignore that for a start).

@isiahmeadows what do you mean by false negative? The tests in the Flems? The real test suite uses a distinct root for each test, but the Flems is reusing document.body each time, and I had forgotten to remove document.body.vnodes leading to the issues I mentioned above (where m.render was trying to remove old nodes that had been manually wiped out).

The tests are fine, don't worry, it was just a bad harness.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Dec 3, 2017

@isiahmeadows @lhorie @tivac Can we merge this?

@tivac

tivac approved these changes Dec 3, 2017

Copy link
Member

tivac left a comment

I am nowhere near competent to review the actual logic changes on a super-deep level, but based on the comments & a cursory reading it all seems fine. Tests look reasonable as well.

I say :shipit:

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Dec 3, 2017

@pygy I'll try to do a proper review this time. 😉

@isiahmeadows
Copy link
Collaborator

isiahmeadows left a comment

LGTM mod a couple small nits

@@ -161,86 +161,215 @@ module.exports = function($window) {
}

//update
function updateNodes(parent, old, vnodes, recycling, hooks, nextSibling, ns) {
if (old === vnodes || old == null && vnodes == null) return
/**

This comment has been minimized.

@isiahmeadows

isiahmeadows Dec 3, 2017

Collaborator

I seriously appreciate this nice comment. I'm familiar with the argument order from having toyed with it for a while, but this is a nice little explanatory landmark that will make our lives much easier.

// - deals with trivial cases
// - determines whether the lists are keyed or unkeyed
// (we take advantage of the fact that mixed diff is not supported and settle on the
// keyedness of the first vnode we find)

This comment has been minimized.

@isiahmeadows

isiahmeadows Dec 3, 2017

Collaborator

Just to clarify, is this actually documented, or is this comment it? (BTW, I was already aware this was the case; I was just wondering.)

This comment has been minimized.

@pygy

pygy Dec 3, 2017

Member

IIRC it is "undefined behavior" in the sense that it isn't discussed in the docs, intentionally. But see #2046... Previously, two non-null, unkeyed nodes were required to deem a list unkeyed for sure. I'll probably backtrack here for the v1 branch.

//
// It should be noted that the description of the four sections above is not perfect, because those
// parts are actually implemented as only two loops, one for the first two parts, and one for
// the other two. I'm not sure it wins us anything except maybe a few bytes of file size.

This comment has been minimized.

@isiahmeadows

isiahmeadows Dec 3, 2017

Collaborator

Note for later: we should investigate whether splitting up the loops would actually help us performance-wise. Sometimes, in simpler cases, we could get some serious wins out of multiple smaller loops, but not always.

(Nothing actually needs to change until we do some benchmarking.)

This comment has been minimized.

@pygy

pygy Dec 3, 2017

Member

I've been researching diff algos, and studying ivi in particular since it is performant and has a nice explanation of what it does to minimize the amount of DOM operations. I've also chatted a bit with @localvoid about this, he's been really helpful.

We'll have to split the keyed/unkeyed diff completely.

In keyed mode, you must tag each node in the new list with the position of its previous incarnation in the old list, marking new nodes as -1. So going from a b c d to d b a c gives us 4 2 1 3. In that list, the longest increasing sequence (LIS, 1 3, so a c) must be left untouched, and the other nodes must be moved. There's a general algo to determine the LIS that is O(N log(N)), but there strategies that allow to defer generating the sequence and to narrow down the length of the sublists on which it must be computed:

As long as the keys match, a linear search from both ends can update the nodes in place, they are guaranteed to be part of the LIS and must not be moved. Also, if the nodes under the cursors from both ends can be swapped (which happens in list reversals, or if only two nodes have been swapped in an otherwise untouched list for example) they are guaranteed not to be in the LIS and they can be swapped safely. In those scenarios, the pool can be ignored since there are no nodes insertions or deletions either.

When that strategy fails you have to generate the key=>position map and the LIS, based on the parts of the lists that still have to be processed, to determine what node has been added/removed, and which one must be moved in the DOM.

There's another tricky issue: fragments. ivi doesn't support them, but we do. If we compute the LIS based on the vnodes, we can end up moving extra DOM nodes.

a b ccc d => ccc a b d: 333 1 2 4, where ccc is a three element fragment. The vnode LIS is 1 2 4, but if you look at DOM nodes if it is 333 4.

We can probably adapt the LIS algo to take the domLength into account but it will be original research AFAIK. Also users may expect to see the fragment move rather than the other two nodes so maybe we could simply ignore the issue (that's what I plan to do initially, actually).

This comment has been minimized.

@isiahmeadows

isiahmeadows Dec 3, 2017

Collaborator

To clarify, I meant splitting it into more than two. Sorry for the ambiguity.

This comment has been minimized.

@pygy

pygy Dec 3, 2017

Member

No problem, I will have to be revamped one way or another. I just wanted to dump those things I've had on my mind, and it seemed vaguely on topic here ;-)

// in three circumstances:
// - If the old and the new vnodes are the same object (`===`), diff is not performed unless
// the old node comes from the pool (since it must be recycled/re-created).
// - The value of `oFromPool` is passed as the `recycling` parameter of `upateNode()` (whether

This comment has been minimized.

@isiahmeadows

isiahmeadows Dec 3, 2017

Collaborator

Nit: minor typo

if (result != null && typeof result.then === "function") {
expected++
result.then(continuation, continuation)
if (!recycling) {

This comment has been minimized.

@isiahmeadows

isiahmeadows Dec 3, 2017

Collaborator

This would read cleaner (and have a much smaller diff) if you did if (recycling) return and dropped the nesting level back to where it was. Also, it could just as easily be done a couple lines above where it is.

This comment has been minimized.

@pygy

pygy Dec 4, 2017

Member

Looking more closely, we must still call the continuation, so we must define expected and called. I've moved the original assignment inside the conditional.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Dec 3, 2017

Note: my review wasn't super in depth, but I have toyed around with the core algorithm some, enough to be dangerous. I still didn't fully grasp it, but I did understand some of the fundamentals of how it worked.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Dec 3, 2017

Thank you both, I'll update the PR based on @isiahmeadows' suggestions (and the #2046 caveat) and will merge it.

@pygy pygy force-pushed the pygy:fix-updateNodes branch from f0cbbcf to 9ae04bb Dec 4, 2017

oFromPool = hasPool && oldStart >= originalOldLength
if (o === v && !oFromPool && !recyclingParent || o == null && v == null) oldStart++, start++
else if (o == null) {
if (isUnkeyed || v.key == null) {

This comment has been minimized.

@pygy

pygy Dec 4, 2017

Member

The old unkeyed detection` may actually fail to detect keyedness if

old === [null, {tag: "x"}] and vnodes === [{tag:'x'}, null], triggering #2003 once again.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Dec 4, 2017

There we go!

@pygy pygy merged commit 9f09ac0 into MithrilJS:next Dec 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

pygy added a commit that referenced this pull request Dec 4, 2017

pygy added a commit to pygy/mithril.js that referenced this pull request Dec 8, 2017

pygy added a commit to pygy/mithril.js that referenced this pull request Dec 8, 2017

@JacksonJN

This comment has been minimized.

Copy link

JacksonJN commented Apr 13, 2018

Hey @pygy, we are running into a couple of these issues in our production code. Just wondering if there are any releases with this fix planned soon ?

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 13, 2018

Hi @JacksonJN a release is long overdue. I don't remember if these patches can be backported to the v1 branch, and I don't have the time to investigate until at least the end of next week.

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this pull request Oct 12, 2018

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this pull request Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment