From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <t.lamprecht@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id EA04B69283
for <pmg-devel@lists.proxmox.com>; Mon, 22 Mar 2021 10:00:58 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id E81C6211EC
for <pmg-devel@lists.proxmox.com>; Mon, 22 Mar 2021 10:00:58 +0100 (CET)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
[212.186.127.180])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS id DAE36211DE
for <pmg-devel@lists.proxmox.com>; Mon, 22 Mar 2021 10:00:57 +0100 (CET)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 9B889428EF
for <pmg-devel@lists.proxmox.com>; Mon, 22 Mar 2021 10:00:57 +0100 (CET)
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: pmg-devel@lists.proxmox.com
Date: Mon, 22 Mar 2021 10:00:45 +0100
Message-Id: <20210322090046.26278-2-t.lamprecht@proxmox.com>
X-Mailer: git-send-email 2.20.1
In-Reply-To: <20210322090046.26278-1-t.lamprecht@proxmox.com>
References: <20210322090046.26278-1-t.lamprecht@proxmox.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.046 Adjusted score from AWL reputation of From: address
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/,
medium trust
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: [pmg-devel] [PATCH] override deselection in CheckboxModel to
improve performance
X-BeenThere: pmg-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Mail Gateway development discussion
<pmg-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pmg-devel>,
<mailto:pmg-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pmg-devel/>
List-Post: <mailto:pmg-devel@lists.proxmox.com>
List-Help: <mailto:pmg-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel>,
<mailto:pmg-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Mon, 22 Mar 2021 09:00:59 -0000
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