-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Performance issue in sortable widget with long lists #2062
Comments
Update: $.ui.sortable.prototype._setHandleClassName = function() {
this._removeClass( this.element.find( ".ui-sortable-handle" ), "ui-sortable-handle" );
$.each( this.items, function() {
(this.instance.options.handle
? this.item.find( this.instance.options.handle )
: this.item
).addClass('ui-sortable-handle');
});
}; If set before the initialization of the sortable. |
Thanks for the report. Since the issue is already in 1.12, given limited team resources it's not likely to be fixed by the UI team; see the project status at https://blog.jqueryui.com/2021/10/jquery-maintainers-update-and-transition-jquery-ui-as-part-of-overall-modernization-efforts/. I'd review a PR if you'd like to submit one, though. |
This addresses jquery#2062 and is relevant for large lists with more than 1000 items. The solution to the issue was suggested at https://stackoverflow.com/a/46925710/1119601 (credits to @Ernst). At the end this commit restores the function's behaviour of previous 1.11.x version.
@mgol I've submitted a PR ;-) |
I can confirm for the last 5 years in Primefaces we have used this same patch. Here is the original report ticket: primefaces/primefaces#3675 // GitHub PrimeFaces #3675 performance
$.widget( "ui.sortable", $.ui.sortable, {
_setHandleClassName: function() {
this._removeClass( this.element.find( ".ui-sortable-handle" ), "ui-sortable-handle" );
$.each( this.items, function() {
(this.instance.options.handle
? this.item.find( this.instance.options.handle )
: this.item
).addClass('ui-sortable-handle');
} );
}
}); |
…e leak from version 1.12 - see more jquery/jquery-ui#2062 - there is still not accepted PR jquery/jquery-ui#2063
…e leak from version 1.12 - see more jquery/jquery-ui#2062 - there is still not accepted PR jquery/jquery-ui#2063
…e leak from version 1.12 - see more jquery/jquery-ui#2062 - there is still not accepted PR jquery/jquery-ui#2063
…e leak from version 1.12 - see more jquery/jquery-ui#2062 - there is still not accepted PR jquery/jquery-ui#2063
…e leak from version 1.12 - see more jquery/jquery-ui#2062 - there is still not accepted PR jquery/jquery-ui#2063
@mgol, @fnagel, the cause for the excessive time complexity is an O(n*n*log(n)) algorithm. AnalysisConsider this call stack: . _setHandleClassName (loops over all items) Given that the |
I don't see an easy fix for this. We'd probably have to stop keeping |
BTW, the Line 600 in a9e8520
is not the only issue. A lot of the slowdown also comes from recomputing classElementsLookup modifications, in particular:Line 528 in a9e8520
classElementsLookup is an object with values being jQuery objects, so adding elements by one to it has the same computational complexity issues.
Ideally, I submitted a draft PR with these changes; it passes tests and the initialization time reported for the test case reported here is at ~50 ms on my machine. I am not sure it won't introduce other issues, though, plus it changes the shape of some objects, so landing it would be risky. Feel free to have a look at the code & test these changes locally, though. PR: #2313 |
There is a huge performance issue in the
sortable
widget which started in version (1.12.1
) and is still existing in current version1.13.1
, when used with large lists.There's already a ticket for this issue in the old tracker: https://bugs.jqueryui.com/ticket/15097 and a corresponding stackoverflow question.
Today I have created some new fiddles to show the issue. Please compare the console output in the fiddle:
There are some solutions suggested in the stackoverflow answeres and in the old ticket.
It would be great if you could fix the issue. Otherwise we are stuck on version 1.11.4 :-(
The text was updated successfully, but these errors were encountered: