From: Leif Åstrand Date: Sat, 20 Sep 2014 08:52:55 +0000 (+0300) Subject: Don't iterate all connectors for unregistering (#14714) X-Git-Tag: 7.4.0.beta1~85 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=828f1f0f20a296d97c693dd2acd004d4c90ad5cd;p=vaadin-framework.git Don't iterate all connectors for unregistering (#14714) This reduces the time spent in unregisterRemovedConnectors when updating the text of one Label with about 380 connectors registered from 20 to 2 ms as reported by the Vaadin profiler. Performance when removing lots of connectors remains at about 10 ms for removing 360 connectors. Profiled in Firefox 32 on OS X. Change-Id: I203fd8790f8ccc7c098ee91c44831a5ac6c4a82b --- diff --git a/client/src/com/vaadin/client/ApplicationConnection.java b/client/src/com/vaadin/client/ApplicationConnection.java index cba9639067..d7c1c54a2d 100644 --- a/client/src/com/vaadin/client/ApplicationConnection.java +++ b/client/src/com/vaadin/client/ApplicationConnection.java @@ -135,6 +135,11 @@ public class ApplicationConnection implements HasHandlers { * Needed to know where captions might need to get updated */ private FastStringSet parentChangedIds = FastStringSet.create(); + + /** + * Connectors for which the parent has been set to null + */ + private FastStringSet detachedConnectorIds = FastStringSet.create(); } public static final String MODIFIED_CLASSNAME = "v-modified"; @@ -1624,7 +1629,7 @@ public class ApplicationConnection implements HasHandlers { json.getValueMap("dd")); } - unregisterRemovedConnectors(); + unregisterRemovedConnectors(connectorHierarchyUpdateResult.detachedConnectorIds); VConsole.log("handleUIDLMessage: " + (Duration.currentTimeMillis() - processUidlStart) @@ -1722,7 +1727,7 @@ public class ApplicationConnection implements HasHandlers { sendHierarchyChangeEvents(connectorHierarchyUpdateResult.events); // Unregister all the old connectors that have now been removed - unregisterRemovedConnectors(); + unregisterRemovedConnectors(connectorHierarchyUpdateResult.detachedConnectorIds); getLayoutManager().cleanMeasuredSizes(); } @@ -1877,47 +1882,63 @@ public class ApplicationConnection implements HasHandlers { Profiler.leave("sendStateChangeEvents"); } - private void unregisterRemovedConnectors() { - Profiler.enter("unregisterRemovedConnectors"); + private void verifyConnectorHierarchy() { + Profiler.enter("verifyConnectorHierarchy - this is only performed in debug mode"); - int unregistered = 0; JsArrayObject currentConnectors = connectorMap .getConnectorsAsJsArray(); int size = currentConnectors.size(); for (int i = 0; i < size; i++) { ServerConnector c = currentConnectors.get(i); if (c.getParent() != null) { - // only do this check if debug mode is active - if (ApplicationConfiguration.isDebugMode()) { - Profiler.enter("unregisterRemovedConnectors check parent - this is only performed in debug mode"); - // this is slow for large layouts, 25-30% of total - // time for some operations even on modern browsers - if (!c.getParent().getChildren().contains(c)) { - VConsole.error("ERROR: Connector is connected to a parent but the parent does not contain the connector"); - } - Profiler.leave("unregisterRemovedConnectors check parent - this is only performed in debug mode"); + if (!c.getParent().getChildren().contains(c)) { + VConsole.error("ERROR: Connector " + + c.getConnectorId() + + " is connected to a parent but the parent (" + + c.getParent().getConnectorId() + + ") does not contain the connector"); } } else if (c == getUIConnector()) { - // UIConnector for this connection, leave as-is + // UIConnector for this connection, ignore } else if (c instanceof WindowConnector && getUIConnector().hasSubWindow( (WindowConnector) c)) { - // Sub window attached to this UIConnector, leave - // as-is + // Sub window attached to this UIConnector, ignore } else { // The connector has been detached from the - // hierarchy, unregister it and any possible - // children. The UIConnector should never be - // unregistered even though it has no parent. - Profiler.enter("unregisterRemovedConnectors unregisterConnector"); - connectorMap.unregisterConnector(c); - Profiler.leave("unregisterRemovedConnectors unregisterConnector"); - unregistered++; + // hierarchy but was not unregistered. + VConsole.error("ERROR: Connector " + + c.getConnectorId() + + " is not attached to a parent but has not been unregistered"); } } - VConsole.log("* Unregistered " + unregistered + " connectors"); + Profiler.leave("verifyConnectorHierarchy - this is only performed in debug mode"); + } + + private void unregisterRemovedConnectors( + FastStringSet detachedConnectors) { + Profiler.enter("unregisterRemovedConnectors"); + + JsArrayString detachedArray = detachedConnectors.dump(); + for (int i = 0; i < detachedArray.length(); i++) { + ServerConnector connector = connectorMap + .getConnector(detachedArray.get(i)); + + Profiler.enter("unregisterRemovedConnectors unregisterConnector"); + connectorMap.unregisterConnector(connector); + Profiler.leave("unregisterRemovedConnectors unregisterConnector"); + } + + if (ApplicationConfiguration.isDebugMode()) { + // Do some extra checking if we're in debug mode (i.e. debug + // window is open) + verifyConnectorHierarchy(); + } + + VConsole.log("* Unregistered " + detachedArray.length() + + " connectors"); Profiler.leave("unregisterRemovedConnectors"); } @@ -2327,7 +2348,8 @@ public class ApplicationConnection implements HasHandlers { for (int i = 0; i < maybeDetachedArray.length(); i++) { ServerConnector removed = connectorMap .getConnector(maybeDetachedArray.get(i)); - recursivelyDetach(removed, result.events); + recursivelyDetach(removed, result.events, + result.detachedConnectorIds); } Profiler.leave("updateConnectorHierarchy detach removed connectors"); @@ -2339,7 +2361,9 @@ public class ApplicationConnection implements HasHandlers { } private void recursivelyDetach(ServerConnector connector, - JsArrayObject events) { + JsArrayObject events, + FastStringSet detachedConnectors) { + detachedConnectors.add(connector.getConnectorId()); /* * Reset state in an attempt to keep it consistent with the @@ -2400,7 +2424,7 @@ public class ApplicationConnection implements HasHandlers { if (child.getParent() != connector) { continue; } - recursivelyDetach(child, events); + recursivelyDetach(child, events, detachedConnectors); } Profiler.leave("ApplicationConnection recursivelyDetach perform detach"); diff --git a/uitest/src/com/vaadin/tests/performance/BasicPerformanceTest.java b/uitest/src/com/vaadin/tests/performance/BasicPerformanceTest.java index 04fe18fc08..a97f2611d1 100644 --- a/uitest/src/com/vaadin/tests/performance/BasicPerformanceTest.java +++ b/uitest/src/com/vaadin/tests/performance/BasicPerformanceTest.java @@ -4,6 +4,7 @@ import java.util.Iterator; import com.vaadin.server.VaadinRequest; import com.vaadin.tests.util.TestUtils; +import com.vaadin.ui.AbstractOrderedLayout; import com.vaadin.ui.Button; import com.vaadin.ui.Button.ClickEvent; import com.vaadin.ui.Component; @@ -18,6 +19,8 @@ import com.vaadin.ui.themes.Reindeer; public class BasicPerformanceTest extends UI { + private int updateOneCount = 0; + private final VerticalLayout contentLayout = new VerticalLayout(); private int clientLimit; @@ -64,7 +67,7 @@ public class BasicPerformanceTest extends UI { TestUtils.installPerformanceReporting(performanceReportArea); VerticalLayout leftBar = new VerticalLayout(); - leftBar.setSizeUndefined(); + leftBar.setWidth("250px"); leftBar.addComponent(new Label("This is the left bar")); leftBar.addComponent(performanceReportArea); leftBar.addComponent(reportPerformanceButton); @@ -126,6 +129,26 @@ public class BasicPerformanceTest extends UI { } })); + leftBar.addComponent(new Button("Update one label", + new Button.ClickListener() { + @Override + public void buttonClick(ClickEvent event) { + Component child = contentLayout.getComponent(0); + if (child instanceof Panel) { + Panel panel = (Panel) child; + child = panel.getContent(); + } + + AbstractOrderedLayout layout = (AbstractOrderedLayout) ((AbstractOrderedLayout) child) + .getComponent(0); + Label label = (Label) layout.getComponent(0); + + label.setValue("New value " + updateOneCount++); + + updatePerformanceReporting("Update one", 10, 10); + } + })); + leftBar.addComponent(new Button("Clear content", new Button.ClickListener() { @Override