Bug Tracker

Modify

Ticket #12436 (closed bug: fixed)

Opened 20 months ago

Last modified 19 months ago

Performance degradation with delegate events and pseudo-classes

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

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.

Change History

comment:1 Changed 20 months ago by timmywil

  • Cc dmethvin added
  • Priority changed from undecided to low
  • Status changed from new to open
  • Component changed from unfiled to event

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.

comment:2 Changed 20 months ago by dmethvin

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.

comment:3 Changed 20 months ago by menno.vanslooten@…

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.

comment:4 Changed 20 months ago by josh@…

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.

comment:5 Changed 20 months ago by dmethvin

  • Owner set to dmethvin
  • Status changed from open to assigned

@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.

comment:6 Changed 20 months ago by dmethvin

  • Priority changed from low to blocker
  • Milestone changed from None to 1.8.2

comment:7 Changed 20 months ago by josh@…

@dmethvin yeah, no custom pseudo extensions.

Reverting this commit basically fixes the issue for us.  https://github.com/jquery/jquery/commit/afd717df9e189d3e374ce938f03e2712f31c29e3

comment:8 Changed 20 months ago by Dave Methvin

  • Status changed from assigned to closed
  • Resolution set to fixed

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

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

Changeset: 9b67b4c0ef311f96d6e035fd38bbfbe42a82b392

comment:9 Changed 20 months ago by mikesherov

#12510 is a duplicate of this ticket.

comment:10 Changed 20 months ago by timmywil

#12513 is a duplicate of this ticket.

comment:11 Changed 20 months ago by timmywil

#12543 is a duplicate of this ticket.

comment:12 Changed 19 months ago by latchkey

I think this just broke my site.

 http://jsfiddle.net/ZzPtf/

Works fine with 1.8.1, but breaks with 1.8.2.

comment:13 Changed 19 months ago by dmethvin

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

comment:14 Changed 19 months ago by info@…

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.

comment:15 Changed 19 months ago by mikesherov

  • Cc timmywil added; dmethvin removed

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

comment:16 Changed 19 months ago by dmethvin

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.

comment:17 Changed 19 months ago by timmywil

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.

Last edited 19 months ago by timmywil (previous) (diff)

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.