Skip to content

Loading…

Move offset check to the back in JQUERY.EXPR.FILTERS.HIDDEN to optimize ... #1027

Closed
wants to merge 1 commit into from

5 participants

@rspilker

...performance. Fixes #12885.

I did not test it because the instructions on how to run grunt are incomplete/broken. The 'grunt && grunt watch' command does build jQuery.js but no tests are run and no (web)server is started. AFAIK, grunt watch is not meant for this task. Shouldn't it be grunt test?

@staabm

Great find! Did you test it on Jsperf ?

@timmywil
jQuery Foundation member

@rspilker: Sorry for the confusion. grunt watch is there to help you build jQuery as you make changes. To actually test, follow the instructions right after that on setting up a local server with PHP (and opening test/index.html from localhost). https://github.com/jquery/jquery/blob/master/README.md

Also, it would be great to see perf tests so we can be sure this is actually faster. Even if the codepath for curCSS is not hit very often, that could potentially slow it down if there are multiple elements on the page that require it. I suggest making two tests: one with lots of different kinds of elements(i.e. elements with different tag names) that are hidden and one with only one hidden element on the page.

@gnarf
jQuery Foundation member

The ajax failure in the build went away after a retest.

@dmethvin
jQuery Foundation member

@rspilker can you sign the CLA at http://jquery.github.com/cla.html ?

@rspilker

Done.

@dmethvin
jQuery Foundation member

I am not sure about the perf. I was trying to test was whether gCS caused a reflow when asked for the display property, because we know that offsetHeight will do so if any dimensions have changed. Seems like it may vary widely by browser, or perhaps my perf is too crummy. http://jsperf.com/offsetheight-vs-gcs-display

@dmethvin
jQuery Foundation member

The bench from @timmywil wasn't very definitive either, although I had some problems running it on Chrome. I'd say we should leave the code as-is without proof that it improves, since the risk of regression is high. http://jsperf.com/jquery-hidden-1-8-vs-1-9

@rspilker don't be discouraged by this, please do let us know if you're interested in taking on any of the tickets in the tracker. If is the possibility of a perf regression it's always a good idea to do a jsperf.

@dmethvin dmethvin closed this
@rspilker

I'm not discouraged. I understand your reasoning and agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 12, 2012
  1. @rspilker

    Move offset check to the back in JQUERY.EXPR.FILTERS.HIDDEN to optimi…

    rspilker committed
    …ze performance. Fixes #12885.
This page is out of date. Refresh to see the latest.
Showing with 1 addition and 1 deletion.
  1. +1 −1 src/css.js
View
2 src/css.js
@@ -597,7 +597,7 @@ jQuery(function() {
if ( jQuery.expr && jQuery.expr.filters ) {
jQuery.expr.filters.hidden = function( elem ) {
- return ( elem.offsetWidth === 0 && elem.offsetHeight === 0 ) || (!jQuery.support.reliableHiddenOffsets && ((elem.style && elem.style.display) || curCSS( elem, "display" )) === "none");
+ return (!jQuery.support.reliableHiddenOffsets && ((elem.style && elem.style.display) || curCSS( elem, "display" )) === "none") || ( elem.offsetWidth === 0 && elem.offsetHeight === 0 );
};
jQuery.expr.filters.visible = function( elem ) {
Something went wrong with that request. Please try again.