8252811: The list of cells in a VirtualFlow is cleared every time the number of items changes

[jperedadnr]

This PR removes an old fix (RT-13965/JDK-8113318), which was applied in 2011 to avoid a memory leak in VirtualFlow::sheetChildren, after new items were constantly added.

With the current VirtualFlow implementation, there are in place the necessary methods that take care of cleaning or adding new cells to the sheetChildren list, and such memory leak doesn't exist, the size remains constant when new items are added or removed (see included unity test).

The call to sheetchildren.clear(), on the contrary, has a big impact in performance and CPU usage, when new items are constantly added, as has been reported in JDK-8185886/#108.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8252811: The list of cells in a VirtualFlow is cleared every time the number of items changes

Reviewers

• Ajit Ghaisas (@aghaisas - Reviewer)
• Kevin Rushforth (@kevinrushforth - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/298/head:pull/298
$ git checkout pull/298

[bridgekeeper]

👋 Welcome back jpereda! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge]

Webrevs

• 00: Full (ab71146)

[johanvos]

This looks ok to me. It would be ideal to have the test failing before JDK-8113318 was applied, as that would show the issue for which JDK-8113318 was created is now fixed. But I do understand it is very hard to apply this test to a code snapshot from 2011.

[kevinrushforth]

/reviewers 2

[openjdk]

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

[aghaisas]

As there is no automated test to make sure we are not re-introducing the leak, I have verified by running Ensemble8 KeyEvent sample mentioned in JDK-8113318 and typing more than 800 keys. There is no performance degradation observed.

The change and test looks OK.

[kleopatra]

Just a note: fixing this makes JDK-8244826 disappear - not entirely certain, if it's really a fix for that one or only smearing over it.

[kevinrushforth]

Looks good to me.

[openjdk]

@jperedadnr This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8252811: The list of cells in a VirtualFlow is cleared every time the number of items changes

Reviewed-by: aghaisas, kcr

• If you would like to add a summary, use the /summary command.
• To credit additional contributors, use the /contributor command.
• To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 8 commits pushed to the master branch:

• 47e67b4: 8253123: Switch FX build to use JDK 15 as boot JDK
• 13ab2cb: 8252446: Screen.getScreens() is empty sometimes
• b2e2000: 8252546: Move ObservableValue's equality check and lazy evaluation descriptions to @implSpec
• 976a763: 8240499: Enforce whitespace checking for additional source files
• d6dee34: 8252547: Correct transformations docs in Node
• 0514116: 8246745: ListCell/Skin: misbehavior on switching skin
• 7506554: Merge
• 4b75750: 8252784: Create release notes for JavaFX 15 (8252784: Create release notes for JavaFX 15 #296)

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 47e67b40097add18a9986266d3d156f02af5bd36.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[jperedadnr]

/integrate

[openjdk]

@jperedadnr Since your change was applied there have been 8 commits pushed to the master branch:

• 47e67b4: 8253123: Switch FX build to use JDK 15 as boot JDK
• 13ab2cb: 8252446: Screen.getScreens() is empty sometimes
• b2e2000: 8252546: Move ObservableValue's equality check and lazy evaluation descriptions to @implSpec
• 976a763: 8240499: Enforce whitespace checking for additional source files
• d6dee34: 8252547: Correct transformations docs in Node
• 0514116: 8246745: ListCell/Skin: misbehavior on switching skin
• 7506554: Merge
• 4b75750: 8252784: Create release notes for JavaFX 15 (8252784: Create release notes for JavaFX 15 #296)

Your commit was automatically rebased without conflicts.

Pushed as commit d10f948.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.