Riot: When updating a tag with a loop, rendered result doesn't always reflect the updated data

[cdanielw]

The below fiddle recreates the issue. Checking an item should remove it. However, checking any but the last item with riot 2.2.2, while the item is removed, the item below becomes checked. It works fine with 2.1.0 and 2.0.15.

http://jsfiddle.net/cdanielw/bkw5ommu/4/

[danieltian]

The source of this problem is that the checkbox event is still performing its default behavior, leading to the issue you've observed. The fix is to simply add an e.preventDefault() to your toggled(e) function:

https://jsfiddle.net/bkw5ommu/7/

Riot tags will automatically use e.preventDefault() for stuff like links, but it looks like checkboxes isn't one of them (@GianlucaGuarini, bug?).

Edit: not a bug according to the docs linked by @rsbondi.

[r4j4h]

Riot tags will automatically use e.preventDefault() for stuff like links

Is this documented somewhere?

[rsbondi]

https://github.com/riot/riot/edit/master/doc/guide/index.md#L636

[weepy]

What is the reasoning behind why checkboxes aren't automatically handled ?

[danieltian]

@weepy Probably because checking a checkbox/clicking a radio button and having it selected is the behavior you'd want in the majority of cases. With the e.preventDefault(), the checkbox/radio won't get selected, and you'll have to do it manually.

In the case of this issue, I'm just taking a stab at the dark, but what's probably happening is that the toggled() function is running, followed by the this.on('update') listener. Only after that does the default event happen, but the checkbox that it's supposed to be checking has been removed from the DOM, so the event just finds the next child and checks that instead (if it exists).

[cdanielw]

My take on this is that it’s due to the way leftover items are unmounted in each.js – take this with a grain of salt though, since I’m new to Riot.

  var frag = document.createDocumentFragment(),
      i = tags.length,
      j = items.length

  // unmount leftover items
  while (i > j) {
    tags[--i].unmount()
    tags.splice(i, 1)
  }

When an item in the middle is removed, the last tag will be unmounted, not the tag previously bound to the removed item. Then the remaining items are rebound to the tags. This doesn’t take into account that the DOM nodes themselves are stateful. The below fiddle shows this more clearly. Here, the background color is set on the node to be removed.

https://jsfiddle.net/cdanielw/bkw5ommu/8/

[rsbondi]

Calling e.preventDefault is only masking the issue as pointed out in the example by @cdanielw

[oderwat]

We lost a lot of time because of this behavior and had to rewrite a lot of code without fully realizing that this confirms as bug.

[pakastin]

We discussed a lot about this here:
https://github.com/riot/riot/issues/484

[oderwat]

I feared that #484 will be brought up. But that talked imho about the order of the dom nodes. Now we also talk about all their content and total loss of sync between the state in riot and the dom.

That optimization destroyed multiple days of menpower for us as we tried to recover from switching to 2.2 by changing our code. I should have simply not ordered the team to adapt to 2.2. "in any case" and stayed with 2.1 or 2.0.15 which worked much more reliable in that regard.

Now there is no end to it and things are popping up here an there. I think the behavior from the jsfiddle @cdanielw contributed is enough to qualify this as a problem which needs to get fixed. You can't reset all properties in every update all of the time manually just because an entry got deleted.

BTW: Having an optimization which requests to manually bring dom nodes into sync again and again is not worth it. It is always measurably faster to dump core functionality and let it to the implementor to fix that. So either add another each which guarantees sync or fix the current one such that it works as people expect it to work.

[pakastin]

I see this as the same problem when you lose the reference to the DOM elements.

Only way to fix the issue for now is to move all the logic to the template, since you can't trust the JS (DOM and data might lose their relationship in any point).

And again the only way to efficiently keep the DOM references is to introduce a unique id for looping items. But I'm repeating myself here.

[GianlucaGuarini]

@oderwat

That optimization destroyed multiple days of menpower for us as we tried to recover from switching to 2.2 by changing our code. I should have simply not ordered the team to adapt to 2.2. "in any case" and stayed with 2.1 or 2.0.15 which worked much more reliable in that regard.

I am sorry that our optimization did not fit with your project legacy code, could you show us a simple example where this new riot behavior breaks your application I am curious to see what's the problem

Now there is no end to it and things are popping up here an there. I think the behavior from the jsfiddle @cdanielw contributed is enough to qualify this as a problem which needs to get fixed. You can't reset all properties in every update all of the time manually just because an entry got deleted.

from riot 2.0.0 to riot 2.1 anytime a child node was removed all the other expressions where evaluated anyway, so in this case nothing is changed. What do you mean by resetting all the properties just because an entry got deleted? Your components should be in always in sync with the items collection, and you should let riot updating your DOM nodes, without doing it manually

Thank you guys for raising this up, I would like to have more opinions and examples about this riot behavior

[GianlucaGuarini]

@cdanielw riot updates just the expressions in a loop and it does not keep track of the DOM properties, your example could be rewritten in this way (no need of e.preventDefault)
This optimization makes riot one of the fastest framework and maybe it should be better documented, but I am not sure we need to switch back to the older riot looping strategy
check yourself!
current riot in the dev branch
riot 2.1.0

[pakastin]

If you have the following data (in the DOM):

[{id: 1, title: 'Item 1'}, {id: 2, title: 'Item 2'}, {id: 3, title: 'Item 3'}]

..and you remove the first item:

[{id: 2, title: 'Item 2'}, {id: 3, title: 'Item 3'}]

What you'd expect to happen:

• 1 gets removed

What really happens:

• 1 becomes 2 (id: 1 -> id: 2, title: 'Item 1' > title: 'Item 2')
• 2 becomes 3 (id: 2 -> id: 3, title: 'Item 2' > title: 'Item 3')
• 3 gets removed

I think this is the case :)

[GianlucaGuarini]

Yes I see, this could be easily solved getting the index of the items to remove, but how to handle a data set like this:

var items = ['foo', 'bar', 4, 'foo', 3, 4, 3, 'bar']
// update
items = ['foo', 'bar', 4, 3, 3]
// which tag must be removed here? indexOf will get always the first match

The current sync method seems to me the easiest and the most consistent

[oderwat]

As @pakastin says... If you set a style or form input value to any of those and remove element 1. They will suddenly appear as part of entry two.

@GianlucaGuarini I really don't feel like "inventing" another example. Those listed here are all showing the problem. The example you rewrote totally skips modifying the background color of the dom node. But that is what happens to be the problem (see the modified version of your code: http://jsfiddle.net/o9xzg0jy/1/)

In our code we insert new elements in the middle of the array which gets rendered with each as custom tags. What happens is that everything which is nor explicitly "overwritten" inside the update event, will stay well unmodified instead that the dom nodes get shifted/updated.

It is also pretty lame to have custom tags with their own data (opts or inside this) but need to access the entries from parent using an index all the time. That was not needed in Riot 2.0 afaik.

I don't say that it is not cool to have faster updates where it is working but I think there should be an opt in to slower updates but guaranteed sync with the dom nodes.

Can't we have both?

[oderwat]

@GianlucaGuarini about your example... the items needed to be removed are the ones which "object address" are not contained in the update anymore. We use push() and splice() on the items. Data is objects. Used with each and custom tags the data corresponds to the custom tag itself. Maybe we do it all wrong :) But that worked wonderful in 2.0 ...

[weepy]

How often in real code do you need to render something like ['foo', 'bar', 4, 'foo', 3, 4, 3, 'bar'] vs an array of objects?
Not very often methinks?

On Mon, 3 Aug 2015 22:18 Hans Raaf [email protected] wrote:

As @pakastin https://github.com/pakastin says... If you set a style or
form input value to any of those and remove element 1. They will suddenly
appear as part of entry two.

@GianlucaGuarini https://github.com/GianlucaGuarini I really don't feel
like "inventing" another example. Those listed here are all showing the
problem. The example you rewrote totally skips modifying the background
color of the dom node. But that is what happens to be the problem (see the
modified version of your code: http://jsfiddle.net/o9xzg0jy/1/)

In our code we insert new elements in the middle of the array which gets
rendered with each as custom tags. What happens is that everything which is
nor explicitly "overwritten" inside the update event, will stay well
unmodified instead that the dom nodes get shifted/updated.

It is also pretty lame to have custom tags with their own data (opts or
inside this) but need to access the entries from parent using an index all
the time. That was not needed in Riot 2.0 afaik.

I don't say that it is not cool to have faster updates where it is working
but I think there should be an opt in to slower updates but guaranteed sync
with the dom nodes.

Can't we have both?

—
Reply to this email directly or view it on GitHub
https://github.com/riot/riot/issues/990#issuecomment-127408576.

[GianlucaGuarini]

@GianlucaGuarini I really don't feel like "inventing" another example. Those listed here are all showing the problem. The example you rewrote totally skips modifying the background color of the dom node. But that is what happens to be the problem (see the modified version of your code: http://jsfiddle.net/o9xzg0jy/1/)

Riot has a fast and reliable template engine, why should you update the DOM without using it http://jsfiddle.net/o9xzg0jy/2/ ?

In our code we insert new elements in the middle of the array which gets rendered with each as custom tags. What happens is that everything which is nor explicitly "overwritten" inside the update event, will stay well unmodified instead that the dom nodes get shifted/updated.

Probably in your app you are mixing riot expressions, with custom jquery plugins, and other manual DOM updates, in this case you are right the current riot loops will fail I must admit that this can be considered a problem

It is also pretty lame to have custom tags with their own data (opts or inside this) but need to access the entries from parent using an index all the time. That was not needed in Riot 2.0 afaik.

This was just an example in real world application you need a flux architecture

I am still curious to see a real world example of your app, where you have problems updating your DOM. I would be really glad to help you if I could

[oderwat]

@GianlucaGuarini I could give you access to our internal git repository with the project. It would be much appreciated if you could give us some pointers - even if we did something pretty wrong. Actually it is not me who is coding it and I have limited knowledge about the problems in detail. I just try to supervise the work without using riot myself very much.

[GianlucaGuarini]

I don't know if I have time to check your entire project, but I guess probably you are doing something wrong. Just let some of your devs posting a jsfiddle where the current riot loops fail, so I will understand where is the issue. In general I strongly recommend a deep tour of our useful links in our readme file before starting any complex project
Please check also RiotControl to understand how could be simple to manage the riot loops

[pakastin]

Another real world example:

var element = {id: 1, title: 'Element 1'}
var element2 = {id: 2, title: 'Element 2'}
var element3 = {id: 3, title: 'Element 3'}

var item = {id: 1, elements: []}
var item2 = {id: 2, elements: [element, element2, element3]}
var item3 = {id: 3, elements: []}

The change:

[item, item2, item3]

->

[item2, item3]

What you'd expect:
• Item 1 gets removed

What really happens:
• Item 1 becomes item 2 (id: 1 -> id: 2 + element, element2 and element3 gets created)
• Item 2 becomes item 3 (id: 2 -> id: 3 + element, element2 and element3 gets removed)
• Item 3 gets removed

Think about a situation where you have lots of nested elements getting created and removed just because one item got removed (or reordered)..

[oderwat]

We know what is wrong @GianlucaGuarini ... you don't care about sync between the dom and the data anymore since 2.1 ... thats the problem. Choosing the best workaround is the problem here! It worked flawlessly in 2.0 there is no doubt about that.

You already have a fiddle where it fails ... if you say: Well we need to update every aspect of every dom node manually, yeah.. thats what I said before: The optimization kills the old code and one has to do it all manually. If I set an background color on mount I don't want to set it on every update again. I just want to set what changes. And again. This seemed to work in 2.0 and is now broken. As is re-ordering the elements. Which was a problem we had before and solved it by rewriting the nice and clean code into a mess of manually updates in the update event.

[oderwat]

@GianlucaGuarini BTW: First saying your would like to see the real use case (code) and then say you don't have the time is not helping much. Either you want to see the real code or you not :) And, yes we use RiotControl already :)... We also had to remove nearly all nested tags to get stuff back to a working state as everything seems to break with 2.2 and nested tags :(

[GianlucaGuarini]

@oderwat We will release riot 2.2.3 really soon. I will come back to this issue with riot 2.3 I swear :smile:
Hoping to find a solution that makes anyone happy

[oderwat]

We have a working project atm. But it is much less "cool" and elegant as it was before. I will review the code tomorrow with one of our devs in regards of using the template engine again. We had this at first but removed some of it because we needed to do stuff in the update / mount event anyway.

For example it did not work to have a custom tag with an input field "name" where the name comes from the opts. I thought I have an issue for this here too but can't find it atm. But this rendered custom tags kinda useless to be reused. For example we had a time entry field which interprets different strings into different internal minute based encoded values. But that custom tags input field needs a name. If you use <input name={opts.myname}> the name is not bound to this as this is not filled in on mount. So there we could not use the template for example and had to set the name in the javascript ourself.

Probably there was another bug / problem in between the versions. But anyway: I think syncing dom / data for nested tags with each should be a feature which at least can be "enabled" if needed.

[iamliqiang]

@cdanielw +1, I agree your point, upgrade to 2.2.x, the legacy code became strange,

 "If you use <input name={opts.myname}> the name is not bound to this as this is not filled in on mount. So there we could not use the template for example and had to set the name in the javascript ourself."

and in some situation , this.update() , not work anymore, so, i had to stay in 2.1.5, and test 2.2.3 in anothor server:(.....
syncing dom is meaning, or you can use shadow dom to update #$#$@#$@#$@#

[Mouvedia]

Too bad that destructuring assignment isn't well supported. It would be really useful in our case.

check the fourth example of http://robertnyman.com/javascript/javascript-1.7.html#destructuring-assignment

[pakastin]

Here's a reordering performance test with the latest version of my new vanilla js framework, frzr:
Create & reverse 10 000 items with reordering
Create & reverse 10 000 items without reordering

I'm getting almost equal results both ways.
It's even not that complicated..

This is where I create new view list:
https://github.com/frzrjs/frzr/blob/master/lib/viewlist.js#L72-L85 (without reordering)
https://github.com/frzrjs/frzr/blob/master/lib/viewlist.js#L88-L111 (with reordering)

Here's how I reorder nodes:
https://github.com/frzrjs/frzr/blob/master/lib/viewlist.js#L42-L62

Again, feel free to borrow some ideas ;)

[GianlucaGuarini]

@pakastin these are the results I get after ~10 clicks on firefox 42.0a2 macbookpro 10.10.5

Thanks for your input, would you have time to make a pull request?

[oderwat]

Well I have times around 1300 ms without reordering and also 1300 ms but then increasing ones with the reordering in Firefox (40.0.2 on Mac). So that gets slower and slower.

That said it is initially faster with Safari (130 ms) but the reordering version after some clicks jumps to be 1300+ ms and then sometimes needs 2-3 seconds for updating while reporting around 1400 ms still.

Interestingly both browsers show initial times <100 ms when the link is opened for the first time.

[pakastin]

Chrome and Safari gives better results.. I'd still need that trackBy/idAttribute -option though..

[GianlucaGuarini]

@pakastin Make your proposal, we could discuss about that in your pull request, I think it makes sense.
In any case with make perf you can check the speed of the riot loops compared to the previous release.

[pakastin]

I think jsfiddle has some issues with browser optimizations, it should work better properly hosted.. I've seen similar issues before with other frameworks as well in jsfiddle but not in production.

[txchen]

I am getting the same issue when I update my todomvc example, from riot 2.1.0 to 2.2.4. The issue is exactly same as @pakastin described.

[pakastin]

Tests should work better here:
http://frzr.js.org/example/with_reordering.html
http://frzr.js.org/example/without_reordering.html

I'm getting <100 ms both of them..

[pakastin]

Btw. if you keep developer tools open, it can slow things down quite a bit..

[tipiirai]

@GianlucaGuarini from my experience it seems that loops are the weakest point of Riot at the moment. Aside from that the library feels pretty stable. I would love to help with the issue, but the loop code is quite different now.

Do you have any ideas for fixing loops? What is your current priority?

Thanks.

[GianlucaGuarini]

@tipiirai I would like to start working on riot-cli as soon as @aMarCruz will release his first riot-compiler beta.
You can work on the loops ( now I am too busy.. ). I think until right now I was only able to fix the unmount method. Please note that also @rsbondi is working on the same file each.js so there could be some merge conflicts in future

[GianlucaGuarini]

Do you have any ideas for fixing loops? What is your current priority?

I think we just need to swap the children Tag instances properties when an item has changed its position in the collection, this should be enough to solve the issue.
Basically it should work in this way

item1 => TagInstance1
item2 => TagInstance2

↓ update

item2 => TagInstance1 takes the properties of TagInstance2 (except root)
item1 => TagInstance2 takes the properties of TagInstance1 (except root)

[tipiirai]

I'll see what I can do. Thanks!

[pakastin]

Here's a free tip – haven't used Riot anymore so wouldn't know where to start if I added it myself. This is a super fast and simple operation iterating childNodes and reordering them according to views (if you keep DOM references). view.$el is the DOM reference (has nothing to do with jQuery):

// start from firstChild and traverse:
var traverse = root.firstChild

// go through views:
views.forEach(function (view, i) {
  if (traverse === view.$el) {
    // already in place, continue to nextSibling
    traverse = traverse.nextSibling
    return
  }
  if (traverse) {
    // insert before current
    root.insertBefore(view.$el, traverse)
  } else {
    // append
    root.appendChild(view.$el)
  }
})

nextSibling is the secret here – using childNodes is slower..

[tipiirai]

Thank you @pakastin. The above code indeed looks clean! Hope I can incorporate such simplicity into Riot loops – feels hard on this very moment.

[tipiirai]

Unfortunately the code breaks if I remove items from the array with view.splice. Trying to fix that now

[tipiirai]

Looks like all the remaining children whose index is greater than view.length can be removed after the splice operation. This is what I have atm:

function update(root) {
  var el = root.firstChild

  tags.forEach(function(tag, i) {
    el === tag.root ? el = el.nextSibling :
    el ? root.insertBefore(tag.root, el) :
    root.appendChild(tag.root)
  });

  // remaining items can be removed
  ;[].slice.call(root.children, tags.length).forEach(function(el) {
    el.parentNode.removeChild(el)
  })

}

[pakastin]

I do it like this:

// start from firstChild and traverse:
var traverse = root.firstChild

// go through views:
views.forEach(function (view, i) {
  if (traverse === view.$el) {
    // already in place, continue to nextSibling
    traverse = traverse.nextSibling
    return
  }
  if (traverse) {
    // insert before current
    root.insertBefore(view.$el, traverse)
  } else {
    // append
    root.appendChild(view.$el)
  }
})
// Remove other DOM elements
var next
while (traverse) {
  next = traverse.nextSibling
  root.removeChild(traverse)
  traverse = next
}

[tipiirai]

My own version is easier for me :) Anyway --- beautiful stuff. Hope I can integrate.

[pakastin]

Glad to help! ;)

[rsbondi]

My work is trivial and should be easy to integrate after these changes, so I will hold off on any updates to each.js

[tipiirai]

Good to hear @rsbondi. Looks like I'm able to simplify each.js quite a bit. Thanks to @pakastin! Lot's of cases to take care of but I think I can make it. Hoping to finish tomorrow.

[pakastin]

Anytime @tipiirai! I feel responsible since I wrote the previous reordering ;D

[tipiirai]

I got really far with the new each.js. It uses the above neat traverse method and makes the implementation rather simple.

I was able to loop custom tags and push / unshift / slice elements from the data array, but then got stuck with looping regular HTML. The items are rendered on the page but the expressions aren't evaluated:

After hours of debugging It was a painful dead-end for me. @GianlucaGuarini – do you possibly have any insights?

[pakastin]

I love how you simplified my example even further, nice tricks here and there - you have such a unique way to write javascript :)

[tipiirai]

Thanks! Hopefully we can make the whole library very easy to understand. The above bug is pretty much impossible for me to figure out and see where the thing breaks. Unfortunately some internal parts of Riot are too complex at the moment.

[GianlucaGuarini]

Thanks @tipiirai I will check you branch tonight hold on..

[tipiirai]

Thank you so much!

[oderwat]

I just want to thank anybody working on that issue! We are stuck with a pretty slow application right now and I hope if this gets fixed we get up to the former speed!

[GianlucaGuarini]

@tipiirai I have fixed the issue. You were using the wrong DOM nodes for the loop children: the root gets replaced during the mount event

Btw I have checked your code and it looks nicer than the old one but I think it's also a lot slower ( due to the massive usage of DOM manipulations ) probably you can use document.createDocumentFragment() to improve the performances.
When I need to check the loops speed I always run the benchmark in test/performance/loop.html and now it seems to run again 4/5 times slower than the version in the dev branch
Please let me know if I could help you on anything else ;)

[oderwat]

If reordering only works with slower code I vote for getting two different routine: one with reodering and one without.

[pakastin]

There's no "better" or "faster" here: Performance depends on the situation. Lesser the items moving, faster it should be with reordering. Even all of them moving you'll get really close in optimum conditions. Depends on what else is happening and is there layout trashing going on etc..

Sometimes it's also compulsory to keep references.

Of course reordering should be optional: need depends on the project / task in hand.

[pakastin]

Sometimes you don't want reordering or content replacing at all. For example with Deck of Cards I just switch z-indexes to keep things fast: https://deck-of-cards.js.org

Also with flexbox there's a way to reorder by just changing the order value in CSS.

[tipiirai]

Thank you for the fix @GianlucaGuarini! I was a bit frustrated and I'm glad the fix was that simple.

So the performance issue is only present on reordering? And there is a way to avoid that by using documentFragment?

[tipiirai]

@pakastin, I saw your deck of cards the day it was announced. Impressive indeed!

[pakastin]

With documentFragment you'd need to find out sequent (peräkkäiset) items first and group them. Usually you don't even get any benefit.

[tipiirai]

hmmm.. doesn't sound like a thing to do then. And the performance penalty with the new version is only when reordering?

Anyway - I'm still seeing many issues with the new each.js. damn..

[pakastin]

Where can I test the new each.js out?

[tipiirai]

It's on the loops branch. Run make watch (watch for edits) and riot will be build to dist/riot/riot.js.

[pakastin]

make watch throws: Error: Cannot find module '../../dist/riot/riot'

[tipiirai]

Try npm install first

[pakastin]

I did that already..

Update: Made first change and the error was gone. Works now!

[tipiirai]

Damn. Really no idea then. Haven't touched that area recently

[pakastin]

There must be some layout trashing going on – shouldn't be that slow:
http://frzr.js.org/example/with_reordering.html
http://frzr.js.org/example/without_reordering.html

[tipiirai]

I'm just trying to get the tests passed first. For example object loop isn't there yet.

[pakastin]

If you open Chrome dev tools' "timeline" tab there's A LOT innerHTML calls going on (mkdom.js line 37), no wonder it's slow.

[pakastin]

Reproduce:

• start recording
• hit "reverse list"
• end recording

[tipiirai]

Okay, thanks.

I try to focus on performance after I get things working.

[tipiirai]

Unfortunately I cannot. Too many issues with the new loop. I'll revert back to dev version.

@GianlucaGuarini – my next call is to wait for you to try this out

[pakastin]

Reordering was not the issue here - triggering innerHTML change for every item is what makes it slow.

[tipiirai]

Thank you for the performance analysis, @pakastin

[GianlucaGuarini]

@tipiirai I also would start from the current code as well. I think It just needs to be cleaned up and with a few changes it should update/move the DOM nodes in the right order
Let me know if I must to be involved in this refactor because I would avoid that we work in two on the same thing.

p.s.
hint: keep always an eye on the runner.html page while you are changing the riot source code so you don't need to figure out why the tests are failing just at the end

[rsbondi]

by replacing here items.length with -1 this fixes the issue, it is takes about half the time as the loops branch but 1.5 times as long as the dev branch, so maybe just optimize that a little, or maybe it is acceptable

[rsbondi]

Actually it is closer to a 15% time increase over current dev which should be reasonable

http://plnkr.co/edit/LB1uaImENzAASGcVYMlW?p=preview

line 939 of the wip file you can change between -1 and items.length to see the difference

for some reason, either way, the first reversal is slow but after that it is fine

[rsbondi]

Minor tweaks, previous was no good for the color example

while (i--) { // check all, could be one removed and one added, this was my intention first pass
  if (!~items.indexOf(oldItems[i])) {
    var tag = tags[i]
    tags.splice(i, 1) // splice first, if you unmount tag first will be null
    tag.unmount()
  }
}

http://jsfiddle.net/xqte53n8/

there is still work to be done, riot should be able to do this as it did in previous versions.

Not sure how to get there, and keep performance.

[tipiirai]

Unfortunately the above didn't fix the issue on this fiddle. I simply unshift a new item to the top of the array and it doesn't render correctly.

[GianlucaGuarini]

@tipiirai @rsbondi I have cleaned up the each function in the dev branch, trying to bring the best I could get from the loops branch. I will see whether during the weekend I could work on the loops again fixing the unshift reordering issue and also #1142

[tipiirai]

Thank you so much.

[rsbondi]

My latest brainstorm. Start with the each loop from 2.0.15, which worked but was slow. Clone the root and swap into the dom. Then run the loop on the actual root which is now in memory. Then swap back. Only 2 dom updates.

[tipiirai]

Sounds like a sane approach. Are you giving a try?

[GianlucaGuarini]

I am still working on this but it's more complicated than I thought I need a bit more time ( all my tests failed somehow ) but I would also be happy if someone else could help me on this. BTW I have cleaned up the each.js file adding some comments hoping it will smoother working on it

[rsbondi]

I tried my brainstorm approach, it was simple enough to try just modifying the release, but it seems the loops were slow, not the dom updates so it was good for maybe a 5% increase tops. I think moving forward with the new code and adding an efficient way to determine when and where to move nodes is what is needed.

I would be glad to take over this task, since I am also working on virtual tags, which I have working with the dev branch before the loop branch was merged, but it would be good to move forward with virtual tags in mind, it would not lend well to tracking nodes instead of tags for example.

[GianlucaGuarini]

@rsbondi I would be glad to let you working on this issue because I've wasted too much time without getting any efficient solution. My last approach is available in the branch feature/reorder maybe you can take something good from it. Good luck

[rsbondi]

ok, I will look at the reorder branch and take over this task for now

[tipiirai]

Thank you @rsbondi. This is a tricky one!

[GianlucaGuarini]

I checked how React renders the loops and it seems they also update just the expressions without reordering the DOM nodes: please check how the class names stay the same http://jsfiddle.net/72wjyyus/
With the new riot loop reordering, our library will become again a lot slower in updating the DOM nodes position in runtime but at this point it seems to be the only solution we have to handle all our users wild needs :cry:

[pakastin]

It shouldn't be any slower – instead in many cases it should be faster (because of it's doing less work if only few items moved)! If it's slower, there's something wrong elsewhere. For example:
https://camo.githubusercontent.com/996f695a762a7a62c3210ce8929071fabab08ba5/687474703a2f2f70616b617374696e2e66692f69737375652e706e67

[andynemzek]

@GianlucaGuarini just fyi - I don't think this is a 'wild need'. We have a sizable app built with riot where I work and similar to @oderwat we found that riot's current loop implementation leads to very non-sensical results and ultimately made our app appear radically broken is certain scenarios.

Specifically, our app has lists that contain child-tags that load and store data onto themselves (async) after the loop has run. Basically, you can click an item in the list to see its detail. We found that when modifying these lists (ie: sorting, adding, deleting items) this 'detail data' would stick around on the tag (expected) but the tag would not be moved to match the changed positions of the elements of the list being iterated on. So the internal list of riot tags would become out-of-sync with the data that's being iterated on. For us, this meant that our list items were displaying details for the wrong items.

I had to spend over a day patching our app to work around this...so the problem is very real :(

[oderwat]

Please don't forget that you are also doing microbenchmarks with certain tests. It may be slower to reverse 100.000.000 items or even update simple values. But we only have about 30 Items and it is much slower to re-render all their (sub) content because the already rendered results don't get moved to the right place. We now have to internally cache all this data in a "pre-rendered" state and also re-render it unconditional on every update (which we then try to optimize ourselfs so the app still works 'at all'). With propper reordering we could just use the raw data and the dom. Where items just get redrawn if they really update their data.

[GianlucaGuarini]

@oderwat I understand it, We are working also for you :) But it does not mean that we should ignore the riot performances. @rsbondi I think I could fix the single test that is failing, give me a bit more time please.

[GianlucaGuarini]

This issue got solved thanks to @rsbondi and the patch is now available in the feature/reorder branch.
@tipiirai please test it and let me know if we can merge it into the dev branch. With this fix riot becomes 4x slower than the previous release!

[oderwat]

Well I personally lost $$$ and time for that useless (from our point of view) iteration of functionality. But that way my fault because I said that we go with the newest release even if it breaks our code. Not knowing how bad that becomes. Still I thank you for your time and effort. We will also check out how the new code affects our project when it lands in dev.

[GianlucaGuarini]

I think that before merging it in the dev branch I will implement also the possibility to render faster loops using the DOM attribute reorder="false" on the loop node

[tipiirai]

I think reorder: false could be the default, unless unshift or splice methods require it.

[GianlucaGuarini]

@tipiirai ok I will add some tests as well

[pakastin]

I profiled JS performance during reorder (10 000 reverse) – most of the time goes to waste because try/catch is being used (V8 cannot optimize try/catch -code):

Every try/catch should be removed!

[pakastin]

If you see any exclamation mark warning sign in profiler (Profiles - Collect Javascript CPU Profile), there's unoptimizable code.

[pakastin]

See: https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#2-unsupported-syntax

[GianlucaGuarini]

@pakastin now I can't test it, but what if you try to remove all the try and catch? Does it run faster? This does not explain why it is 4/5 times faster ( also faster than React ) without reordering the DOM nodes

[pakastin]

If I remove try/catch, it hits an error (Uncaught TypeError: Cannot set property currentTarget of # which has only a getter). Seems it catches errors all the time. Should be fixed first.

[pakastin]

Type checking, checking for null/undefined should be used instead of try/catch. I'm sure that would speed things up a lot. Check out: https://www.youtube.com/watch?v=1kAkGWJZ6Zo for insight about browser optimizations.

[tipiirai]

@pakastin you can ditch the try block by replacing this block

try {
  e.currentTarget = dom
  if (!e.target) e.target = e.srcElement
  if (!e.which) e.which = e.charCode || e.keyCode
} catch (ignored) { /**/ }

with this:

if (!e.currentTarget) e.currentTarget = dom
if (!e.target) e.target = e.srcElement
if (!e.which) e.which = e.charCode || e.keyCode

Would love to see the results with the change.

[GianlucaGuarini]

@tipiirai I think we need to use getOwnPropertyDescriptor on the event object https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Object/getOwnPropertyDescriptor to understand if the properties are writable

[tipiirai]

Actually we don't need that code at all! Those properties are there for IE8 only. Just checked for browser support. Just need to check whether the currentTarget points to the indented element.

[GianlucaGuarini]

So let's delete them ;)

[pakastin]

I made an example with a draft version of the upcoming frzr.js.

Try add multiple bunch of items without reordering first and compare that to adding them with reorder on:
https://frzr.js.org/example/index.html

Good example of that reordering isn't slow, and that it's even faster than replacing contents in some cases.

[weepy]

@pakastin I get that reordering is always faster than not reordering,

[pakastin]

Those test results should be pretty accurate, since I'm using batched DOM updates inside one requestAnimationFrame and that render time tells only the rendering and nothing else.

[pakastin]

What's happening there:

• with reordering: add 1000 items to head
• without: add 1000 items to tail and update every items' content

That's why without reordering render time also grows and grows when more and more items are added.

[pakastin]

I added the "legendary" reverse test, even that seems to run faster now!
https://frzr.js.org/example/index.html

I was doing it wrong in the previous frzr..

[oderwat]

@pakastin reverse without reordering does just update the fields "at once" for me. While doing it with reodering the view gets empty in between (just showing the top element and then the remaining appear a bit later. Why does this happen? This could make a huge difference in an actual application.

[GianlucaGuarini]

@pakastin I really appreciate you interest in this topic but your posts regarding "your way" of looping in frzr.js does not help me a lot improving the riot code. I would like you could help us contributing on the riot source code if you have enough time otherwise we need to handle the improvements by ourselves.
I appreciate a lot more the help coming from @tipiirai and @rsbondi that helped me refactoring the code making it cleaner and more readable.
If you had spent the half of the time you have used for writing your examples trying to understand why reordering nodes in riot is slow we could have been already working on other features pushing the project forward.
Nothing personal and thanks anyway

p.s.
this is just my personal opinion that does not represent the opinion of the other riot contributors

[pakastin]

I'm just proving what the performance would be in the optimal conditions, because you keep repeating that reordering is slower which simply isn't true.

I have also profiled Riot and given the results, there are lots of warnings in the profiler. I have also given an example how to reorder really simply. I just don't understand the details in Riot source anymore and don't know what to keep and what to delete.

I'm really just trying to help here. The reason I do these tests is that I'm researching the most performant way of doing DOM updates and how the V8 optimizer works. It's easier to play with smaller codebase when you want to try out different ideas.

[pakastin]

@oderwat: which browser? This is really preliminary test and optimized just for Chrome/webkit.

[oderwat]

@pakastin that was safari os x 10.11 but is the same with firefox on osx too.

[pakastin]

Ok, thanks for letting me know!

update: now I got that also – seems there's something wrong in my batched animationFrame update

[GianlucaGuarini]

BTW this is the "performance improvement" I get removing all the try {} catch(e) {} from the riot code

[pakastin]

@oderwat: It's fixed now: https://frzr.js.org/example/index.html

@GianlucaGuarini: Show me what profiler says (Dev tools - Profiles - Collect JavaScript CPU Profile - hit Start, press Reverse, hit Stop). Is there any warning signs?

[GianlucaGuarini]

Here you got it

[pakastin]

Thanks! Seems there are still lots of optimization errors. Where I can try that?

[GianlucaGuarini]

here

[pakastin]

Ok thanks, I'll check that out.

[GianlucaGuarini]

this issue now is fixed and the patch is available in the dev branch. To enable the dom reordering you should set reorder=true on dom node you want to loop. The issues with unshift and the wrong children removal were solved also without enabling the reorder attribute. I have added some tests to avoid this issue in future. The riot loops became a bit slower but more reliable.

[tipiirai]

Holy crap.

I haven't tried this yet, but looks like you did a fantastic job. This was certainly a pain in the ass. Thank you for the hard push!

[GianlucaGuarini]

@tipiirai yeah it was but without @rsbondi I could not fix this issue. Thank you guys for your help

[pakastin]

@GianlucaGuarini: still saying DOM reorder is slow?
https://twitter.com/pakastin/status/651581181910208512

Thanks for challenging me! ;)

[tipiirai]

Awesome performance there on frzr. Congrats!

We obviously hope to get to the same speed level, but it's much harder when you need to support unlimited nesting of custom tags, which can also have nested loops and conditionals.

This is an ongoing process. Maybe profiling tools can help.

[pakastin]

I know, and still I'm not comparing - just trying to push the browser limits and show what could be done. And FRZR wouldn't exist without Riot: I didn't understand anything about this stuff before I fell in love with the simplicity of Riot.

Profiling helped a lot. I will try profile Riot also when I figure out how to use this new reorder parameter.

[GianlucaGuarini]

I close this bug it got fixed we can move forward

[pakastin]

@tipiirai: FRZR does those also http://pakastin.github.io/frzr-todo/ ;)

[GianlucaGuarini]

so definitely frzr.js is better than riot maybe I should start using it! :joy_cat:

[pakastin]

I sense some sarcasm there.. :) Well, FRZR is mainly for my personal use, but I just wanted to share the research. Of course Riot is much easier to use i.e. because of templating, that's something I intentionally left out. FRZR is more of a javascript-first view engine.

[GianlucaGuarini]

but I just want to share the research

I use to call it spam

[pakastin]

Hey, you're now using the new efficient reorder method in Riot as well, which came out as a side product from my FRZR tests. Show some respect. Also the only reason I shared the tests here was you not believing me that reordering is not slow (which I'm grateful to you, because it pushed me towards researching even better performance).

Sometimes it's easier to iterate (& benchmark) ideas with a fresh project.

FRZR is not competing with Riot, you should already know that.

[pakastin]

This logic:
https://github.com/riot/riot/blob/dev/lib/browser/tag/each.js#L73-L108

Was here first:
https://github.com/pakastin/frzr/blob/master/lib/views.js#L62-L76

..and I shared it here:
https://github.com/riot/riot/issues/990#issuecomment-142216140

[oderwat]

@GianlucaGuarini Why don't you realize that: https://github.com/riot/riot/issues/484#issuecomment-113805214 was simply a wrong decision. That broke projects and in the end cost people a lot of time and money. @pakastin already took part of the conversation at that time and even if you would not use any of his ideas hey saw the weight of the problem you didn't. You even wrote "Riot 2.2.0 is finally out! I will close this issue hoping our users will not complain the old each method :smile:" :(

[oderwat]

To make it clear: I think that Riot with the new optional optimized "reordering" is much cooler that "being as fast as anyone else without reordering". And that was (imho) the spirit @pakastin wanted to bring to the project. Thanks @all btw...

[pakastin]

I even tried to say back then many times that dropping reordering will cause problems, i.e.:
https://github.com/riot/riot/issues/484#issuecomment-109757765

Btw. this fix doesn't seem to reorder DOM elements anymore, am I right?
That's a breaking change and might cause problems for some. At least for me reordering is a required feature.

and:
https://github.com/riot/riot/issues/484#issuecomment-109890032

Think that you have a tree structure, where you render every depth concurrently. If you move a node having a deep structure, then it's easier to render only the DOM reordering, and not removing components in all depths and recreating them in the new place.. (If replacing branch doesn't have equal deep structure). There are lots of other use cases where reordering is needed.

Remember that the loops in Riot are not only data loops, but also there could be components involved.

[oderwat]

Let's not beat the dead horse :)

I take the "I close this bug it got fixed we can move forward" as the essence. After all I am happy that we can use such a mature project basically for free!

[pakastin]

I agree :)

[rsbondi]

I tend to think that the default should be the reorder mode, that works in all cases, but is slower, and if you don't need reordering but need speed, then you should set an attribute no-reorder or something. Also, I would be happy not having any attribute and living with the slower speed, which is not an issue unless there is an unreasonable amount of tags mounted(I am a pagination fan but that does not seem to be the trend these days), that would keep riot more simple, one less parameter to learn. Only my opinion, would like to hear others.

[oderwat]

I go with @rsbondi but would like an opt-in to an optional faster version (noreoder). It could also be each and fasteach or noreach or raweach instead of an option. Which would make it possible to track usage more easily.

[GianlucaGuarini]

@rsbondi @oderwat could you please open a new issue/discussion about the reorder attribute?

[Mouvedia]

:+1: @rsbondi

[iamliqiang]

congratulation, 990 was fixed ! thanks for riot.js team