RFR: 8359599: Calling refresh() for all virtualized controls recreates all cells instead of refreshing the cells [v3]

[Andy Goryachev]

On Sun, 5 Oct 2025 20:10:02 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

>> When calling `refresh()` on virtualized Controls (`ListView`, `TreeView`, `TableView`, `TreeTableView`), all cells will be recreated completely, instead of just refreshing them.
>> 
>> This is because `recreateCells()` of the `VirtualFlow` is called when `refresh()` was called. This is not needed, since refreshing the cells can be done much cheaper with `rebuildCells()`.
>> 
>> This will reset all cells (`index = -1`), add them to the pile and fill them back in the viewport with an index again. This ensures `updateItem()` is called.
>> 
>> The contract of `refresh()` is also a big vague, stating:
>> 
>> Calling {@code refresh()} forces the XXX control to recreate and repopulate the cells 
>> necessary to populate the visual bounds of the control.
>> In other words, this forces the XXX to update what it is showing to the user. 
>> This is useful in cases where the underlying data source has changed in a way that is not observed by the XXX itself.
>> 
>> 
>> As written above, recreating is not needed in order to fulfull the contract of updating what is shown to the user in case the underlying data source changed without JavaFX noticing (e.g. calling a normal Setter without any Property and therefore listener involved).
>> 
>> ----
>> 
>> Benchmarks
>> -
>> 
>> Setup:
>> - `TableView`
>> - `100 TableColumns`
>> - `1000 Items`
>> 
>> Calling `refresh()` with a `Button` press.
>> 
>> | WHAT | BEFORE | AFTER |
>> | - | - | - |
>> | Init | 0ms |0 ms |
>> | Init | 0ms | 0 ms |
>> | Init | 251 ms | 246 ms |
>> | Init | 47 ms | 50 ms |
>> | Init | 24 ms | 5 ms |
>> | Refresh Nr. 1 | 238 ms | 51 ms -> 0ms |
>> | Refresh Nr. 2 | 282 ms | 35 ms -> 0ms |
>> | Refresh Nr. 3 | 176 ms | 36 ms -> 0ms |
>> | Refresh Nr. 4 | 151 ms | 39 ms -> 0ms |
>> | Refresh Nr. 5 | 156 ms | 34 ms -> 0ms |
>> 
>> 
>> 
>> <details>
>> <summary>Benchmark code</summary>
>> 
>> 
>> import javafx.application.Application;
>> import javafx.beans.property.SimpleStringProperty;
>> import javafx.scene.Scene;
>> import javafx.scene.control.Button;
>> import javafx.scene.control.IndexedCell;
>> import javafx.scene.control.TableColumn;
>> import javafx.scene.control.TableRow;
>> import javafx.scene.control.TableView;
>> import javafx.scene.control.skin.TableViewSkin;
>> import javafx.scene.control.skin.VirtualFlow;
>> import javafx.scene.layout.BorderPane;
>> import javafx.scene.layout.HBox;
>> import javafx.stage.Stage;
>> 
>> public class FXBug {
>> 
>>     public static void main(String[] args) {
>>         Application.launch(FxApp.class, args);
>>     }
>> 
>>  ...
>
> Marius Hanl has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - remove newline from imports
>  - assert row creation count as well

I don't see any obvious issues with the new code.

Performance wise, feels no different than the master branch.  Using the monkey tester with 10,000 rows and 200 columns, both feel sluggish on vertical scrolling, which disappears once a fixedCellSize is set.

modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 1767:

> 1765:     /**
> 1766:      * Calling {@code refresh()} forces the TableView control to repopulate the
> 1767:      * cells necessary to populate the visual bounds of the control.

the word 'repopulate' is used twice (here and elsewhere)

I would suggest to rephrase these comments to indicate what happens exactly, possibly borrowing text from the `VirtualFlow`:

- recreate: a layout pass should be done, and that the cell factory has changed. All cells in the viewport are recreated with the new cell factory.
- rebuild: a layout pass should be done, and cell contents have changed. All cells are removed and then added properly in the viewport

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableViewSkinBase.java line 305:

> 303:                 return;
> 304:             }
> 305:             if (Properties.RECREATE.equals(c.getKey())) {

I just have to raise this concern.

It looks weird to me to use properties as a roundabout way to invoke a hidden method in the skin.  Node properties, a public facility, can be easily mutated by unrelated code, making it easy to break the intended functionality.

Why not make these two methods explicitly a part of the public API by replacing `requestRebuildCells` and `requestRecreateCells` with


protected VirtualContainerBase.rebuildCells()
protected VirtualContainerBase.recreateCells()


? 

(requestXXX is a misnomer - it might suggest an async nature, whereas it these are synchronous methods)

-------------

Marked as reviewed by angorya (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1830#pullrequestreview-3325735209
PR Review Comment: https://git.openjdk.org/jfx/pull/1830#discussion_r2421948069
PR Review Comment: https://git.openjdk.org/jfx/pull/1830#discussion_r2422022393