1383362 - Use for loop rather than forEach in Services.jsm

Status: RESOLVED FIXED

(Toolkit :: General, enhancement)

Description

[Kris Maglione [:kmag]]

This winds up being about twice as fast.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1383362: Use for-of loop rather than forEach() in Services.jsm.

Review commit: https://reviewboard.mozilla.org/r/160042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160042/

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8889003 [details]
Bug 1383362: Use for-of loop rather than forEach() in Services.jsm.

https://reviewboard.mozilla.org/r/160042/#review165454

I saw this .forEach in profiles a few times too. Also true of the .forEach calls used for similar purposes in the root scopes nsBrowserGlue.js and browser.js

(In reply to Kris Maglione [:kmag] from comment #0)
> This winds up being about twice as fast.

Out of curiosity, how did you measure this?

The last time I tried to compare forEach, for..of and for (let i; i < array.length; ++i), benchmarking correctly ended up being non trivial, and I had to get help from JS engine folks to get correct answers... which were unfortunately much more complicated than I initially hoped for. See bug 1355874 comment 8.

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8889003 [details]
Bug 1383362: Use for-of loop rather than forEach() in Services.jsm.

https://reviewboard.mozilla.org/r/160042/#review165454

I just measured the time it takes to load the Services.jsm root scope according to the profiler.

It's hard to really say whether for-of is faster than forEach in the general case, but it definitely is faster here, and has also been faster in other slow sections of code that I've changed in the past.

Comment 4

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6469a8fbaae429b58fc07d4a447e4c2e294328f7
Bug 1383362: Use for-of loop rather than forEach() in Services.jsm. r=florian

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6469a8fbaae4