[pmg-devel] [PATCH] override deselection in CheckboxModel to improve performance

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

After the UI hang for tens of seconds for a few thousand elements got
deselected I investigated with Firefox developer tools performance
analysis, where the waterfall view showed that most of the time was
spent in array splicing.

Previously, all removed elements got removed on by one from the
`selected` Ext.util.Collection - which is basically an helper class
around arrays and objects, most of it may have become obsolete with
modern browsers. The single remove resulted into further splicing of
the array, and all in all it resulted in a dramatically increased
complexity, ~ O(n^3).

The "remove one by one" logic comes highly probably from the fact
that users can register a `beforedeselection` listener which can
block a removal of a specific record. But, that's not used by us and
not really something one would often need in practice, but still its
a documented feature of ExtJS grids we want to keep; so go for an
alternative.
So, override `doDeselect` and change the old removal logic to one
that first record those entries which got blocked from removal and
remove them in one go from the "to-be-removed" collection.

Before/After this patch on my FF 86.0.1 with my i9-9900K and
deselecting ~10k records went from ~40s to about 33 ms total, so for
that case we went 1000x faster.

The remaining time is now mostly spend in the event fire/handling
logic, but even with 50k records we spent <<500ms in total, so not
too bad and thus kept as is for now.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---

Note, I had some more optimizations for the deselectAll case specifically, but
I omitted that as that one provided small extra benefits, so less
changes/overrides where favored.

 src/Toolkit.js | 66 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/src/Toolkit.js b/src/Toolkit.js
index 6dcccd3..79bc5ae 100644
--- a/src/Toolkit.js
+++ b/src/Toolkit.js
@@ -492,11 +492,75 @@ Ext.define('Proxmox.validIdReOverride', {
     validIdRe: /^[a-z_][a-z0-9\-_@]*$/i,
 });
 
-// use whole checkbox cell to multiselect, not only the checkbox
 Ext.define('Proxmox.selection.CheckboxModel', {
     override: 'Ext.selection.CheckboxModel',
 
+    // [P] use whole checkbox cell to multiselect, not only the checkbox
     checkSelector: '.x-grid-cell-row-checker',
+
+    // [ P: optimized to remove all records at once as single remove is O(n^3) slow ]
+    // records can be an index, a record or an array of records
+    doDeselect: function(records, suppressEvent) {
+        var me = this,
+            selected = me.selected,
+            i = 0,
+            len, record,
+            commit;
+        if (me.locked || !me.store) {
+            return false;
+        }
+        if (typeof records === "number") {
+            // No matching record, jump out
+            record = me.store.getAt(records);
+            if (!record) {
+                return false;
+            }
+            records = [
+                record,
+            ];
+        } else if (!Ext.isArray(records)) {
+            records = [
+                records,
+            ];
+        }
+	// [P] a beforedeselection, triggered by me.onSelectChange below, can block removal by
+	// returning false, thus the original implementation removed only here in the commit fn,
+	// which has an abysmal performance O(n^3). As blocking removal is not the norm, go do the
+	// reverse, record blocked records and remove them from the to-be-removed array before
+	// applying it. A FF86 i9-9900K on 10k records goes from >40s to ~33ms for >90% deselection
+	let committed = false;
+	commit = function() {
+	    committed = true;
+	    if (record === me.selectionStart) {
+		me.selectionStart = null;
+	    }
+	};
+	let removalBlocked = [];
+        len = records.length;
+        me.suspendChanges();
+        for (; i < len; i++) {
+            record = records[i];
+            if (me.isSelected(record)) {
+		committed = false;
+                me.onSelectChange(record, false, suppressEvent, commit);
+		if (!committed) {
+		    removalBlocked.push(record);
+		}
+                if (me.destroyed) {
+                    return false;
+                }
+            }
+        }
+	if (removalBlocked.length > 0) {
+	    records.remove(removalBlocked);
+	}
+	selected.remove(records); // [P] FAST(er)
+	me.lastSelected = selected.last();
+        me.resumeChanges();
+        // fire selchange if there was a change and there is no suppressEvent flag
+	me.maybeFireSelectionChange(records.length > 0 && !suppressEvent);
+	return records.length > 0;
+    },
 });
 
 // force alert boxes to be rendered with an Error Icon
-- 
2.20.1