Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#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 4 years ago by tj.vantoll

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.

comment:2 follow-up: Changed 4 years ago by scottgonzalez

  • Component changed from ui.core to ui.widget
  • Owner set to frederik.elvhage
  • Status changed from new to 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 in reply to: ↑ 2 Changed 4 years ago by frederik.elvhage

  • Status changed from pending to 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 4 years ago by scottgonzalez

  • Status changed from new to pending

We'll wait for the reduced test case to investigate. You may find something just by reducing your code.

comment:5 Changed 4 years ago by frederik.elvhage

  • Status changed from pending to new

I made a fiddle:

jsfiddle/slow

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.

Last edited 4 years ago by frederik.elvhage (previous) (diff)

comment:6 Changed 4 years ago by tj.vantoll

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 4 years ago by scottgonzalez

  • Resolution set to patcheswelcome
  • Status changed from new to 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 in reply to: ↑ description Changed 4 years ago by frederik.elvhage

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.

Last edited 4 years ago by frederik.elvhage (previous) (diff)

comment:9 Changed 3 years ago by scottgonzalez

  • Resolution patcheswelcome deleted
  • Status changed from closed to reopened

comment:10 Changed 3 years ago by Frederik Elvhage

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

Widget: Improve performance of cleanData method

Fixes #9546 Closes gh-1291

Changeset: f7429edfe96d322cdec850f7207efba8125767a6

comment:11 Changed 3 years ago by scottgonzalez

  • Milestone changed from none to 1.11.1

comment:12 Changed 3 years ago by scottgonzalez

  • Summary changed from cleanData-Proxy slows down ajax in IE8 to cleanData() proxy is slow
Note: See TracTickets for help on using tickets.