Bug Tracker

Modify

Ticket #10733 (closed bug: fixed)

Opened 2 years ago

Last modified 2 years ago

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:
Blocking: Blocked by:

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?

Change History

comment:1 Changed 2 years ago by timmywil

  • Keywords 1.8-discuss added
  • Priority changed from undecided to low
  • Status changed from new to open
  • Component changed from unfiled to misc

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).

comment:2 Changed 2 years ago by timmywil

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

comment:3 Changed 2 years ago by mikesherov

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.

comment:4 Changed 2 years ago by timmywil

@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.

Version 0, edited 2 years ago by timmywil (next)

comment:5 Changed 2 years ago by timmywil

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

comment:6 Changed 2 years ago by mikesherov

Ok. Will do.

comment:7 Changed 2 years ago by mikesherov

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.

comment:8 Changed 2 years ago by rwaldron

  • Owner set to mikesherov
  • Status changed from open to assigned
  • Summary changed from remove uses of jQuery.each internally in favor of a for loop to remove uses of jQuery.each in css module in favor of a for loop

comment:9 Changed 2 years ago by mikesherov

comment:10 Changed 2 years ago by rwaldron

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

comment:11 Changed 2 years ago by mikesherov

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

comment:12 Changed 2 years ago by mikesherov

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.

comment:13 Changed 2 years ago by timmywil

  • Component changed from misc to css
  • Milestone changed from None to 1.8

Sounds good to me.

comment:14 Changed 2 years ago by rwaldron

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

comment:16 follow-up: ↓ 18 Changed 2 years ago by mikesherov

please close this ticket as the PR has been merged.

comment:17 Changed 2 years ago by dmethvin

  • Status changed from assigned to closed
  • Resolution set to fixed

comment:18 in reply to: ↑ 16 Changed 2 years ago by jaubourg

Replying to 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 ;)

comment:19 Changed 2 years ago by mikesherov

yeah, I missed on that one.

comment:20 Changed 2 years ago by timmywil

  • Keywords 1.8-discuss removed
  • Milestone changed from 1.8 to 1.7.1

OR, if dave used pulley... ;)

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.