sortable: Use addClass in _setHandleClassName to improve performance #2063
Conversation
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.
|
|
| } ); | ||
| }, | ||
|
|
||
| _destroy: function() { | ||
| this.element.find( ".ui-sortable-handle" ).removeClass( "ui-sortable-handle" ); |
There was a problem hiding this comment.
Looking at it, I'm not sure about this, it seems to be diverging from how classes are generally managed in jQuery UI.
@fnagel what do you think?
There was a problem hiding this comment.
I commented in the ticket that we use this same current workaround because users reported the same thing 4 years ago. The performance difference with 1000 items is 2 seconds instead of 2+ minutes in the browser
There was a problem hiding this comment.
@melloware Thanks, that's an important data point. I'd still want to hear from @fnagel who was involved with UI when the team was larger, I may be lacking some context from those times.
There was a problem hiding this comment.
Sorry for the late response. Still in the process of moving my office and flat.
The thing is that _addClass does keep track of the elements and does some stuff when the element is removed (afair). It's part of the this.options.classes functionality and I think its related to #2037
One objection against this change is that we might reintroduce old issues we once fixed by adding the classes option. Seems we paid for bug fixes with performance penalty.
There was a problem hiding this comment.
Thanks for your reply. If you aren't comfortable with the patch, you can close it.
I have monkey patched that method in my project, so that I can still use v13 ;-)
|
I am also seeing this performance regression in jQuery UI 1.12.1, used as part of redmine 4 (backlogs plugin). Applying the patch (I didn't apply the change to the _destroy function) leads to a pretty spectacular speedup when rendering hundreds of items in a sorted list (it eliminates a ~30s wait), and brings back the performance to the level it was at with jQuery UI 1.11.x. It would be good to see this regression fixed. |
|
The risk here is that the proposed mechanism skips the logic used to handle classes which can bite in future changes to the library. I'm not that happy with hacks like that as they can bite in the future, edge cases can be discovered, etc. It'd be better to understand what exactly causes the slowdown and see if there's a fix in the logic possible that would speed things up. See #2037 for an example of such a PR. It solved issue #2014 without skipping any core logic. I don't have time to dive into this too much at the moment but if anyone finds a proper fix, I'd be happy to review such a PR, feel free to ping me in a PR then. |
…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
This addresses #2062 and is relevant for large lists with more than 1000 items.
The solution to the issue was suggested at stackoverflow.com (credits to @Ernst).
At the end this commit restores the function's behaviour of previous 1.11.x version.