#10733 closed bug (fixed)

Opened November 09, 2011 01:32PM UTC

Closed November 11, 2011 01:57PM UTC

Last modified March 08, 2012 06:23PM UTC

remove uses of jQuery.each in css module in favor of a for loop

Reported by:	mikesherov	Owned by:	mikesherov
Priority:	low	Milestone:	1.7.1
Component:	css	Version:	1.7
Keywords:		Cc:
Blocked by:		Blocking:

Description

as far as I can tell, using $.each is monstrously slow compared to a regular for loop. See here: http://jsperf.com/for-loop-vs-each/6

In interest of performance, should we avoid using it internally when possible.

1.8?

Attachments (0)

Change History (20)

Changed November 09, 2011 02:19PM UTC by timmywil comment:1

component:	unfiled → misc
keywords:	→ 1.8-discuss
priority:	undecided → low
status:	new → open

In that perf, you're still talking millions. I don't think we need to worry about the use of $.each defining functions and the like, but we should be concerned about the performance of the most commonly used functions (and I think this is already done internally for the most part).

Changed November 09, 2011 02:20PM UTC by timmywil comment:2

-1, I'm not against performance improvements, but we should balance performance with size, maintainability, and readability.

Changed November 09, 2011 03:04PM UTC by mikesherov comment:3

timmywil, to your point, there are only a few places where .each is used in actual function execution, and one of them is css.js. Can we change the title to REMOVE USES OF JQUERY.EACH INTERNALLY IN FAVOR OF A FOR LOOP WHEN IMPACTING FUNCTION EXECUTION

I'll adress each point for just that example:

size: replacing jQuery.each with a for loop in getWH(), the code when gzipped and minified is one byte less.

readability: is a for loop less readable than $.each? Maybe.

maintainability: is a for loop less maintainable than $.each? Maybe.

perf: no choice but to be faster, but I'll make a perf to prove it. It might be negligible.

If the perf is negligible and the savings is only 1 byte, than perhaps $.each is more readable enough to warrant not changing.

Thanks for the response.

Changed November 09, 2011 03:10PM UTC by timmywil comment:4

_comment0:	@mikesherov: actually, I'd be in favor of the change in getWH. That is something that does get used a lot possibly called many times per second in certain use cases. → 1320851477591495

@mikesherov: actually, I'd be in favor of the change in getWH. That is something that does get used a lot and possibly called many times per second in certain use cases.

Changed November 09, 2011 03:18PM UTC by timmywil comment:5

Please feel free to present more spots you think we could benefit from this change. Changing my vote to +0, depending on the place.

Changed November 09, 2011 06:31PM UTC by mikesherov comment:6

Ok. Will do.

Changed November 10, 2011 01:05AM UTC by mikesherov comment:7

So, it seems that .each is used mostly for function definition except for:

effects.js:
the .step() function when done to restore offset properties.
the genFx function which seems to get called once per animation
manipulation.js
when eval'ing scripts in .domManip()
css.js
where I already mentioned

Seems like only in .css is worth it. Please change ticket name to reflect only in .css. I'll submit a perf and PR shortly.

Changed November 10, 2011 01:15AM UTC by rwaldron comment:8

owner:	→ mikesherov
status:	open → assigned
summary:	remove uses of jQuery.each internally in favor of a for loop → remove uses of jQuery.each in css module in favor of a for loop

Changed November 10, 2011 02:34AM UTC by mikesherov comment:9

savings minimal: http://jsperf.com/each-vs-for-loop-in-css-js

Changed November 10, 2011 03:01AM UTC by rwaldron comment:10

Not surprised that modern browsers are just fast. What about IE6, 7?

Changed November 10, 2011 01:09PM UTC by mikesherov comment:11

I don't have IE7 and IE6 on my laptop, anyone else willing to test it in IE7 and IE6 for me?

Changed November 10, 2011 01:59PM UTC by mikesherov comment:12

IE7 seems negligible. IE8 / safari / opera / and FF seem to have decent speed ups, on the the order of 10 - 20%.

I'll submit the PR, and leave it up to you guys on whether it's less maintainable or not.

Changed November 10, 2011 02:15PM UTC by timmywil comment:13

component:	misc → css
milestone:	None → 1.8

Sounds good to me.

Changed November 10, 2011 04:26PM UTC by rwaldron comment:14

@mikesherov say hello to your new BFF: http://www.browserstack.com

Changed November 11, 2011 02:25AM UTC by mikesherov comment:15

https://github.com/jquery/jquery/pull/590

Changed November 11, 2011 01:50PM UTC by mikesherov comment:16

please close this ticket as the PR has been merged.

Changed November 11, 2011 01:57PM UTC by dmethvin comment:17

resolution:	→ fixed
status:	assigned → closed

Changed November 11, 2011 03:12PM UTC by jaubourg comment:18

Replying to [comment:16 mikesherov]: