Skip to content
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

Potential performance improvement #223

Closed
devanp92 opened this issue Jun 4, 2016 · 20 comments
Closed

Potential performance improvement #223

devanp92 opened this issue Jun 4, 2016 · 20 comments

Comments

@devanp92
Copy link

devanp92 commented Jun 4, 2016

In rendering.js each for-loop can be optimized by first caching the length of the array. Unfortunately, jsPerf is down, but this article might provide insight.

for (var i = 0, length = arr.length; i < length; i++) {...}
@leeoniya
Copy link

leeoniya commented Jun 4, 2016

pretty much all modern Javascript JITs will detect the length invariance and do this optimization for you. it may have mattered when the article was written (2013), but not today.

@devanp92 devanp92 closed this as completed Jun 4, 2016
@devanp92
Copy link
Author

devanp92 commented Jun 4, 2016

Ah, gotcha. Thanks - I didn't know about that.

@localvoid
Copy link

http://mrale.ph/blog/2014/12/24/array-length-caching.html

@developit
Copy link

@devanp92 JSPerf won't be back any time soon sadly :( I threw esbench.com together a while back as a replacement, using the same Benchmark.js lib. Here's a benchmark showing @leeoniya & @localvoid are very much correct!

@leeoniya
Copy link

leeoniya commented Jun 6, 2016

@developit thanks for esbench, i'll be playing with it. you and @localvoid should collab to integrate https://github.com/localvoid/perf-monitor and/or https://github.com/localvoid/uibench into esbench.com ;)

@developit
Copy link

Definitely. I use uibench probably 3 hours per day (is that embarrassing? I blame @trueadm lol). It would be super cool to have a benchmarking tool focussed on UI stuff that supported importing npm modules and/or files from disk.

@trueadm
Copy link
Member

trueadm commented Jun 6, 2016

@leeoniya @developit I wish everyone would stop using that "js-repaint" tool as a metric for actual performance. It's completely inaccurate, has no actually bearing on performance and has been wholly misleading the entire community as to how dbmonster "really" performs. I was going to write a Medium post on it actually, but didn't have the time.

@trueadm
Copy link
Member

trueadm commented Jun 6, 2016

@developit I'm glad my compulsive performance addiction rubbed off on you :D

@leeoniya
Copy link

leeoniya commented Jun 6, 2016

@trueadm

It's completely inaccurate, has no actually bearing on performance and has been wholly misleading the entire community as to how dbmonster "really" performs.

It's difficult to agree with such a grand statement when the bench does in fact correlate well with the perceived performance attained and overhead incurred. At least in relative terms it is a very useful comparison/sanity-check tool even if the "fps" and other metrics are not accurate in the absolute. I would be very interested to read your writeup if you get around to it, btw.

@trueadm
Copy link
Member

trueadm commented Jun 6, 2016

@leeoniya It's inaccurate as you can easily gain "higher" repaints artificially. Prime example is the Angular 2 and the Web Worker example, where it shows repaints at 130/s on my machine, yet you can't even scroll the page due to the lag it causes (I'm sure you saw/have the link, I'm on my phone atm so don't have it handy). The thing about pure FPS is, well it's actually the UI updating accordingly to how fast the DOM is rendering (including repaints/reflows).

@leeoniya
Copy link

leeoniya commented Jun 6, 2016

@trueadm it's one thing to say the bench is game-able but something else to state that it is therefore entirely worthless. You're right that the current crop of implementations need to be vetted for using such sneaky tactics. Without profiling tools built into the browser itself (like repaint/reflow/mem-alloc/gc events) i think it would be difficult (impossible?) to make live perf tools that cannot be gamed. That being said, we have to have something in the mean time other than IRHydra, --trace-deopt and other external tools that can at least give relative regression tracking.

@trueadm
Copy link
Member

trueadm commented Jun 6, 2016

@leeoniya In my profession and experience, and this is something I have a lot of experience with, if you don't make a stand and say something is useless in terms of a benchmark (even if it had some value at one stage) people will simply neglect changing it and continue to use said tools.

Personally: I think the uibench is far better indication of performance in terms of real world use. We could make a better "dbmonster" bench that: removed/added dynamic height rows/columns upon update and applied animations via JS to cells – now that would be a far better indicator. :)

@trueadm
Copy link
Member

trueadm commented Jun 6, 2016

On a side note: I'm trying to work out why the new Mithril re-write:

https://cdn.rawgit.com/lhorie/mithril.js/rewrite/examples/dbmonster/mithril/index.html

Performs 4-5 FPS better than Kivi and Inferno. Not really had a chance to dwelve too deeply in it, but it's interested to know if @localvoid and I are missing some better way at applying DOM mutations.

@developit I'd really appreciate it if you moved Preact to using this update FPS counter so I could see how that performs too – maybe we can all understand and work with one another on implementation details to make our libraries more efficient. :)

@leeoniya
Copy link

leeoniya commented Jun 6, 2016

@trueadm for the record, the only person who mentioned "js-repaint" and "dbmonster" was yourself; i only brought up perf-monitor and uibench :D

@trueadm
Copy link
Member

trueadm commented Jun 6, 2016

@leeoniya you knew what I was referring to :)

@developit
Copy link

developit commented Jun 6, 2016

@trueadm which FPS counter? I actually ended up grabbing monitor.js and the FPS display from your hosted version since they seemed less bad. I've been hacking on things privately but will publish soon as part of the official 5.0 cutover. I'm still annoyed that for a performance benchmark, the monitor is creating an infinite stream of TextNodes (setting .textContent on an Element rather than on the sole TextNode child).

What's your go-to benchmark right now? Just normal UIBench? I've mostly been using it to monitor for performance regressions - @localvoid mentions that as a use-case in the description and it's saved me at least twice now (accidental double-reconciliation from misaligned diff, shows up as an obvious 2x perf drop).

--edit--

Also, I've been thinking it would be awesome to have an "app benchmark" that was a bunch of real-world use-cases collected together. Not for comparing between various UI libraries, but for understanding the behaviors of these libraries in different real-world scenarios. Honestly we have quite a few equivalents to the microbenchmark for vdom libs, and they tend to serve the library authors more than the potential users of these libraries. It could even be something done via BigRig or browser-perf so we could have a clean way to measure the behavior of the application as a whole rather than analyzing things like the JS heap size. Thoughts?

@leeoniya
Copy link

leeoniya commented Jun 6, 2016

Something I use for DOM-op regressions (and is part of all tests) is a DOM-instrumentor I put together [1]. It ensures that only the expected quantity of specific DOM ops are performed during creating/recycling/removing/reordering. I've also considered using strictdom[1], but since none of our libs read the DOM, it may not be too useful. For lib overhead regressions not related to DOM i still use dbmonster/perf-monitor, but will probably move to UIBench at some point.

BTW, have you guys found createDocumentFragment to be helpful enough to warrant its use? It didnt seem to make much difference for me, neither for detached nor live DOM.

[1] https://github.com/leeoniya/domvm/blob/1.x-dev/test/lib/dominstr.js
[2] https://github.com/wilsonpage/strictdom

@developit
Copy link

developit commented Jun 6, 2016

@leeoniya yeah. uibench is super helpful because you can see which types of operations regressed (eg table/sort slows down a bunch? keys issue, recycler priority, offset bug - etc).

For fragments, that's actually an optimization I've had in preact (here) for a while but never directly tested to check if it was worth the bit of bloat. Here's a quick & dirty microbench (note: will hang for ~1 min) that shows appending 4 nodes is actually far faster when done without the fragment in a detached DOM, and just pointless for a live one:

So I'm thinking you're quite right, this might be another case of performance advice that made sense 5 years ago but was made irrelevant when we got async dom.

@localvoid
Copy link

@trueadm

Performs 4-5 FPS better than Kivi and Inferno. Not really had a chance to dwelve too deeply in it, but it's interested to know if @localvoid and I are missing some better way at applying DOM mutations.

Opened dev tools, found out that it recalculates styles way much faster, then opened css file: https://cdn.rawgit.com/lhorie/mithril.js/rewrite/examples/dbmonster/styles.css
The main difference is that we use //netdna.bootstrapcdn.com/bootstrap/3.1.1/css/bootstrap.min.css

@localvoid
Copy link

localvoid commented Jun 7, 2016

@leeoniya

BTW, have you guys found createDocumentFragment to be helpful enough to warrant its use? It didnt seem to make much difference for me, neither for detached nor live DOM.

Inserting children from fragments should be slightly slower:

https://chromium.googlesource.com/chromium/blink/+/master/Source/core/dom/ContainerNode.cpp#66
https://chromium.googlesource.com/chromium/blink/+/master/Source/core/dom/ContainerNode.h#367

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

No branches or pull requests

5 participants