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

[John Hendrikx]

On Thu, 2 Oct 2025 19:47:41 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
> 
>  - Merge branch 'master' of https://github.com/openjdk/jfx into 8359599-refresh-recreates-all
>  - Calling refresh() for all virtualized controls recreates all cells instead of refreshing the cells

I think the changes look good.  I'm a bit confused in the performance table with what is meant with the `50 ms -> 0 ms` in the "after" cases though?

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableRowSkinTest.java line 368:

> 366:         JMemoryBuddy.assertCollectable(ref);
> 367:     }
> 368: 

Is there another test that verifies that cells are garbage collectable?  For example, in the case where a table / list / tree table becomes smaller visually, I think that it should then perhaps discard some cells that then should be collectable?

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

PR Review: https://git.openjdk.org/jfx/pull/1830#pullrequestreview-3297470172
PR Review Comment: https://git.openjdk.org/jfx/pull/1830#discussion_r2400825922