Closed
Bug 598771
Opened 14 years ago
Closed 12 years ago
forEach slower in Firefox than other browsers
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
DUPLICATE
of bug 714010
People
(Reporter: erikvvold, Unassigned)
References
(Depends on 1 open bug)
Details
(Keywords: perf)
Attachments
(1 obsolete file)
I did a search for forEach in mozilla central and found hundreds of
matches, and many did not seem to be in test code, not sure if or how many of
these are custom implementations & uses, on an object for instance.
Also did some testing http://jsperf.com/array-for-loops and forEach also
appears to be about 1/4 to 1/5 as fast as a normal for loop.
I suggest removing it where it can be replaced with a for loop.
Comment 1•14 years ago
|
||
Sounds this should be fixed in the JS engine, i.e. forEach shouldn't be slow.
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
(In reply to comment #1)
> Sounds this should be fixed in the JS engine, i.e. forEach shouldn't be slow.
I doubt that is possible, a forEach implies setting up a new scope and arguments array per iteration (which prob means it causes more work for the gc), but a for loop only needs a new scope setup once, and needs no arguments array.
(In reply to comment #2)
> a for loop only needs a new scope setup once
I take that back, it's 0 times
Comment 4•14 years ago
|
||
If the code involved is not hot, does it matter? forEach can be much more readable....
That said, there are existing JS engine bugs on this sort of thing, iirc.
Whiteboard: DUPEME
Comment 5•14 years ago
|
||
This bug is invalid as stated. A forEach functional-style iteration can be the right tool for the job in terms of code simplicity (maintainability) and time-to-completion when coding, and if it's not on any critical path, it's foolish to micro-optimize to a for(;;) loop.
What problem is being solved here? As comment 2 says, it's hard to optimize a function call per iteration to match the evaluation of a for-loop body, but we always try to go faster.
From http://jsperf.com/array-for-loops (nice work, Erik) I see we are doing well against the competition (although no IE8 or IE9 beta results). If we were far off then we could focus this bug on catching up, but we seem to be ahead or roughly even. Unless I'm missing something, please resolve this bug invalid.
/be
Whiteboard: DUPEME
Updated•14 years ago
|
Whiteboard: DUPEME
Comment 6•14 years ago
|
||
Or a dup. I think invalid, though, since despite the nice jsperf.com page, this bug is unfocused and the premise of comment 1, that one should always take time to over-optimize JS at the expense of simplicity and functional-programming style or taste, is false.
/be
Comment 7•14 years ago
|
||
Never mind comment 1, the summary is false. Clearly, people use forEach and it is not obviously too slow for use, or they wouldn't (i.e., we would already have had bugs filed on user-facing perf probs blamed on such forEach usage).
/be
Comment 8•14 years ago
|
||
I think the perf delta against jsc and recent v8 is a useful thing to have a bug on (but again, I think things like the invoke gatling gun already cover parts of it..)
(In reply to comment #7)
> Never mind comment 1, the summary is false. Clearly, people use forEach and it
> is not obviously too slow for use, or they wouldn't
I don't buy this claim, developers write slow code that users suffer with all of the time.
Firefox has been a very slow program for a long time, and as someone who loves it I simply want to see clearly inefficient code removed amap. So I guess I disagree that a developer should choose taste over performance, when the diff in difficultly is trivial. I don't think that someone should be tasked with making hundreds of these little changes, but I would like to see this bug depend on those hundreds of little bugs. So that the volunteer community can try to deal with it. Also so that it may serve as a warning sign against using it in the future.
Because it appears to be used habitually, I don't mean to pick on anyone, but take view-source:resource://gre/modules/AddonManager.jsm as a example (there are many others) I don't think it's necessary to use forEach anywhere in there, and it effects moments as important as shutdown time.
I know it's hardly noticeable diff, but multiply it by 400 million how many times per day?
Besides the fact that making these changes would make minor improves Firefox's performance (without hurting readability or maintainability in many cases, which is something that is in the eye of the beholder anyway) it would serve as an example to others as to what the better techniques are, instead of demonstrating how nifty the slower techniques are.
Comment 10•14 years ago
|
||
Here's my hack to force the GRE dir to be the correct one for component registration.
Comment 11•14 years ago
|
||
Comment on attachment 478124 [details] [diff] [review]
hax0r
Argh, wrong bug.
Attachment #478124 -
Attachment is obsolete: true
Comment 12•14 years ago
|
||
(In reply to comment #9)
> (In reply to comment #7)
> > Never mind comment 1, the summary is false. Clearly, people use forEach and it
> > is not obviously too slow for use, or they wouldn't
>
> I don't buy this claim, developers write slow code
That is the claim for which there is no evidence.
Slow code needs profiling to prove the slowness is due to forEach. You have not produced any such evidence against real sites or apps. Synthetic benchmarks can cause all sorts of wild goose chases.
Sorry, but the burden of proof is on you, not me. We're trying to do science here, not cherry-pick and assert based on hunches or synthetic and biased tests.
But the thing that really makes this borderline invalid, beyond its misstated summary, is that we are not that slow! Not compared to the competition and not compared to a raw for loop.
Let's focus on important bugs first.
/be
Comment 13•13 years ago
|
||
Are we sure this is a duplicate?
Summary: forEach is too slow for use → forEach slower in Firefox than other browsers
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•