RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic [v3]

[John Nader]

On Sat, 2 Oct 2021 13:35:51 GMT, Kevin Rushforth <kcr at 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