fix: do not create constant pool key eagerly (#12124) (#12166)

Stops creating a constant pool key eagerly as it was being created 11 times for a simple click listener after each event data expression was evaluated. The change improves the server side roundtrip processing time for 2000 buttons with click listeners from roughly 210ms to 130ms (-40%) by not eagerly creating the id using MessageDigest after evaluating each event data expression. Difference to buttons without any click listeners is still about x2 (65ms->130ms). Next step is to also make it not create the MessageDigest over and over again. Part of #7826 Co-authored-by: Pekka Hyvönen <[email protected]>
vaadin · Oct 26, 2021 · 47407bf · 47407bf
1 parent a549e84
commit 47407bf
Show file tree

Hide file tree

Showing 5 changed files with 98 additions and 10 deletions.
diff --git a/flow-server/src/main/java/com/vaadin/flow/internal/ConstantPoolKey.java b/flow-server/src/main/java/com/vaadin/flow/internal/ConstantPoolKey.java
@@ -40,7 +40,7 @@
  */
 public class ConstantPoolKey implements Serializable {
     private final JsonValue json;
-    private final String id;
+    private String id;
 
     /**
      * Creates a new constant pool key for the given JSON value. The value
@@ -53,8 +53,6 @@ public class ConstantPoolKey implements Serializable {
     public ConstantPoolKey(JsonValue json) {
         assert json != null;
         this.json = json;
-
-        id = calculateHash(json);
     }
 
     /**
@@ -63,22 +61,23 @@ public ConstantPoolKey(JsonValue json) {
      * @return the id used to identify this value
      */
     public String getId() {
+        if (id == null) {
+            id = calculateHash(json);
+        }
         return id;
     }
 
     /**
-     * Exports the this key into a JSON object to send to the client. This
-     * method should be called only by the {@link ConstantPool} instance that
-     * manages this value. It may be called multiple times.
+     * Exports this key into a JSON object to send to the client. This method
+     * should be called only by the {@link ConstantPool} instance that manages
+     * this value. It may be called multiple times.
      *
      * @param clientConstantPoolUpdate
      *            the constant pool update that is to be sent to the client, not
      *            <code>null</code>
      */
     public void export(JsonObject clientConstantPoolUpdate) {
-        assert id.equals(calculateHash(json)) : "Json value has been changed";
-
-        clientConstantPoolUpdate.put(id, json);
+        clientConstantPoolUpdate.put(getId(), json);
     }
 
     /**

diff --git a/flow-server/src/test/java/com/vaadin/flow/component/ComponentEventBusTest.java b/flow-server/src/test/java/com/vaadin/flow/component/ComponentEventBusTest.java
@@ -21,10 +21,13 @@
 
 import org.junit.Assert;
 import org.junit.Test;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
 
 import com.vaadin.flow.component.ComponentTest.TestComponent;
 import com.vaadin.flow.dom.Element;
 import com.vaadin.flow.internal.JsonCodec;
+import com.vaadin.flow.internal.MessageDigestUtil;
 import com.vaadin.flow.internal.nodefeature.ElementListenerMap;
 import com.vaadin.flow.shared.Registration;
 
@@ -86,6 +89,11 @@ public boolean equals(Object obj) {
         }
     }
 
+    @Tag("button")
+    private class TestButton extends Component implements ClickNotifier {
+
+    }
+
     private void fireDomEvent(Component component, String domEvent,
             JsonObject eventData) {
         Element e = component.getElement();
@@ -482,4 +490,19 @@ public void eventUnregisterListener_outsideListenerTwiceThrows() {
         c.fireEvent(new ServerEvent(c, new BigDecimal(0)));
         storedEvent.get().unregisterListener();
     }
+
+    @Test // #7826
+    public void addListener_eventDataExpressionsPresent_constantPoolKeyNotCreatedAfterEachExpression() {
+        final TestButton button = new TestButton();
+        try (MockedStatic<MessageDigestUtil> util = Mockito
+                .mockStatic(MessageDigestUtil.class)) {
+            util.when(() -> MessageDigestUtil.sha256(Mockito.anyString()))
+                    .thenReturn(
+                            new byte[] { 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, });
+            button.addClickListener(event -> {
+            });
+            util.verifyNoInteractions();
+        }
+
+    }
 }
diff --git a/flow-server/src/test/java/com/vaadin/flow/internal/ConstantPoolTest.java b/flow-server/src/test/java/com/vaadin/flow/internal/ConstantPoolTest.java
@@ -74,4 +74,13 @@ public void differentValue_differentId() {
         Assert.assertNotEquals(constantId, otherId);
         Assert.assertTrue(constantPool.hasNewConstants());
     }
+
+    @Test
+    public void constantPoolKey_exportedDirectly_idCreated() {
+        final ConstantPoolKey constantPoolKey = new ConstantPoolKey(
+                Json.createObject());
+        final JsonObject message = Json.createObject();
+        constantPoolKey.export(message);
+        Assert.assertTrue(message.hasKey(constantPoolKey.getId()));
+    }
 }
diff --git a/...est-root-context/src/main/java/com/vaadin/flow/uitest/ui/ConstantPoolPerformanceView.java b/...est-root-context/src/main/java/com/vaadin/flow/uitest/ui/ConstantPoolPerformanceView.java
@@ -0,0 +1,52 @@
+/*
+ * Copyright 2000-2021 Vaadin Ltd.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package com.vaadin.flow.uitest.ui;
+
+import com.vaadin.flow.component.html.Div;
+import com.vaadin.flow.component.html.NativeButton;
+import com.vaadin.flow.router.Route;
+
+/* Manual test case for #7826 */
+@Route("performance/constant-pool")
+public class ConstantPoolPerformanceView extends AbstractDivView {
+
+    Div container = new Div();
+    Div notification = new Div();
+
+    public ConstantPoolPerformanceView() {
+        NativeButton btn = new NativeButton(
+                "add 2k divs without click listener");
+        btn.addClickListener(e -> {
+            container.removeAll();
+            for (int i = 0; i < 2000; i++) {
+                container.add(new NativeButton("No click listener "));
+            }
+        });
+        NativeButton btn2 = new NativeButton("add 2k divs with click listener");
+        btn2.addClickListener(e -> {
+            container.removeAll();
+            for (int i = 0; i < 2000; i++) {
+                container.add(new NativeButton("With click listener " + i,
+                        e2 -> notification.setText("clicked")));
+            }
+        });
+        final NativeButton clearButtons = new NativeButton("clear buttons",
+                e -> container.removeAll());
+        add(btn, btn2, clearButtons, notification, container);
+    }
+
+}
diff --git a/pom.xml b/pom.xml
@@ -277,6 +277,11 @@
                 <artifactId>mockito-core</artifactId>
                 <version>3.10.0</version>
             </dependency>
+            <dependency>
+                <groupId>org.mockito</groupId>
+                <artifactId>mockito-inline</artifactId>
+                <version>3.12.4</version>
+            </dependency>
             <dependency>
                 <groupId>org.hamcrest</groupId>
                 <artifactId>hamcrest-all</artifactId>
@@ -303,7 +308,7 @@
         </dependency>
         <dependency>
             <groupId>org.mockito</groupId>
-            <artifactId>mockito-core</artifactId>
+            <artifactId>mockito-inline</artifactId>
             <scope>test</scope>
         </dependency>
         <dependency>