Ticket #4759 (closed feature: fixed)

Opened 5 years ago

Last modified 2 years ago

a major optimization is possible in sortable()

Reported by: mesoconcepts Owned by:
Priority: major Milestone: 1.8.18
Component: ui.sortable Version: 1.7.2
Keywords: Cc:
Blocking: Blocked by:

Description

found this while writing a patch with WP bug 10021:

 http://core.trac.wordpress.org/ticket/10021

Basically, the key optimization was that:

$('foo').sortable(
  connectWith: 'bar'
);

is tremendously slower than:

$('foo').sortable().sortable('option', 'connectWith', 'bar');

and by this, I'm really meaning slower by inordinate amounts of time. Like... by a factor of 5 in my case, with 15 placeholders.

I haven't looked into your code's specific, but I would assume you're running an O(n!) algorithm in there instead of an O(n).

The connectWidth option, when applied to an array of objects, should be ignored, and applied at the very end, once all of the array elements are instantiated. Else, it recursively jQueries the document for the needed elements, and does the same job n! times on the first element.

I'm pretty certain you've similar optimizations to do over in draggable and droppable, and possibly other areas.

Change History

comment:1 Changed 5 years ago by mesoconcepts

Some further information on this one. I've spotted a somewhat related issue... Possibly the same, too. In Opera 9.6, the init code goes:

		var o = this.options;
		this.containerCache = {};
		this.element.addClass("ui-sortable");

		//Get the items
		this.refresh();

		//Let's determine if the items are floating
		this.floating = this.items.length ? (/left|right/).test(this.items[0].item.css('float')) : false;

		//Let's determine the parent's offset
		this.offset = this.element.offset();

		//Initialize mouse events for interaction
		this._mouseInit();

each of:

  • this.refresh();
  • this.floating = ...
  • this.offset = this.element.offset();
  • this._mouseInit();

Makes use of the rendering engine in a way or another, and ends up causing several seconds of lag on the huge page I'm doing tests on.

See also this and its follow-up comment:

 http://core.trac.wordpress.org/ticket/10021#comment:20

And, along the same lines, I've noted that adding an arbitrary ID to the page using js before the ready event is fired, or at the beginning of the ready event, will cause massive lags in Opera. The bug has been reported upstream, but it's likely something that should be worked around in UI until it's fixed.

comment:2 Changed 3 years ago by scott.gonzalez

  • Priority changed from critical to major
  • Type changed from bug to feature

comment:3 Changed 2 years ago by jdmarshall

What seems to be happening is that if connectWith is turned on, each container tries to cache the position and dimensions for for all the items in all of the connected sortables, instead of just caching its own.

Given that connectWith is probably most often used when connecting containers to each other (ie containerA and containerB are mutually sortable), it would be better if each container only cached its own stuff. There is also a bit of cache invalidation going on here that I haven't fully comprehended, but that may be a source of the O(n!) behavior as well.

The problem may also be due to the fact that sortable() is called on the elements directly. Each container is not aware that it has peers that are being configured at the same time. Maybe a fixed version of this would be to implement jQuery.sortable(containers, options), where it can then be smart enough to avoid recalculating cache data.

comment:4 Changed 2 years ago by jdmarshall

Note that #5886 was closed as a duplicate of this issue, but has some benchmarks.

comment:5 Changed 2 years ago by jdmarshall

I am seeing about a 15-20% improvement to sortable() with jQuery 1.7 (vs 1.5), however this change appears to be due entirely to optimization in jQuery internals. A leaf method in the CSS code runs faster, giving a linear speedup.

sortable() still represents more than 60% of the page load time in our app, so it'd be good to get this fixed sooner rather than later.

Last edited 2 years ago by jdmarshall (previous) (diff)

comment:6 Changed 2 years ago by SpoonNZ

I've made a proposed fix at  https://github.com/SpoonNZ/jquery-ui/commit/2b2a487c20c91112cd66b3eb8bd08b6de2923a08

This fixes the issue, though of course may not be the best way!

Basically all it does is stop the if(connectWith) conditional within the _refreshItems() function from running until after the sortable is completely initiated.

Last edited 2 years ago by SpoonNZ (previous) (diff)

comment:7 Changed 2 years ago by SpoonNZ

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

Sortable: Added a variable to track if initialization is complete. Fixes #4759 - a major optimization is possible in sortable().

Changeset: ba6916f22ac3fac993975abc0f86d6cb0bf9c08d

comment:8 Changed 2 years ago by SpoonNZ

Sortable: Added a variable to track if initialization is complete. Fixes #4759 - a major optimization is possible in sortable(). (cherry picked from commit ba6916f22ac3fac993975abc0f86d6cb0bf9c08d)

Changeset: b00faa95d0d372f345e24f9abe9d16a2b67ca258

comment:9 Changed 2 years ago by scott.gonzalez

  • Milestone changed from 1.next to 1.8.18
Note: See TracTickets for help on using tickets.