From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; 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 ; 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 ; 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 ; Mon, 22 Mar 2021 10:00:57 +0100 (CET) From: Thomas Lamprecht 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 --- 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