Reuse updated nodes and contexts.

[AllSeeingEye]

This is the initial attempt to avoid costly add/remove DOM operations by reusing DOM nodes and the respective context(s), as discussed in #37.

This patch still relies on the arrayChange; script now detects add/delete sequence on the same index and replaces it with a single 'update' operation. This, however, causes issues with some tests that rely on DOM node identity - please see my changes to spec\knockout-fast-foreach-spec.js (I wasn't able to fix all the test issues, unfortunately).

In the React/Angular/Knockout/raw test:

https://brianmhunt.github.io/chrisharrington-performance.html

this new version is about 9 times faster (for the update operation, not for the initial array deployment; 5000 elements) than the current master branch code.

Future work: we should probably reuse all the possible nodes, only adding/removing the tail ones as necessary (and caching nodes, maybe?).

[cervengoc]

Hello @AllSeeingEye, thank you for your work. At first glance the idea seems to be good, however I have some worries, basically two.

First, I don't know if it's supported to change an existing bindingContext like this. I mean just simply setting the $data and the $rawData members. It seems to be dangerous and I have a feeling that it can have unwanted side-effects. But honestly I'm not so competent in deep knockout behavior, so @brianmhunt you should check this too please.

Second, reusing the active nodes of a previous bindingContext is not so simple in more complex scenarios. I don't have time currently to think it over, but please consider this following use case.

<ul data-bind="fastForEach: Items">
  <li>
    <span data-bind="text: Caption"></span>
    <div data-bind="visible: Details, with: Details">
      <span data-bind="text: DetailCaption"></span>
      ... some other valuable nodes
    </div>
  </li> 
</ul>

The key here is the with binding. If the Detail child object of an item is null, the with binding will remove the nodes. But I don't think it will restore them on ko.cleanNode. So next time you want to apply the item bindings, you'll loose the content nodes of the div with the with binding. This might not be true at all, it's just an assumption, and a scenario for you to think about.

The problem is not only and not exactly with the with binding, but with the fact, that several bindings (especially custom ones, but also some built-in) will manipulate the nodes dynamically based on their data, and they won't clean up after themselves when calling ko.cleanNode, but only when the node is removed (see the domNodeDisposalCallback feature).

So basically while I like this idea, I think it's suitable only for simple cases, where one uses a constant node set as the fastForEach template, which is always guaranteed to be just simply rebindable without loosing its integrity. Still, it would probably make sense to introduce this as an opt-in behavior for thses scenarios, because the performance boost can indeed be great.

[brianmhunt]

@cervengoc Yep, behaviour when modifying bindingContext would be not well tested for many bindings. I would classify this as "experimental".

The nested re-use is definitely an issue, so I would definitely consider this an optional parameter (defaulting to false/off).

The afterAdd test would also need to be fixed.

That said, the performance improvement is pretty spectacular, so if it can be done - even for optional simple cases that would make some people's lives great.

If we go down the optional flag path, what would be a good name for the flag? reuseBindingContext?

[brianmhunt]

An alternative, (that may or may not be) worth considering, is forking fast-foreach and creating another binding for simple cases, that extends the FastForEach class. Just in case we're getting too complex here with experimental options / etc. The big advantage is that you wouldn't have to duplicate every test with the flag enabled (which is something perhaps needed to ensure that it works as expected in all the same cases as fast-foreach).

[cervengoc]

I like the fork idea, I was thinking about that too, following the pattern of the already discussed with and using (aka. "light" with) binding, which deals with somewhat similar concepts.

If with / using will make it to release, then we should probably name it also as foreach and foreachUsing.

[brianmhunt]

The with / using bindings are in tko.

I think foreachUsing works, unless we thing of something cleverer. 😄

[brianmhunt]

What's the issue here?

[AllSeeingEye]

This one is strange. I think that test script doesn't catch up with the rendering speed (?), as I get different assertion results every time - it is in the range of 97-100. Gimme a bit of time to look into this.

[AllSeeingEye]

Test passes if I wrap the 2nd part of the script (everything after console.timeEnd("with move");) with a setTimeout(function() { ... }, 1). What do you think of this?

[brianmhunt]

We'll want to re-enable this before merging (unless there's a compelling reason to disable it)

[AllSeeingEye]

The very last assertion in this test:

assert.equal(nodes, 4, 'n4')

needs to be rewritten as:

assert.equal(nodes, 3, 'n4')

This fixes the test.

[AllSeeingEye]

@cervengoc: thank you for your comment; I felt that handling the binding context(s) the way I did was dangerous - I just wanted to get an input on this. I've moved to a safer code in my latest commits.

I need to think more on your comment on binding update/cleanup, thanks for the heads up.

[Scarbous]

Thanks, that solved my flickering problem.
Why is it still not merged?