Don't use forEach in loops #631

Merged
merged 16 commits into from Apr 20, 2013

Conversation

Projects
None yet
4 participants
Contributor

twpayne commented Apr 19, 2013

I know that @tschaub doesn't like forEach :)

This PR uses for loops instead of forEach in performance-critical code.

Open questions:

  • Should the length property be cached or not? i.e. should we use for (i = 0, n = arr.length; i < n; ++i) or for (i = 0; i < arr.length; ++i) ? jsperf says it only has marginal effect, and typically arr.length < 10.
  • Should we check object properties with hasOwnProperty, even when we know that the object has no prototype? Crockford thinks we should, but I don't know if this test decreases performance (by adding a check) or increases performance (by hinting to the JS engine that we'll never traverse the prototype chain).

Thanks for any input.

Owner

tschaub commented Apr 19, 2013

Nice @twpayne.

My votes:

  • cache length
  • don't use hasOwnProperty when iterating through properties of objects we create or where we expect object literals from users; document that we work with all enumerable properties on user supplied objects; document that we might not find user provided toString on old IE and any other caveats
Owner

elemoine commented Apr 19, 2013

  • +0 on caching length (useless for small arrays, but shouldn't hurt either)
  • +1 on not using hasOwnProperty when not necessary
Owner

ahocevar commented Apr 20, 2013

I consider caching length important and good practice. However, if the direction of your loop through the array does not matter, you can loop backwards and don't need a cache: for (var i = n.length - 1; i >= 0; --i).

I agree that hasOwnProperty should not be used when iterating through properties, including the need to document.

Contributor

twpayne commented Apr 20, 2013

However, if the direction of your loop through the array does not matter, you can loop backwards and don't need a cache: for (var i = n.length - 1; i >= 0; --i).

Hmmm, JSperf indicates that this has not great performance.

So, I'll update this PR to cache length.

twpayne merged commit 945d2f6 into openlayers:master Apr 20, 2013

1 check passed

default The Travis build passed
Details

twpayne deleted the twpayne:dont-use-foreach-in-loops branch Apr 20, 2013

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