From 47407bf72860258dbc29a3ad5d961fc4c5f6378b Mon Sep 17 00:00:00 2001 From: Vaadin Bot Date: Tue, 26 Oct 2021 14:41:27 +0300 Subject: [PATCH] fix: do not create constant pool key eagerly (#12124) (#12166) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../vaadin/flow/internal/ConstantPoolKey.java | 17 +++--- .../flow/component/ComponentEventBusTest.java | 23 ++++++++ .../flow/internal/ConstantPoolTest.java | 9 ++++ .../ui/ConstantPoolPerformanceView.java | 52 +++++++++++++++++++ pom.xml | 7 ++- 5 files changed, 98 insertions(+), 10 deletions(-) create mode 100644 flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/ConstantPoolPerformanceView.java 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 index 2a05771a714..b51d5d592c7 100644 --- 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 * null */ 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 index 57449dd8b09..df5895634b2 100644 --- 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 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 index 4a4c5fcd130..abb3518ecf8 100644 --- 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/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/ConstantPoolPerformanceView.java b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/ConstantPoolPerformanceView.java new file mode 100644 index 00000000000..00102a46bea --- /dev/null +++ b/flow-tests/test-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 index c36699624a2..bcc15449ed7 100644 --- a/pom.xml +++ b/pom.xml @@ -277,6 +277,11 @@ mockito-core 3.10.0 + + org.mockito + mockito-inline + 3.12.4 + org.hamcrest hamcrest-all @@ -303,7 +308,7 @@ org.mockito - mockito-core + mockito-inline test