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

elem.is(':visible') calls native webkitMatchesSelector #2042

Closed
paulirish opened this Issue Jan 29, 2015 · 13 comments

Comments

Projects
None yet
7 participants
@paulirish
Member

paulirish commented Jan 29, 2015

Yesterday I spent some time profiling the effect shown in this article:
http://engineeringblog.yelp.com/2015/01/animating-the-mobile-web.html (and here's the live m.yelp page)

image

During profiling I some interesting things, but two of which are specific to jQuery.

  1. webKitMatchesSelector is being called during elem.is(':visible')
    2 ) jQuery handler for touchmove seems very slow. (This one we can punt to another ticket.)

A closer look at the is to matchesSelector stack:
image

Here's the a.rl method we're looking at:

a.rl = function(b) {
            return b.qu.is(":visible") || b.Ts.is(":visible")
        };

In total, this stack from a.rl through webKitMatchesSelector accounts for a full 15% of all time on CPU (including JS, recalc, paint, etc) during this effect, so certainly not trivial.

But this still strikes me as odd in the first place.

I don't think we would ever want a native qSA or matchesSelector to handle this sort of call, would we?

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jan 29, 2015

Member

We don't try to parse the selector in JS until we run it through the native method. If the native method throws an exception, we try parsing it ourselves. The only other way we could know to avoid it would be to parse the string ourselves, which would penalize people who use just standard selectors.

In the talks I do, I strongly recommend against :visible or :hidden as selectors. Besides being jQuery custom selectors and having to take the Sizzle path after being rejected by native methods, they also force layout if it's dirty. Most of the time it's easily trackable by classes. The docs warn about this as well.

Member

dmethvin commented Jan 29, 2015

We don't try to parse the selector in JS until we run it through the native method. If the native method throws an exception, we try parsing it ourselves. The only other way we could know to avoid it would be to parse the string ourselves, which would penalize people who use just standard selectors.

In the talks I do, I strongly recommend against :visible or :hidden as selectors. Besides being jQuery custom selectors and having to take the Sizzle path after being rejected by native methods, they also force layout if it's dirty. Most of the time it's easily trackable by classes. The docs warn about this as well.

@paulirish

This comment has been minimized.

Show comment
Hide comment
@paulirish

paulirish Jan 29, 2015

Member

In the talks I do, I strongly recommend against :visible or :hidden as selectors

Great. In this and with Wikipedia's new visual editor, a tremendous amount of the app's slowness is directly attributable to using those. It's baaad. But anyway, moving on.

If the native method throws an exception, we try parsing it ourselves.

I see that now. In the case of obviously invalid selectors, it'd be better to not even go down the patch of bouncing off the exception.

image

Follow the total time column above. is() is ~111ms. In the end, 69ms of that is matchesSelector just returning an exception and 18ms is Sizzle() doing its thing. (which it does, everytime)

The overhead to throw an extra regex test along with the other two here is going to be sub-millisecond on this timescale. And since we're already running two regex's against the string, the overall VM overhead will be minimal.

Member

paulirish commented Jan 29, 2015

In the talks I do, I strongly recommend against :visible or :hidden as selectors

Great. In this and with Wikipedia's new visual editor, a tremendous amount of the app's slowness is directly attributable to using those. It's baaad. But anyway, moving on.

If the native method throws an exception, we try parsing it ourselves.

I see that now. In the case of obviously invalid selectors, it'd be better to not even go down the patch of bouncing off the exception.

image

Follow the total time column above. is() is ~111ms. In the end, 69ms of that is matchesSelector just returning an exception and 18ms is Sizzle() doing its thing. (which it does, everytime)

The overhead to throw an extra regex test along with the other two here is going to be sub-millisecond on this timescale. And since we're already running two regex's against the string, the overall VM overhead will be minimal.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jan 29, 2015

Member

I'll pull in @timmywil and @gibson042 here, maybe we could do a quick regex to look for obvious offenders? Discussing the issue in IRC, perhaps we should move things like :hidden and other really slow custom selectors to jQuery Migrate in 4.0. Of course that brings up some Sizzle questions as well.

Member

dmethvin commented Jan 29, 2015

I'll pull in @timmywil and @gibson042 here, maybe we could do a quick regex to look for obvious offenders? Discussing the issue in IRC, perhaps we should move things like :hidden and other really slow custom selectors to jQuery Migrate in 4.0. Of course that brings up some Sizzle questions as well.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Jan 29, 2015

Member

This suggestion is essentially to match custom pseudos in rbuggyQSA, which would save legitimate uses but force false positives (e.g., [attr-name=":visible"]) down the slow path—in other words, a net win. However, identifying them will be a trick, given that our extension mechanism is just property assignment to the same object containing matchers for standard pseudos.

As for removing custom pseudos from standard builds, I see :hidden, :visible, and :animated in core, and would not mourn the loss of any (although that last one is essentially impossible to add back without knowledge of undocumented effects internals). We may also want to look at jQuery UI while we're at it: http://api.jqueryui.com/category/selectors/.

Member

gibson042 commented Jan 29, 2015

This suggestion is essentially to match custom pseudos in rbuggyQSA, which would save legitimate uses but force false positives (e.g., [attr-name=":visible"]) down the slow path—in other words, a net win. However, identifying them will be a trick, given that our extension mechanism is just property assignment to the same object containing matchers for standard pseudos.

As for removing custom pseudos from standard builds, I see :hidden, :visible, and :animated in core, and would not mourn the loss of any (although that last one is essentially impossible to add back without knowledge of undocumented effects internals). We may also want to look at jQuery UI while we're at it: http://api.jqueryui.com/category/selectors/.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jan 29, 2015

Member

I'm a little surprised it takes that long for webkitMatchesSelector to parse the selector and determine whether it's invalid. I wonder if that can be improved natively.

Member

timmywil commented Jan 29, 2015

I'm a little surprised it takes that long for webkitMatchesSelector to parse the selector and determine whether it's invalid. I wonder if that can be improved natively.

@timmywil timmywil added the Selector label Jan 30, 2015

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Feb 2, 2015

Member

Could we cache the fact that the selector failed qSA or mS and skip it for subsequent ones? Actually, if you have an entry in the Sizzle cache doesn't that mean it must have failed previously?

Member

dmethvin commented Feb 2, 2015

Could we cache the fact that the selector failed qSA or mS and skip it for subsequent ones? Actually, if you have an entry in the Sizzle cache doesn't that mean it must have failed previously?

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Feb 2, 2015

Member

Hmm, that's not a bad idea. @gibson042 ?

Member

timmywil commented Feb 2, 2015

Hmm, that's not a bad idea. @gibson042 ?

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Feb 2, 2015

Member

Yeah, I like that a lot. It's not a panacea when the selectors theirselves are different, but it avoids a lot of pain for little effort.

Member

gibson042 commented Feb 2, 2015

Yeah, I like that a lot. It's not a panacea when the selectors theirselves are different, but it avoids a lot of pain for little effort.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Feb 3, 2015

Member

Issue opened here. That should address this issue fairly well with little cost, as gibson said. We'll close this issue when we merge in that Sizzle update.

Member

timmywil commented Feb 3, 2015

Issue opened here. That should address this issue fairly well with little cost, as gibson said. We'll close this issue when we merge in that Sizzle update.

@timmywil timmywil added this to the 3.0.0 milestone Feb 3, 2015

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Feb 13, 2015

Member

@paulirish You'll be happy to know that we now see a 1600% improvement on :visible/:hidden. jquery/sizzle#315 (comment)

Member

timmywil commented Feb 13, 2015

@paulirish You'll be happy to know that we now see a 1600% improvement on :visible/:hidden. jquery/sizzle#315 (comment)

@arthurvr

This comment has been minimized.

Show comment
Hide comment
@arthurvr

arthurvr Feb 13, 2015

Member

Wow. Amazing!

Member

arthurvr commented Feb 13, 2015

Wow. Amazing!

@paulirish

This comment has been minimized.

Show comment
Hide comment
@paulirish

paulirish Feb 14, 2015

Member
Member

paulirish commented Feb 14, 2015

@danilanna

This comment has been minimized.

Show comment
Hide comment
@danilanna

danilanna Feb 20, 2015

Wow! This is so cool!

danilanna commented Feb 20, 2015

Wow! This is so cool!

@timmywil timmywil self-assigned this Feb 23, 2015

@timmywil timmywil closed this in 3a0dd5a Apr 13, 2015

timmywil added a commit that referenced this issue Apr 13, 2015

timmywil added a commit that referenced this issue Nov 10, 2015

@dmethvin dmethvin modified the milestones: 1.12/2.2, 3.0.0 Jan 7, 2016

@cssmagic cssmagic referenced this issue May 18, 2016

Open

jQuery #5

@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.