Opened 7 years ago
Closed 5 years ago
#17530 closed enhancement (patchwelcome)
FOR loop performance
Reported by: | Samuel Ondrek | Owned by: | |
---|---|---|---|
Priority: | undecided | Milestone: | 1.13 |
Component: | Core | Version: | 1.9.1 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
We should improve a performance of core and files like dojo.js (also other files like dom-class.js, dom-construct.js, dom-form.js, deferred.js or html-js.. and many many others) because they still use slower way of writing loops with checking of length in every iteration. And this is not only performance issue, also semantic and readability side of solition is quite creepy, when we check length in every loop, but inside of loop don't change it.
for(var i=0; i<arr.length; i++) { return; } for(var i=0, len=arr.length; i<len; i++) { return; }
How slow? .. normally are these impacts not so huge (http://jsperf.com/for-loop-length-declaration in my case 3~5% faster), but if you use Dojo for massive DOM manipulation, it can be really interesting (minimal 10 loops in all dojo files * 5% slower * numberOfElements...).
Minifier business? Also there is situation, that you cannot handle this in Minifier, because sometimes (like in case when you really need every iteration check lenght) you really want to use slower version and it's pretty impossible to detect it..
Google Closure? It's funny, that Dmitry Baranovskiy, author of gRaphael and Raphael, have very critical article about same issue in Google Closure with interview "How to do not javascript" cca 5 years ago!!! (.. and after this article guys from google refactor some code parts).
Change History (5)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
Unfortunately I cannot reply specific situations what we are working on (company rules). -- Yep, I can send a pull request, I have signed CLA 2 years ago (I just checked it, looks Ok). -- And no, it doesn't mean that I need patch code, I'm just humble asking with about an idea. It just "nice to have".
comment:3 Changed 7 years ago by
While I agree with you in general -- in most cases caching an array's length makes perfect sense -- I am a little bit skeptical about the premise: "massive DOM manipulations". I benchmarked the problem several years ago and at that time DOM manipulations were quite heavy -- easily overweighting loop expenses. The problem is that DOM objects are not native JS objects and come with a lot of hidden expenses, so manipulations on numbers with arrays or other intrinsics do not emulate their performance sufficiently. Another thing is that DOM manipulations rarely contain a lot of looping, and frequently it can be avoided by other techniques. Admittedly this benchmarking is long in the tooth now, and we can benefit from new one.
comment:4 Changed 5 years ago by
Component: | General → Core |
---|
comment:5 Changed 5 years ago by
Milestone: | tbd → 1.12 |
---|---|
Resolution: | → patchwelcome |
Status: | new → closed |
Given that no one has shown interest in creating a patch in the past 2+ years, I'm closing this as patchwelcome. Please let us know if you would like to get involved in helping make this change to Dojo!
You mentioned "massive DOM manipulation"... are you talking about iterating live collections like returned by getElementsByTagName()? If so, we benchmarked that a few years ago and ended up with
while(elem = list[idx++])
as the optimal pattern.Also, since you are claiming a 5% improvement that means that you have a patch and a benchmark? If so, can you submit them as a pull request after signing a CLA?