Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #7974: Keep animations synchronized and cleanup effects.js #177

Closed
wants to merge 31 commits into from

Conversation

@louisremi
Copy link
Contributor

louisremi commented Jan 13, 2011

small changes to effect.js that:

  • synchronize animations
  • reduce function calls, jQuery.now() calls in particular
  • reduce number of out-of-scope variables lookups
  • cache more variables to reduce file-size once minified
  • simplifies easing resolution code
  • add some comments and improve code readability here and there

It yields some interesting performance improvement, as demonstrated in this example: http://jsfiddle.net/V3Xqt/6/ (remove patched effects.js in "manage ressources" menu and hit 'run' again to see the difference)

  • function calls: 1148 -> 981 = 14% better
  • average execution time: 13.2 -> 11.8 = 11% better

/!\ example might be unrealistic /!\

As it keeps animations synchronized, it fixes http://bugs.jquery.com/7917 and addresses a similar problem to http://bugs.jquery.com/6281

@davidmurdoch

This comment has been minimized.

Copy link
Contributor

davidmurdoch commented Jan 13, 2011

got any before/after benchmarks?

…ated elements.

Side effect: fixes #7917, synchronize animations FTW!
@louisremi

This comment has been minimized.

Copy link
Contributor Author

louisremi commented Jan 14, 2011

Yep,

I've built a jsFiddle with my updated effects.js added on top of jquery 1.4.4 http://jsfiddle.net/louisremi/V3Xqt/5/ (remove effects.js in "manage ressources" to see the difference)
In my test case (which might be unrealistic), it shows a 13% reduction in function calls (1177 -> 1022) and a 10% improvement in execution time (26.1ms -> 23.4ms in average)

Tell me what you see.

@louisremi

This comment has been minimized.

Copy link
Contributor Author

louisremi commented Jan 14, 2011

And here is the ticket: http://bugs.jquery.com/ticket/7974

@rwaldron

This comment has been minimized.

Copy link

rwaldron commented on src/effects.js in fe350a3 Mar 14, 2011

for the sake of character saving, can we rename this something shorter? It's used 5 places and kinda verbose :D

This comment has been minimized.

Copy link
Owner

louisremi replied Mar 14, 2011

But it will be trimmed to one character at minification, won't it?
What about "timestamp", "fxCur", "curTime"?

@jeresig

This comment has been minimized.

Copy link
Member

jeresig commented Apr 12, 2011

That has been superseded by timmywil's pull.

@jeresig jeresig closed this Apr 12, 2011
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.