search

crysalead-js / dom-layer

Virtual DOM implementation.
MIT License
30 stars 1 forks source link

issues with new patching method #38

Closed ghost closed 9 years ago

ghost commented 9 years ago

Hi. I'm back to normal, and the electricity is back too. So time for finding your bugs again :)

Your new patch has some problems it seems for me at least. Test cases and everything working just fine, but have you tried your input example?

Try it, and see if you encounter the same issues as me. Here is my findings:

ghost commented 9 years ago
ghost commented 9 years ago

More findings.... The issues I found, are in all major browsers, but in Firefox I experienced another strange thing. Sometimes - not all the time - when I click back and forth on the radio buttons I got an insane delay. Up to 23 seconds I had to wait one time before the radio button got active again.

Memory leak in Firefox?

My CPU went up to 54% usage in Firefox and FF are now using 1,3 GB RAM when I tried to run your code.

Average in FF for me are 2 - 4% usage and 340 MB RAM.

And now Firefox freeze :(

jails commented 9 years ago

I think the issue has been fixed in https://github.com/crysalead-js/dom-layer/commit/3af9921658fd8c706759b9d4bfe030bf3dc096e3

ghost commented 9 years ago

Issue seems to be fixed, but I still experience a little slow-down in FF. I don't have any Android phone by hand, but one of my colleagues tested on Android with stock browser, and told me about some lags. Android 4.3.

jails commented 9 years ago

Weird, just tested on my Android 4.1.2 and locally with Firefox & Chrome (linux) for both the speedtest and the input example and I didn't noticed any lag or slow-down issue.

ghost commented 9 years ago

In many cases,code can act different from device to device. However, me too did a re-search on this to try to help you out. And I didn't find a good solution, except it looks like you don't need the slice() in the patch function. At least the code run very well without it and didn't have any FF issues now.

After more re-search, I found this gist: https://gist.github.com/brettz9/6093105

Maybe this could be used to speed things up etc?

jails commented 9 years ago

I updated the patching strategy to use the snabbdom implementation https://github.com/paldepind/snabbdom/blob/master/snabbdom.js#L117-171 which reach similar performances as the citojs one. So it shouldn't create further slow-down than the former implementation. And I used splice() because the patching algorithm is destructive at some point https://github.com/crysalead-js/dom-layer/blob/master/tree/patch.js#L63 and even if it should work w/o it, I think it's better to not mutate a passed parameter (even if the chilren array shouldn't be used any more). However you shouln't notice any performance impact. On my Android 4.1.2 I reach on the speedtest beachmark: ≈ 250 ms for the dom-layer ≈ 200 ms for cito ≈ 2000 ms for React

So contrary to React we are in good shape imo ;-).

ghost commented 9 years ago

I get it. And I guess in the future you will know if any others encounter problems through issue tickets. But great performance! Challenge now would be to beat citojs? Using range(), insertAsjacentHTML etc ?

jails commented 9 years ago

Not really, just wanted to make a flexible customizable virtual dom implementation to start a front end library on top of it (similar to the deku one). And I think the performances with the new strategy are decent enough now http://vdom-benchmark.github.io/vdom-benchmark/ for all the features this library provide.