Bug Tracker

Ticket #7341 (closed bug: fixed)

Opened 4 years ago

Last modified 16 months ago

Slow .add() in IE

Reported by: markus@… Owned by: rwaldron
Priority: high Milestone: 1.6.1
Component: selector Version: 1.4.3
Keywords: Cc: rwaldron
Blocking: Blocked by:

Description

The .add()-function performance is significantly poorer in IE (7 & 8), when using jQuery 1.4.3 vs. older 1.4.2.

Just adding to a collection of a few hundred elements can cause the "Stop running script" alert.

Demo:  http://jsfiddle.net/ZNf8b/6/

Change History

comment:1 Changed 4 years ago by SlexAxton

Can someone with a native IE verify?

comment:2 Changed 4 years ago by rwaldron

  • Owner set to rwaldron
  • Status changed from new to assigned
  • Component changed from unfiled to manipulation

Alex, I'll take a look at this.

comment:3 Changed 4 years ago by addyosmani

  • Cc rwaldron added

Did we manage to take another look at this? :)

comment:4 Changed 4 years ago by rajiv.delwadia@…

This is because add() does a uniqueSort(), and the sortOrder comparison that is used in IE changed in 1.4.3.

The old comparison used .sourceIndex, but the new one builds 2 arrays of ancestors for practically every comparison.

Arguably, $("*",node).add(node) should be cheap, not requiring sorting and uniquifying.

comment:5 Changed 4 years ago by anonymous

I just ran into this and had to revert to jquery 1.4.2.

comment:6 Changed 4 years ago by DonJaime

This is not just a problem when add() is used explicitly, it also gets in the way with addSelf(), remove(), and replaceWith(). Acording to the profiler helpfully supplied along with IE8, add() calls uniqueSort(), which calls Array.sort(), which then calls Array.sort(), which calls an internal function thousands or tens of thousands of times, and that internal function calls Array.unshift tens or hundreds of thousands of times.

I had to make code significantly clunkier to keep some parts of our site usable in IE. The problem with remove() and replaceWith() mean that performance of AJAX page updates is still noticeably degraded in IE. I suggest you treat this with rather more urgency than 'did we manage to take another look at this'.

comment:7 Changed 4 years ago by anonymous

The problem is not just Array.shift/unshift. It was the change (in 1.4.3) for sortOrder() in IE from using sourceIndex to walking to the top of the DOM in every comparison.

comment:8 Changed 4 years ago by khs4473@…

I've forked Sizzle and added sourceIndex support back in. Check it out - I can send a pull request or whatever if you like.

 https://github.com/khs4473/sizzle/commit/6cbad8d304af05cf15c1d4365c4a791c45087d49

comment:10 Changed 4 years ago by john

  • Priority changed from undecided to high
  • Status changed from assigned to closed
  • Resolution set to wontfix
  • Milestone changed from 1.5 to 1.next

We're not planning on moving back to using sourceIndex - sourceIndex was fraught with problems and caused all sorts of compatibility issues. While this particular technique is slower it's also correct - which is important.

comment:11 Changed 4 years ago by anonymous

I'm sorry, John, but I disagree; getting a long-running script error is not correct. Here is another approach to speeding up add():

When adding n arbitrary elements to a set of size m, concatenating and resorting takes (m+n)*log(m+n), whereas doing an insertion of the new elements take n*log(m). This would work because the first set is already known to be sorted. Given that m can be arbitrarily large, and n is usually 1, the insertion will invariably win.

In the case of a comma selector, it gets even better: both sets are known to be already sorted, so they just need a merge. n+m

comment:12 Changed 4 years ago by anonymous

sourceIndex is pretty thoroughly validated in my patch, but hey, it's your engine! : )

comment:13 Changed 4 years ago by anonymous

If something doesn't work properly then it is not correct, even if it is 'correct'.

comment:14 Changed 4 years ago by David Tong

just to add to the discussion, any app that lists a bunch of items with mouseover and mouseout events will trigger withinElement that ultimately calls andSelf() that calls Array.sort. 1.4.4 is slower than previous versions as indicated in the thread above, and on more complex apps, the performance is noticeably slower now.

comment:15 Changed 4 years ago by snover

#8065 is a duplicate of this ticket.

comment:16 Changed 4 years ago by ratibus@…

This performance issue in IE is major in my opinion. It broked parts of our applications because of this bug #8233 which is caused by this particular .add() issue.

Please reopen this ticket. It could not be acceptable to have jquery users facing "slow script" issue in IE because of internal implementation decisions.

comment:17 Changed 4 years ago by twatson@…

I have to agree, with ratibus. We see this issue when placing a large when calling $("#element").empty().html("LARGE CHUCNK OF HTML WITH SCRIPTS"). We do this in order to remove any previous handlers in elements in #element and to run any required scripts in the new HTML. Since 1.4.2 we have been able to update because our IE users are now seeing the long running script warning. Please reopen and provide a workaround. Or if you can suggest a better way to do what we have been doing I will take that! I will happily create fiddler cases to demonstrate that although you are doing the "correct" thing, you are infact making the library unusable.

comment:18 Changed 4 years ago by scott.gonzalez

There are multiple bugs filed against jQuery UI which are caused by this.

comment:19 Changed 4 years ago by anonymous

With all due respect, I have to add that this issue shouldn't be disregarded. What makes jQuery such a great and essential javascript library is how seamlessly it makes web development work across the legion of different browsers. This is especially true when having to deal with Internet Explorer which continues to be the thorn in the side of developers. Now if you make a fundamental change to the jQuery core that has not just a slight, but significant performance hit for IE then you are assailing one of jQuery's fundamental strengths. Also not only are you forcing people to have to stay at jquery 1.4.2 but you're preventing people from taking advantage of jQuery UI as well because of the decision to not find a better solution for this issue. I would hope that developers of such caliber to have been able to create jQuery wouldn't accept this sub par situation.

comment:20 Changed 4 years ago by anonymous

Agree with comment 19.

We've also hit this issue in an MS MVC3 project using the unobtrusive validation framework. The jquery.validate.js plugin is hitting .Add() and performance is so bad in IE8 that a production release is not an option. MS MVC framework seems to be getting good traction so I'm sure many others will run into this issue.

As such we are also roadblocked from moving past v1.4.2.

comment:21 Changed 4 years ago by nsegura3

This is also hitting the Dialog widget of jquery UI as the code that tries to give focus to the first focusable element uses the andSelf() function. We are getting many complains from our users getting the "script taking too long" warning when opening a dialog.

comment:22 follow-up: ↓ 25 Changed 4 years ago by anonymous

"While this particular technique is slower it's also correct - which is important."

John, this is about the dumbest unilateral stance I've ever seen someone take.

When we upgraded jQuery from 1.4.2 to 1.4.4, a simple function on a complex DOM that took 67ms in IE8 now takes over 3.4s. jQuery is now 50 times slower in IE8, which is hardly a metric that should be so lightly dismissed.

No one who runs a data-heavy production website can upgrade beyond 1.4.2 until this issue is fixed for IE8. You're shooting your own project in the foot.

comment:22 follow-up: ↓ 25 Changed 4 years ago by anonymous

"While this particular technique is slower it's also correct - which is important."

John, this is about the dumbest unilateral stance I've ever seen someone take.

When we upgraded jQuery from 1.4.2 to 1.4.4, a simple function on a complex DOM that took 67ms in IE8 now takes over 3.4s. jQuery is now 50 times slower in IE8, which is hardly a metric that should be so lightly dismissed.

No one who runs a data-heavy production website can upgrade beyond 1.4.2 until this issue is fixed for IE8. You're shooting your own project in the foot.

comment:24 Changed 4 years ago by ajpiano

#8561 is a duplicate of this ticket.

comment:25 in reply to: ↑ 22 Changed 4 years ago by asherh

Replying to anonymous: We are also stuck with 1.4.2 because of this checkSibling() IE problem. We have a page with 1.4.3+, UI 1.8.11, and a dialog with a large multiple select (hundreds of options). If you click on an option, IE 7/8 freezes in endless JS calls to checkSibling(). Please re-open, many users will be stuck with 1.4.2 until it is fixed. See also: http://bugs.jquery.com/ticket/7367  http://forum.jquery.com/topic/is-this-a-regression-or-a-known-issue-in-1-4-3-and-ie7-8  http://bugs.jqueryui.com/ticket/6644  http://core.trac.wordpress.org/ticket/16643

comment:26 Changed 4 years ago by anonymous

Also stuck on 1.4.2. I hope it gets resolved.

comment:27 Changed 4 years ago by justbeez

I completely understand that doing things correctly is important. However, there has to be a way to improve the performance of the "correct" way.

IE8 still has too much market share to just have broken interfaces that used to work. (That's a regression.)

I'm using jQuery 5.1.2 (can't revert due to dependencies) to filter and search data based on user input that triggers AJAX calls and returns a list of matching elements from a database. Even with caching all the wrapped sets and optimizing the selectors and setting contexts where possible, the filtering of those sets is just too slow in IE8 and results in the slow script warnings. And I'm only dealing with two sets of ~75 siblings each!

I completely understand that sourceIndex isn't the answer--I've run into some of it's issues before. But we need something that works in IE8 proficiently. If not, this can't be considered a cross-browser library anymore.

comment:28 Changed 4 years ago by cmcnulty

For future reference, here's the commit that has caused the performance regression:

 https://github.com/jquery/sizzle/commit/bb0828d44b6c9d360e6077187e1607076b6e67ab#sizzle.js

comment:29 follow-up: ↓ 41 Changed 4 years ago by brewdente@…

If anyone wants a quick and easy way to get around this issue, just include the code pasted here:

 http://codepaste.net/sygep8

after you include your jQuery file and you'll be back in business. Finally, you can update your jQuery!

comment:30 Changed 4 years ago by anonymous

This has caught our production environment out and we're looking to revert to 1.4.2 - a workable solution going forward would be great.

comment:31 Changed 4 years ago by anonymous

has the "correct" way been "fixed" in 1.6?

comment:32 Changed 4 years ago by anonymous

Has anybody tested 1.6 yet to see if this issue has been resolved?

comment:33 Changed 4 years ago by cmcnulty

1.6 has the same performance problem. I've tried to consolidate all of the info I've gathered about this issue into a perftest to demonstrate the severity of the issue once and for all.

 http://jsperf.com/jquery-1-4-3-perf-degrade/5

I've also created a jsfiddle to attempt to debunk the idea that the commit that caused these problems actually fixed the issue of disconnected nodes:

 http://jsfiddle.net/P8aPL/2/

I'm off to jquery forums to try to see if anyone is interested in taking another look at this issue.

Last edited 4 years ago by cmcnulty (previous) (diff)

comment:34 Changed 4 years ago by cmcnulty

I've opened a discussion of this topic  here.

Hopefully, we can suss out some better test cases for improving a plugin, and maybe, hopefully, someday, improving the sort performance of IE6,7,8 back to 1.4.3 levels.

comment:35 Changed 4 years ago by john

  • Status changed from closed to reopened
  • Resolution wontfix deleted
  • Milestone changed from 1.next to 1.6.1

comment:36 follow-up: ↓ 42 Changed 4 years ago by john

  • Status changed from reopened to closed
  • Resolution set to fixed
  • Component changed from manipulation to selector

Fixed.  https://github.com/jquery/sizzle/commit/2f96e523f77c69fa99138e49d923c44c9bdfae51

I realized that we could use sourceIndex in a very limited case which should help the performance for .add(), overall. Note that we can't use sourceIndex exclusively (as was being asked for) but only as a side to the actual sorting algorithm.

comment:37 Changed 4 years ago by john

Performance test with data here:  http://jsperf.com/jquery-add-performance-2

Right now it's only about 6-10% slower in 1.6.1 vs. 1.4.2 - but substantially faster than 1.4.3 - 1.6 (around 12-20x).

comment:38 Changed 4 years ago by cmcnulty

John, you can also close http://bugs.jquery.com/ticket/8778 as a dupe

Version 0, edited 4 years ago by cmcnulty (next)

comment:39 Changed 4 years ago by john

#8778 is a duplicate of this ticket.

comment:40 Changed 3 years ago by roviury@…

 http://closure-compiler.appspot.com/code/jsc6b0c3922d3ded1527f0deda155d1abcd/default.js

This is a minified, modified function jQuery.fn.add (all jQuery versions, all browsers, 476 bytes)

it can speed up at least 2.0x

PS. two array of elements must in DOM order before

comment:41 in reply to: ↑ 29 Changed 17 months ago by cseppy@…

Replying to brewdente@…: We have tons of users stuck in IE 7 and 8 and this is a great workaround, thanks!

comment:42 in reply to: ↑ 36 Changed 16 months ago by Jamie

Replying to john:

Fixed.  https://github.com/jquery/sizzle/commit/2f96e523f77c69fa99138e49d923c44c9bdfae51

John, I have jquery-1.9.1, and I notice that this change isn't in there. When will it be released to the mainline?

Note: See TracTickets for help on using tickets.