Remove value var form each function. #1510

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

danielhusar commented Feb 7, 2014

Hi guys,

I was thinking there no reason for having value var in "each" function as there no other operations with it.
But if its something on purpose I missed from style guide, sorry for PR and ignore :)

Owner

dmethvin commented Feb 7, 2014

I tend to agree, can't really see why it's needed. Perhaps there was some perf reason? Can you put together a jsperf to see if that's the case? I'd be interested in seeing whether the "special fast case" is really faster in modern browsers as well.

Member

jaubourg commented Feb 7, 2014

I'd be interested in seeing whether the "special fast case" is really faster in modern browsers as well.

My thought exactly.

Owner

dmethvin commented Feb 7, 2014

Interesting! I ran some more browsers through, the one marked Other is IE11.

My feeling is that the "special fast case" isn't worth the extra code, it's a hair faster but if this loop overhead was truly an issue then people would be screaming that Firefox is unusable given its bad performance against the others. And if perf is truly an issue you shouldn't be using .each() or Array#forEach you should use a loop.

Anyone else?

Member

jaubourg commented Feb 7, 2014

The results in firefox are frightening but since they're also consistent between the two paths in the most recent version, I'd be in favour of simplifying the code.

Member

gibson042 commented Feb 7, 2014

And if perf is truly an issue you shouldn't be using .each() or Array#forEach you should use a loop.

👍
These all look close enough, and therefore probably not worth the size and complexity.

Edit: Except that the two modes are more different than they appear on first inspection. 😦
Still worth a look, though.

Contributor

danielhusar commented Feb 10, 2014

Yeah they are not exactly the same, and removing this block will cause 2 ajax tests to fail.

@dmethvin dmethvin added this to the 1.12/2.2 milestone Mar 13, 2014

Member

gibson042 commented Sep 4, 2014

Thanks again for the contribution! I'm ready to land this for 1.12/2.2, but you first need to sign our CLA with your GitHub email address.

Contributor

staabm commented Sep 4, 2014

In FF33 the fast path seems to be 2x faster then the usual path, see jsperfs
(and FF33 seems the fastest browser over all now, for the fast-case)

Contributor

danielhusar commented Sep 4, 2014

Thanks, I have signed the CLA already.

Owner

dmethvin commented Dec 4, 2014

Where does this stand? It sounded like @gibson042 was ready to land but it stalled.

@gibson042 gibson042 closed this in eeda11c Dec 9, 2014

gibson042 added a commit that referenced this pull request Dec 9, 2014

Core: Simplify and speed up .each
Closes gh-1510

(cherry picked from commit eeda11c)

@gibson042 gibson042 added the Core label Dec 9, 2014

Member

gibson042 commented Dec 9, 2014

Sorry about the delay!

Contributor

danielhusar commented Dec 9, 2014

No worries, thanks for merging it :)

markelog added a commit that referenced this pull request Nov 10, 2015

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