make toggle(true/fales) and toggleClass("...", true/false) shortcut to show/hide and add/removeClass #1271

Closed
wants to merge 3 commits into
from

4 participants

@jbedard

The .toggle methods both construct new jQuery objects for each element to test the current state, but in the case where a boolean is passed this isn't needed.

This change avoids the new jQuery objects for cases where a boolean is passed to the toggle methods.

It adds a few bytes in its current state, and adds an extra code path, but it also simplifies the loops a bit since the boolean case no longer goes into the loops. This will also remove a closure from .toggle() and will pass all the elements at once to showHide() which minimizes reflow when you pass multiple elements.

raw gz Compared to master
+122 +16 dist/jquery.js
+50 +3 dist/jquery.min.js

The +3 can probably be removed. For example, inlining the "var type = typeof value" actually makes it smaller but then does a typeof within the .each (which might be fine..?):

raw gz Compared to master
+121 +6 dist/jquery.js
+60 -3 dist/jquery.min.js

@dmethvin
jQuery Foundation member

I'm not clear on the benefit of this change. Both .toggle() and .toggleClass() are likely to cause a reflow which would be one or two orders of magnitude slower than any perf advantage gained by avoiding some JS execution. At least that would be my guesstimate.

@dmethvin
jQuery Foundation member
@jbedard

The reflow would normally be much bigger then avoiding some simple jQuery objects or closures but I thought it might still be worth it. Some cases will also have no reflow (not in DOM, toggleClass having no effect).

Here's an example: http://jsperf.com/cost-of-toggle

With the change .toggle(true/false) is just as fast as .show/hide. Maybe it is skipping the reflow? I'd think the numbers would be higher if it was skipping the reflow though...

@jbedard

The showHide() helper also seems to optimize groups of elements to "avoid the constant reflow", this change will call showHide() with all the elements at once instead of individually (for the case of .toggle(true/false).

@jbedard

Although after adding a .position() after everything the difference is much smaller (which makes more sense): http://jsperf.com/cost-of-toggle/2

@mgol
jQuery Foundation member

I don't see how to reduce it further, all inlining is already done by UglifyJS here.

I'm a little torn on this; in some cases it might, indeed, increase performance; on the other hand, currently it adds 4 bytes to the gzipped version.

@mgol
jQuery Foundation member

Test reduced to just showing an element: http://jsperf.com/cost-of-toggle/3

@jbedard

I found inlining the "var type = typeof value" in toggleClass made the gz -3 instead of +3 bytes. Uglify wouldn't inline that would it? (It doesn't seem to for me).

After pulling the numbers change a bit, now it is +4, and -2 if the "var type..." is inlined.

@dmethvin
jQuery Foundation member

I'm not worried about a few bytes either way, but more whether adding another execution path provides real performance benefit. If not, let's keep the code simple. Thoughts?

@mgol
jQuery Foundation member

@dmethvin The question is what is "real performance benefit". http://jsperf.com/cost-of-toggle/3 shows substantial change but it's not a real-life scenario.

Anyway, I don't see it as adding another execution path but rather as checking one condition before so that we might avoid one execution path.

I'm slightly in favor of this patch though I won't cry if you decide not to pull it.

@gibson042
jQuery Foundation member

I'm slightly in favor of this patch though I won't cry if you decide not to pull it.

I agree.

@dmethvin
jQuery Foundation member

Reading this again, it actually made the code a bit clearer since it got rid of isBool

@dmethvin dmethvin closed this in e53a919 Jun 3, 2013
@dmethvin dmethvin added a commit that referenced this pull request Jun 3, 2013
@jbedard jbedard Avoid jQuery(this) and a closure for .toggle(Boolean), close gh-1271.
(cherry picked from commit e53a919)
9a3683b
@jstonne jstonne added a commit to foundry-modules/jquery that referenced this pull request Nov 15, 2013
@jstonne jstonne Merge commit '3b0b953d98bd13efd8c6b37d5318c5bf1e74931c'
# By Richard Gibson (61) and others
# Via Dave Methvin
* commit '3b0b953d98bd13efd8c6b37d5318c5bf1e74931c': (186 commits)
  Tagging the 1.10.2 release.
  Update Sizzle to version 1.10.2
  Change the changelog header style (cherry picked from commit a44202d)
  Revert "Fix #13983. Switch to //# for sourcemaps."
  Fix #13983. Switch to //# for sourcemaps.
  Fix test for #13937 ticket. Close gh-1299 (cherry picked from commit 308980e)
  Update AUTHORS.txt
  Fixes #14049: don't append px to CSS order value. Close gh-1300. (cherry picked from ec6eb38)
  Update Sizzle: bower manifest
  Moved too-early assignment inside the if stmt where the var is actually used. Close gh-1292. (cherry picked from commit 3a43443)
  Build: Update testswarm task to node-testswarm 1.x API
  Update Sizzle. Avoid the use of frameElement. Fixes #13980.
  Avoid jQuery(this) and a closure for .toggle(Boolean), close gh-1271. (cherry picked from commit e53a919)
  Fix #13974: XML .attr("type")
  No ticket: correct typo in an example in README.md. Close gh-1281. (cherry-picked from a0aa623)
  Updating the source version to 1.10.2-pre
  Tagging the 1.10.1 release.
  Fix #13789: Don't throw when module === null. Close gh-1269. Close gh-1280. (cherry-picked from eabb56c)
  Sizzle Update: fix #13936 - iframe reload should not affect Sizzle.
  Fix #13937: Correctly scope .finish() following multi-element .animate(). Thanks @gnarf37. Close gh-1279.
  ...

Conflicts:
	package.json
	src/.jshintrc
	src/core.js
	src/deprecated.js
	src/exports.js
8a2f018
@mescoda mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014
@jbedard jbedard Avoid jQuery(this) and a closure for .toggle(Boolean), close gh-1271.
(cherry picked from commit e53a919)
8c74559
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment