#7341 closed bug (fixed)
Slow .add() in IE
Reported by: | Owned by: | Rick Waldron | |
---|---|---|---|
Priority: | high | Milestone: | 1.6.1 |
Component: | selector | Version: | 1.4.3 |
Keywords: | Cc: | Rick Waldron | |
Blocked by: | Blocking: |
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.
Change History (41)
comment:1 Changed 13 years ago by
comment:2 Changed 13 years ago by
Component: | unfiled → manipulation |
---|---|
Owner: | set to Rick Waldron |
Status: | new → assigned |
Alex, I'll take a look at this.
comment:3 Changed 13 years ago by
Cc: | Rick Waldron added |
---|
Did we manage to take another look at this? :)
comment:4 Changed 13 years ago by
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:6 Changed 12 years ago by
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 12 years ago by
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 12 years ago by
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 12 years ago by
Milestone: | 1.5 → 1.next |
---|---|
Priority: | undecided → high |
Resolution: | → wontfix |
Status: | assigned → closed |
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 12 years ago by
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 12 years ago by
sourceIndex is pretty thoroughly validated in my patch, but hey, it's your engine! : )
comment:13 Changed 12 years ago by
If something doesn't work properly then it is not correct, even if it is 'correct'.
comment:14 Changed 12 years ago by
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:16 Changed 12 years ago by
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 12 years ago by
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 12 years ago by
There are multiple bugs filed against jQuery UI which are caused by this.
comment:19 Changed 12 years ago by
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 12 years ago by
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 12 years ago by
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 12 years ago by
"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 12 years ago by
"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:25 Changed 12 years ago by
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:27 Changed 12 years ago by
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 12 years ago by
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 12 years ago by
If anyone wants a quick and easy way to get around this issue, just include the code pasted here:
after you include your jQuery file and you'll be back in business. Finally, you can update your jQuery!
comment:30 Changed 12 years ago by
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:32 Changed 12 years ago by
Has anybody tested 1.6 yet to see if this issue has been resolved?
comment:33 Changed 12 years ago by
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:
I'm off to jquery forums to try to see if anyone is interested in taking another look at this issue.
comment:34 Changed 12 years ago by
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 12 years ago by
Milestone: | 1.next → 1.6.1 |
---|---|
Resolution: | wontfix |
Status: | closed → reopened |
comment:36 follow-up: 42 Changed 12 years ago by
Component: | manipulation → selector |
---|---|
Resolution: | → fixed |
Status: | reopened → closed |
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 12 years ago by
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:40 Changed 12 years ago by
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 Changed 10 years ago by
Replying to brewdente@…: We have tons of users stuck in IE 7 and 8 and this is a great workaround, thanks!
comment:42 Changed 10 years ago by
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?
Can someone with a native IE verify?