Fix keyed list ordering issue

[totorigolo]

Fixes: #1220

I took the opportunity to document the code as I was reading it and trying to understand how it works, and to refactor some bits as well.

For the fix by itself, look for Reform::Keep if self.key.is_some().

Still a draft because I would want to investigate performance issues: keyed list feels* slower. To reproduce, just run the new example, add a lot of persons and compare the time it takes to sort the persons when it's keyed or not.

*feels: I tried to measure it using Firefox Performance tab in the Dev Tools, but I cannot find where to read the time a box takes in the JS Flame Chart view.

[jstarry]

Interesting, I'll take a look at this tomorrow. Thanks for diving in! I've been working on updating the Yew example in https://github.com/krausest/js-framework-benchmark and will push a branch that we can do perf testing on

[totorigolo]

Benchmarks would be ideal. I will try to write micro-benchmarks if I have time.

However, I had an idea of better implementation this morning when hiking (true story xD), I'll try it now. The fix so far is pretty dumb an inefficient.

[jstarry]

@totorigolo I just spent some time reviewing the code and I believe the main issue is how eagerly we call detach(). Every keyed right gets detached which is very problematic for a keyed vcomp node. Vcomp nodes destroy the component when detached. Detaching vtag nodes is problematic too because they will recursively detach all of their children.

We need a way to move a vnode without calling detach. So I think we need to add a method to vnode allowing us to get a handle on its dom node which would allow us to move it around.

Thoughts?

[totorigolo]

@jstarry I agree that detach() was called way too often. I fixed it and keyed list has almost the same performance than un-keyed one. I say almost because the example I wrote is simple and it seems that since all the items are simple vtags with the identical same structure, it's faster to change the text of the DOM elements than to move the nodes around. We need an example with heterogeous children of different complexity to see if keys really work.

EDIT: Also, what do you think about the other changes, key as Rc<String> and using ahash? I'd said that we need benchmarks to see if they are interesting.

[totorigolo]

Reordering with both vtags and vcomp is... buggy with keyed components 😇

[jstarry]

Yikes! We should definitely have tests to cover all the combinations. Lists can be keyed as well :)

[jstarry]

I didn't find time today to get the benchmarks setup today. Should be able to get to it tomorrow and can help out on tests if needed

[jstarry]

Here's the branch for updating yew for the JS benchmarks project: https://github.com/jstarry/js-framework-benchmark/tree/yew-update and here's a branch that uses keys (with this PR's fixes): https://github.com/jstarry/js-framework-benchmark/tree/keyed-yew

[totorigolo]

@jstarry Could you have a look? Manual testing seems to show that the diffing is correct. We definitely need unit tests, but I am too lazy to write them (and also late on a project of mine). Maybe later 😇

Performance-wise, it's ok in some situations and bad in others. This can be visualized by putting breakpoints in Firefox DevTools for insert_before and append_child. Swapping elements is bad for instance. I have ideas to improve this, but I feel that this branch is already too old and I would like to finish this before going into performance fixes.

---

Various ideas for performance wins:

• avoid detaching all VList children one by one, but remove the parent at once from the DOM.
• find and fix the (memory?) leak in Yew, which can be noticed in the benchmarks.

[jstarry]

Good call, performance fixes are more fun when you don't have the weight of a huge PR on top. Correctness first, optimization later 👍

[totorigolo]

About the potential memory leak I mentioned, I noticed it in the benchmark "clear rows - clearing a table with 1,000 rows. 8x CPU slowdown", where Yew has terrible results.

wasm-bindgen-v0.2.47-keyed	react-v16.8.6-keyed	react-v16.8.6-non-keyed	seed-v0.6.0-non-keyed	yew-v0.16.2-non-keyed
207.8 +- 5.7(1.00)	279.9 +- 8.1(1.35)	301.9 +- 12.3(1.45)	371.8 +- 5.4(1.79)	27,844.8 +- 17555.6(134.01)

Yew 0.7.0 doesn't seem to have this issue though. If anyone wants to confirm this and have a look 🙂

[jstarry]

😱

[jstarry]

Good work @totorigolo!

3 main things:

1. I think we should try to avoid needing VDiffNodePosition::After and VDiffNodePosition::FirstChild so that we can cut down on DOM API calls.

2. We should make sure we are matching as many keyed list items as we can.

3. Keyed vlists look pretty tricky. Maybe we can remove support for them temporarily?

[jstarry]

We are going to have to rethink this since lists can be keyed themselves. Imagine you have a list of keyed lists that need to be re-ordered 😵

[totorigolo]

I mitigated the issue, see the last commit (16ffa90). What do you think?

[jstarry]

I'm not a big fan of the side effect. What if we returned the reference of the first list element here? When the list is empty, it will return the placeholder element

Pull request has been modified.

[jstarry]

@totorigolo I noticed that krausest/js-framework-benchmark has a restriction for how keyed lists work. It requires that frameworks do not reuse keyed elements if they key is no longer in use. I think it makes sense for Yew to behave that way by default, what do you think?

[totorigolo]

@jstarry I think that it makes sense for keyed elements. However, this imposes quite a few changes to the current reconciliation algorithm:

[jstarry]

Almost there! I think test organization is going to be really important to make sure we are handling all the edge cases. Could you try following the precedent I set in layout_tests::diff?

[totorigolo]

Thanks a lot @jstarry for finishing this! 🙂

[jstarry]

Thanks a lot for driving this effort, it was great working with you on this @totorigolo!