On Sat, 2 Oct 2021 13:35:51 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> The behavior is the same between openjdk 16.0.2 and 17.0.0.1. The test to >> recreate the problem adds many nodes (Rectangle) to a parent Group. The >> process causes each node to add two invalidation listeners (disabledProperty >> and treeVisibleProperty) to the parent node >> [here](https://github.com/openjdk/jfx/blob/64aa92631fa5aad9293553e8dd174eab647de2f3/modules/javafx.graphics/src/main/java/javafx/scene/Node.java#L965) >> using ExpressionHelper.Generic.addListener(). When the children are cleared >> from the group the inefficient linear search through the listener array and >> remove occurs, two linear searches per child. >> >> Here is the minimal application I am using to recreate the issue. >> >> >> public class ClearGroupPerformance extends Application { >> >> private static final int GAP = 10; >> private static final int SIZE = 40; >> private Group groupBase; >> >> public static void main(String[] args) { >> >> launch(args); >> } >> >> @Override >> public void start(Stage stage) { >> >> int width = 10000; >> int height = 10000; >> >> ScrollPane scrollPane = new ScrollPane(); >> StackPane root = createGroupRoot(width, height); >> scrollPane.setContent(root); >> >> Button clearGroupButton = new Button("Clear Group"); >> clearGroupButton.onActionProperty().set(e -> { >> >> long startTime = System.nanoTime(); >> ObservableList<Node> children = groupBase.getChildren(); >> int totalChildren = children.size(); >> children.clear(); >> System.out.printf("Clearing %d nodes took %.2fms%n", >> totalChildren, (System.nanoTime()-startTime)/1_000_000.0); >> >> }); >> >> BorderPane borderPane = new BorderPane(); >> borderPane.setTop(clearGroupButton); >> borderPane.setCenter(scrollPane); >> >> Scene scene = new Scene(borderPane, 100, 100); >> stage.setScene(scene); >> stage.show(); >> >> } >> >> private StackPane createGroupRoot(int width, int height) { >> >> groupBase = new Group(); >> groupBase.getChildren().add(new Rectangle(width, height, new >> Color(0.5, 0.5, 0.5, 1.0))); >> >> for (int posX = GAP; posX + SIZE + GAP < width; posX += SIZE + GAP) { >> for (int posY = GAP; posY + SIZE + GAP < height; posY += SIZE + >> GAP) { >> Rectangle rectangle = new Rectangle(posX, posY, SIZE, SIZE); >> rectangle.setFill(Color.GREEN); >> >> groupBase.getChildren().add(rectangle); >> } >> } >> >> return new StackPane(groupBase); >> } >> >> } > >> Here is the minimal application I am using to recreate the issue. > > I'll attach the test case you provided to the bug report. > >> The question now is should the work done on this PR be abandoned. It >> addresses a current performance limitation taking complexity from O(n log n) >> to O(1). > > That is a fair question, but it begs two additional questions. Are there > enough real world examples where performance is being hurt by using an > ArrayList to justify the effort and risk (more on that below)? If so, are > there other solutions that would reduce the number of listeners needed? The > original problems that motivated this fix were largely addressed by > [JDK-8252935](https://bugs.openjdk.java.net/browse/JDK-8252935) / PR #185 and > [JDK-8252811](https://bugs.openjdk.java.net/browse/JDK-8252811) / PR #298. > >> It appears to be well isolated with low risk to backwards compatibility. The >> reason work was stopped was a concern that the tests should go first in a >> separate PR. > > I disagree that this is a low risk change. This proposed fix touches a > fundamental area used by all bindings, and does so in a way that will > increase memory usage -- and might negatively impact performance -- in the > common case of a small number of listeners. By unconditionally switching from > an `ArrayList` to a `HashSet`, the memory required is significantly > increased, which will be most noticeable for the common case of only a few > listeners. An adaptive solution, which starts out using an `ArrayList` and > dynamically switches to a `LinkedHashSet` (which preserves notification > order...I know it isn't specified, but changing that is likely to break some > applications) when crossing some threshold of the number of listeners, could > mitigate this concern at the cost of added complexity. > > If you still want to pursue this, you will need to do a fair amount of work > to provide the additional tests that will be needed, to motivate the need for > this enhancement (given that the original reasons are mostly addressed), and > to resolve the issues that were raised in this PR. The mechanics of doing > this are pretty easy. First, read the [CONTRIBUTING > guideline](https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md), > specifically the part about signing the OCA, and create a new PR starting > from this one. You would then add the author of this PR as a contributor. > > In the mean time, I'm going to move this PR to Draft (which I should have > done long ago), since it stalled and not being actively worked on or reviewed. @kevinrushforth makes an important point I was missing. Functionally this change is isolated but the memory overhead of a new data structure used by every node would risk performance degradation across many existing applications. I now see that the recommendation @hjohn to avoid adding unnecessary listeners by default should be prioritized over a faster listener lookup data structure. This will clarifies why work on this PR is currently back in draft status. ------------- PR: https://git.openjdk.java.net/jfx/pull/108
