#9546 closed bug (fixed)
cleanData() proxy is slow
Reported by: | frederik.elvhage | Owned by: | frederik.elvhage |
---|---|---|---|
Priority: | minor | Milestone: | 1.11.1 |
Component: | ui.widget | Version: | 1.10.1 |
Keywords: | Cc: | ||
Blocked by: | Blocking: |
Description
The (jQuery-UI) cleanData-Proxy i find in my jquery-ui-1.10.1.custom.js slows down ajax-calls (especially) in internet explorer 8.
I have found that removing this proxy will reduce ajax-processing time by more than 60% in IE8 and on large ajax-calls (~3.3s -> 1.1s).
It is obviously a counter-measure for a jquery-bug that has been fixed in 1.6.3.
$.cleanData = function( elems ) { for ( var i = 0, elem; (elem = elems[i]) != null; i++ ) { try { $( elem ).triggerHandler( "remove" ); // http://bugs.jquery.com/ticket/8235 } catch( e ) {} } _cleanData( elems ); };
I suggest removing this proxy and change legacy-support to 1.6.3+ (instead of 1.6+).
Change History (12)
comment:1 Changed 10 years ago by
comment:2 follow-up: 3 Changed 10 years ago by
Component: | ui.core → ui.widget |
---|---|
Owner: | set to frederik.elvhage |
Status: | new → pending |
This proxy has nothing to do with working around any bugs. This is the implementation of the remove event for auto-destruction of widgets. The comment with the bug URL is pointing to the reason that they try/catch exists. The same try/catch was added in jQuery core 1.6.3. I think you're confused about what's going on here.
It sounds like you're probably doing something specific beyond just making an ajax request. Can you please provide a reduced test case showing the performance problem you're having?
comment:3 Changed 10 years ago by
Status: | pending → new |
---|
Replying to scott.gonzalez:
This proxy has nothing to do with working around any bugs. This is the implementation of the remove event for auto-destruction of widgets. The comment with the bug URL is pointing to the reason that they try/catch exists. The same try/catch was added in jQuery core 1.6.3. I think you're confused about what's going on here.
It sounds like you're probably doing something specific beyond just making an ajax request. Can you please provide a reduced test case showing the performance problem you're having?
Ok, i (now) understand that the cleanData-Method has been proxied for other reasons than the bug mentioned. Looking at it now, i'm not sure what may be the reason for it. It can se a significant difference even when the dom is small (percentagevise).
How i got here? I used dynatrace Ajax-edition to analyze the requests and the cleanData-Method showed up very high in the hotspots view. I looked into the jQuery-UI code and found the proxy. Removed it and found client-processing was 60% faster.
I will check the code tomorrow to see if we have some expensive remove-handlers that make this problem specific to our code. It has to be the additional iteration itself or the handlers.
What we are doing? Wicket6-Applications (jQuery) with ajax. In some very large forms the dom can get huge (10 Tabs). But as said, i can see a great difference in small dom-trees as well.
Thank you for your time.
comment:4 Changed 10 years ago by
Status: | new → pending |
---|
We'll wait for the reduced test case to investigate. You may find something just by reducing your code.
comment:5 Changed 10 years ago by
Status: | pending → new |
---|
I made a fiddle:
It replaces some arbitrary content with and without cleanData and leaves me with the following results (see fiddle)
replacing content with jQuery + jQuery-UI: ~30ms
replacing content with jQuery + jQuery-UI but without cleanData: ~3ms
replacing content with jQuery only: ~9ms
I understand that jQuery-UI must cause some overhead, but this seems a bit too much to me. It makes a difference in all browsers. IE8 is just where we see performance-problems first.
comment:6 Changed 10 years ago by
Thanks for the test case. I'm seeing a difference of...
Chrome latest: ~5ms (no jQuery UI) vs. ~30ms (with jQuery UI) IE8: ~200ms (no jQuery UI) vs. ~600ms (with jQuery UI)
While that's non-trivial, there are 2500+ DOM nodes being removed. Off the top of my head I can't think of a good way to speed this up. We could try to be smarter about when we need to trigger the remove
, but of course there's a cost to doing the detection.
comment:7 Changed 10 years ago by
Resolution: | → patcheswelcome |
---|---|
Status: | new → closed |
We've looked at other ways to implement the remove
event, but we haven't found anything better. Feel free to send a pull request if you have an idea.
comment:8 Changed 10 years ago by
I believe that cleaning up dom on removal is a general concept (to avoid memory leaks aso) so there should be a possiblity to register with the framework for cleanup/remove. So jQuery should offer a callback or something.
In this particular case such a callback would save the extra iteration through all elements.
Doing some further testing i found that the iteration is cheap. The triggerHandler('remove') is whats expensive regardless of attached handlers. The solution would be to trigger the remove-handler soley on elements with widgets attached.
I.e. the widget could set a flag on initialization and check it in the cleanData-Method.
I have made another fiddle that allows testing of different cleanData-Implementations here:
jsfiddle/cleanData
My first naive implementation will test for the existance of events before it calls the remove-handler. This already saves at least 90% of the overhead. I will look further into this in time an provide a pull request.
comment:9 Changed 9 years ago by
Resolution: | patcheswelcome |
---|---|
Status: | closed → reopened |
comment:10 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Widget: Improve performance of cleanData method
Fixes #9546 Closes gh-1291
Changeset: f7429edfe96d322cdec850f7207efba8125767a6
comment:11 Changed 9 years ago by
Milestone: | none → 1.11.1 |
---|
comment:12 Changed 9 years ago by
Summary: | cleanData-Proxy slows down ajax in IE8 → cleanData() proxy is slow |
---|
scott.gonzalez: Have we discussed what versions of jQuery core we're going to support in 1.11? This seems like a decent reason to drop 1.6.