Skip to main content

Bug Tracker

Side navigation

#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: unfiledmisc
keywords: → 1.8-discuss
priority: undecidedlow
status: newopen

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: openassigned
summary: remove uses of jQuery.each internally in favor of a for loopremove uses of jQuery.each in css module in favor of a for loop

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

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: misccss
milestone: None1.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

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: assignedclosed

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

Replying to [comment:16 mikesherov]:

please close this ticket as the PR has been merged.

Mike, use "Fixes #XXX" where XXX is the id of the ticket in your commit message next time and the ticket will be closed automagically ;)

Changed November 11, 2011 03:17PM UTC by mikesherov comment:19

yeah, I missed on that one.

Changed November 11, 2011 05:00PM UTC by timmywil comment:20

keywords: 1.8-discuss
milestone: 1.81.7.1

OR, if dave used pulley... ;)