Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not create constant pool key eagerly #12124

Merged
merged 2 commits into from
Oct 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -53,8 +53,6 @@ public class ConstantPoolKey implements Serializable {
public ConstantPoolKey(JsonValue json) {
assert json != null;
this.json = json;

id = calculateHash(json);
}

/**
Expand All @@ -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);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
}

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}
}
Original file line number Diff line number Diff line change
@@ -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 {
mshabarov marked this conversation as resolved.
Show resolved Hide resolved

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);
}

}
7 changes: 6 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,11 @@
<artifactId>mockito-core</artifactId>
<version>3.12.4</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>
Expand All @@ -275,7 +280,7 @@
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<artifactId>mockito-inline</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Expand Down