From 5ffb2897d7d0772094ae4dc91c938d3045e27795 Mon Sep 17 00:00:00 2001 From: Pepijn Van Eeckhoudt Date: Thu, 12 May 2022 08:22:43 +0200 Subject: [PATCH 1/3] fix: Avoid potential slow Set#removeAll call in HierarchicalCommunicationController#passivateInactiveKeys The performance of calling Set#removeAll(List) is dependent on the relative sizes of the Set and List parameter. Replace Set#removeAll with List#forEach(Set::remove) to avoid this. Fixes #13745 --- .../provider/hierarchy/HierarchicalCommunicationController.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicationController.java b/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicationController.java index 3efb9354128..cf94981d523 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicationController.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicationController.java @@ -287,7 +287,7 @@ private void passivateInactiveKeys(Set oldActive, } // Finally clear any passivated items that have now been confirmed - oldActive.removeAll(newActiveKeyOrder); + newActiveKeyOrder.forEach(oldActive::remove); if (!oldActive.isEmpty()) { passivatedByUpdate.put(Integer.valueOf(updateId), oldActive); } From 21d6303d67a7b786d6d497bd9456365f7db5c082 Mon Sep 17 00:00:00 2001 From: Soroosh Taefi Date: Thu, 19 May 2022 14:46:59 +0300 Subject: [PATCH 2/3] add comment about removeAll usage with big list as input --- .../hierarchy/HierarchicalCommunicationController.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicationController.java b/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicationController.java index cf94981d523..5bd1d67da5c 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicationController.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicationController.java @@ -287,6 +287,13 @@ private void passivateInactiveKeys(Set oldActive, } // Finally clear any passivated items that have now been confirmed + /* + * Due to the implementation of Set#removeAll, if the size of + * newActiveKeyOrder is bigger than oldActive's size, then + * calling oldActive.removeAll(newActiveKeyOrder) would end + * up calling contains for all of newActiveKeyOrder items + * which is a slow operation on lists. The following avoids that: + */ newActiveKeyOrder.forEach(oldActive::remove); if (!oldActive.isEmpty()) { passivatedByUpdate.put(Integer.valueOf(updateId), oldActive); From 5206c5f6945ecb604863efc0a91661a3ded1464f Mon Sep 17 00:00:00 2001 From: Soroosh Taefi Date: Thu, 19 May 2022 16:20:58 +0300 Subject: [PATCH 3/3] format the comments --- .../hierarchy/HierarchicalCommunicationController.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicationController.java b/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicationController.java index 5bd1d67da5c..5833669a482 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicationController.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicationController.java @@ -288,11 +288,11 @@ private void passivateInactiveKeys(Set oldActive, // Finally clear any passivated items that have now been confirmed /* - * Due to the implementation of Set#removeAll, if the size of - * newActiveKeyOrder is bigger than oldActive's size, then - * calling oldActive.removeAll(newActiveKeyOrder) would end - * up calling contains for all of newActiveKeyOrder items - * which is a slow operation on lists. The following avoids that: + * Due to the implementation of AbstractSet#removeAll, if the size + * of newActiveKeyOrder is bigger than oldActive's size, then + * calling oldActive.removeAll(newActiveKeyOrder) would end up + * calling contains method for all of newActiveKeyOrder items which + * is a slow operation on lists. The following avoids that: */ newActiveKeyOrder.forEach(oldActive::remove); if (!oldActive.isEmpty()) {