Closed Bug 598771 Opened 14 years ago Closed 12 years ago

forEach slower in Firefox than other browsers

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
trivial

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.
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
Keywords: perf
(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
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
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
Whiteboard: DUPEME
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
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
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.
Attached patch hax0r (obsolete) — Splinter Review
Here's my hack to force the GRE dir to be the correct one for component registration.
Comment on attachment 478124 [details] [diff] [review]
hax0r

Argh, wrong bug.
Attachment #478124 - Attachment is obsolete: true
(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
Depends on: 462300
Depends on: 602132
Are we sure this is a duplicate?
Summary: forEach is too slow for use → forEach slower in Firefox than other browsers
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Depends on: 784288
Whiteboard: DUPEME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: