Skip to main content

Bug Tracker

Side navigation

#12436 closed bug (fixed)

Opened August 31, 2012 09:39AM UTC

Closed September 10, 2012 01:30AM UTC

Last modified October 16, 2012 12:46PM UTC

Performance degradation with delegate events and pseudo-classes

Reported by: menno.vanslooten@gmail.com Owned by: dmethvin
Priority: blocker Milestone: 1.8.2
Component: event Version: 1.8.1
Keywords: Cc: timmywil
Blocked by: Blocking:
Description

Steps to reproduce

Bind a delegate event handler to the document where the selector contains a pseudo-class.

Expected result:

The number of internal calls for the handler is only dependent on the DOM depth of the element that receives the event.

Actual result

The number of internal calls is dependent on the total size and complexity of the DOM.

Example

If a delegate event handler is added to the document like so:

$(document).on('click', 'button', function() {});

The number of calls internally in jQuery when an element is clicked is only dependent on the DOM depth of the element clicked.

But if such a handler is added for a pseudo selector:

$(document).on('click', ':submit', function() {});

The number of internal calls is now suddenly dependent on the total size of the DOM and grows very quickly.

Open the following jsfiddles in Firefox (+firebug):

http://jsfiddle.net/mennovanslooten/DySJq/4/

This fiddle has a single paragraph and results in 223 internal calls.

http://jsfiddle.net/mennovanslooten/EZGys/5/

This fiddle has 10 paragraphs and results in 295 internal calls. Even though the paragraphs are in parts of the DOM that are not affected by the event.

http://jsfiddle.net/mennovanslooten/u9Cmr/1/

This fiddle uses the jquery.com homepage source as an example of a more complex DOM and results in 2993 calls.

The number of calls grows very quickly. For a project I'm currently working on it's almost 12K on some pages resulting in seconds of latency.

Attachments (0)
Change History (17)

Changed August 31, 2012 03:47PM UTC by timmywil comment:1

cc: → dmethvin
component: unfiledevent
priority: undecidedlow
status: newopen

The expected result is invalid. You're delegating from the document, which is the entire DOM. Take advantage of event delegation by limiting it to a narrower context and it won't have to check every parent.

However, the fiddles made me notice something else. Rather than doing a selection and using .index(), perhaps .is() with contains() would be faster.

Changed August 31, 2012 05:01PM UTC by dmethvin comment:2

Ouch! Yes, we should get matchesSelector goodness where possible. Although it seems like the killer here is just unwise selector and delegation point choice. The importance of both is documented in the Event Performance discussion, I gave a presentation on it last year, I've warned about it in other bug tickets, and mentioned it in commit messages.

Changed August 31, 2012 11:37PM UTC by menno.vanslooten@gmail.com comment:3

The expected result is invalid. You're delegating from the document, which is the entire DOM. Take advantage of event delegation by limiting it to a narrower context and it won't have to check every parent.

I know it's bad practice but to say the expected result is invalid is not doing the issue justice, I feel. Neither 1.8.0 nor 1.7.2 show this behavior. Nor is it showing this behavior when a non-pseudo selector is used.

Although it seems like the killer here is just unwise selector and delegation point choice.

I won't deny that it's not the best idea I've ever had. To me the killer is that it ''looks like'' jQuery checks the entire DOM in stead of only the chain of ancestors whenever a pseudo selector is involved. Has something changed internally making this a worse idea than it was in the past? I just want to understand.

Changed September 05, 2012 05:04AM UTC by josh@joshpeek.com comment:4

Just hit the same issue on github.com. Upgraded to 1.8.1 earlier today and clicking anything on a big dom was painfully slow.

I can look into a patch to Sizzle if someone isn't already working on one.

Changed September 05, 2012 01:14PM UTC by dmethvin comment:5

owner: → dmethvin
status: openassigned

@josh, what do your selectors look like? If they're standard CSS3 selectors the problem should be fixed when we switch back to .is() which will end up using the browser's matchesSelector.

Changed September 05, 2012 01:15PM UTC by dmethvin comment:6

milestone: None1.8.2
priority: lowblocker

Changed September 05, 2012 04:17PM UTC by josh@joshpeek.com comment:7

@dmethvin yeah, no custom pseudo extensions.

Reverting this commit basically fixes the issue for us.

https://github.com/jquery/jquery/commit/afd717df9e189d3e374ce938f03e2712f31c29e3

Changed September 10, 2012 01:30AM UTC by Dave Methvin comment:8

resolution: → fixed
status: assignedclosed

Fix #12436, make delegated events fast again. Close gh-923.

Retains the rooted-at-delegateTarget behavior fixed in #12383 by afd717df9e.

Changeset: 9b67b4c0ef311f96d6e035fd38bbfbe42a82b392

Changed September 11, 2012 03:09PM UTC by mikesherov comment:9

#12510 is a duplicate of this ticket.

Changed September 11, 2012 03:29PM UTC by timmywil comment:10

#12513 is a duplicate of this ticket.

Changed September 14, 2012 09:53PM UTC by timmywil comment:11

#12543 is a duplicate of this ticket.

Changed September 24, 2012 08:39PM UTC by latchkey comment:12

I think this just broke my site.

http://jsfiddle.net/ZzPtf/

Works fine with 1.8.1, but breaks with 1.8.2.

Changed September 24, 2012 08:42PM UTC by dmethvin comment:13

@latchkey, that was never valid. The selector must be a string. This is a closed ticket. Ask for help on the forum.

Changed October 16, 2012 11:38AM UTC by info@alex-funk.net comment:14

The problem is still existent.

We use something like this in our code and you can feel the performance decrease since 1.8.1. (Commit: https://github.com/jquery/jquery/commit/afd717df9e189d3e374ce938f03e2712f31c29e3)

$("#elementWithID").on('click','.elem:eq(1)', function(){...});

Changeset: https://github.com/jquery/jquery/commit/9b67b4c0ef311f96d6e035fd38bbfbe42a82b392 didn't solve the performance issue.

Tested with:

v1.8.3pre d70e64bff2660cf5a801fe818f1170d5ea93e4fe

and 1.8.2

We can't use pseudo-selectors in our delegation events as long this issuse is present.

Changed October 16, 2012 11:45AM UTC by mikesherov comment:15

cc: dmethvintimmywil

Timmy, thoughts? Needsdocs? Or is there still another fix here?

Changed October 16, 2012 12:27PM UTC by dmethvin comment:16

Please don't throw comments onto a closed ticket if a problem still exists. Instead, open a new ticket WITH a test case so we can investigate.

Changed October 16, 2012 12:46PM UTC by timmywil comment:17

_comment0: The links don't help much. "All pseudo selectors" is an overstatement. It is only combinators (which will use qsa) and POS selectors (which won't) that still use a selection with .index. All other selectors will do a simple contextual match and nonlinger have the performance regression.1350391687382993

The links don't help much. "All pseudo selectors" is an overstatement. It is only combinators (which will use qsa) and POS selectors (which won't) that still use a selection with .index. All other selectors will do a simple contextual match and no longer have the performance regression.