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

[Maran23]

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).

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)
• Change requires a CSR request matching fixVersion jfx26 to be approved (needs to be created)

Issue

• JDK-8359599: Calling refresh() for all virtualized controls recreates all cells instead of refreshing the cells (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1830/head:pull/1830
$ git checkout pull/1830

Update a local copy of the PR:
$ git checkout pull/1830
$ git pull https://git.openjdk.org/jfx.git pull/1830/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1830

View PR using the GUI difftool:
$ git pr show -t 1830

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1830.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mhanl! 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.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[mlbridge]

Webrevs

• 00: Full (c9843e79)

[kevinrushforth]

This is a risky change and will need to be carefully tested.

Reviewers: @andy-goryachev-oracle @johanvos

/reviewers 2 reviewers

[openjdk]

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

[johanvos]

It is indeed true that refresh() is often a very expensive operation. Whenever VirtualFlow.recreateCells() is called, most of the internal state of the VirtualFlow is destroyed, and everything is recalculated from scratch. I believe that is not a problem, as long as recreateCells() is not called (by the controls) unless really needed. In that light, this PR seems interesting, as it will limit the number of rebuild-from-scratch cases.
I ran the basic controls tests after applying the PR, and (a bit to my surprise) they all passed, which is great.
However, it is very likely that some code out there implicitly rely on the rebuild-from-scratch logic, and that code will then work different after applying this PR. I believe it would be good to find such a case where the behavior (which, I agree, is often a bit vaguely defined) changes, so that we can discuss this with a concrete example.

Hence, while I like the idea here (avoiding unneeded heavy-cost operations in VirtualFlow), I would like to avoid a number of follow-up issues once this is merged -- driven by a change in "expected" behavior.

[Maran23]

Hence, while I like the idea here (avoiding unneeded heavy-cost operations in VirtualFlow), I would like to avoid a number of follow-up issues once this is merged -- driven by a change in "expected" behavior.

I completely agree. We need to be careful with such changes.

However, it is very likely that some code out there implicitly rely on the rebuild-from-scratch logic, and that code will then work different after applying this PR.

Since all rows (and cells) are reset and then updated, all changes that were not taken into account by the control are taken into account in any case then.
On a side note here: In any JavaFX project, I have overwritten the refresh method (since it is not final) and always implemented the lightweight method as proposed here. I never found any problem.

But I think there is one concrete case which breaks now.
Take the following example (slightly modified version of the code in the ticket):

Example

import java.util.concurrent.atomic.AtomicBoolean;

import javafx.application.Application;
import javafx.beans.property.SimpleStringProperty;
import javafx.collections.FXCollections;
import javafx.geometry.Insets;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.TableColumn;
import javafx.scene.control.TableRow;
import javafx.scene.control.TableView;
import javafx.scene.layout.Priority;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;

public class TableRefresh extends Application {

    public static void main(String[] args) {
        launch(args);
    }

    @Override
    public void start(Stage stage) throws Exception {
        TableView<String> tv = new RefreshTableView<>();
        tv.setEditable(true);
        AtomicBoolean alternate = new AtomicBoolean(false);

        tv.setRowFactory(e -> {
            if (alternate.get()) {
                System.out.println("Creating alternative row");
                return new TableRow<>() {
                    {
                        setPadding(new Insets(5));
                    }
                };
            }

            System.out.println("Creating row");
            return new TableRow<>();
        });
        TableColumn<String, String> col = new TableColumn<>("Name");
        col.setCellValueFactory(i -> new SimpleStringProperty(i.getValue()));

        tv.getColumns().addAll(col);

        tv.setItems(FXCollections.observableArrayList("A", "B"));
        VBox.setVgrow(tv, Priority.ALWAYS);
        Button btn = new Button("Refresh");
        btn.setOnAction(e -> {
            alternate.set(true);
            tv.refresh();
        });
        Scene scene = new Scene(new VBox(tv, btn));

        stage.setScene(scene);
        stage.setTitle("Table Refresh");
        stage.show();
    }
}

Here, I'm aware that refresh() is recreating all rows, and update a boolean flag before calling refresh(), which leads to another path that is picked up and therefore other rows.
In this case, it is much better to just set a new TableRow factory. This will do the same as what refresh() is doing right now (but not after this PR).
In never saw this code in the wild though.

But that is still a valid risk that we need to consider. This is the only problem I see right now.

[bridgekeeper]

@Maran23 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[andy-goryachev-oracle]

My main concern with this PR is that we might get a minute performance improvement, while risking regression. Would it be possible to get some measurements using a modern system?

What do you think?

[johanvos]

I think the performance improvements due to this PR can be pretty significant. The problem is indeed the risk on regression. I believe we need improvements in 2 areas before we can safely merge this:

1. More functional regression tests
2. Performance tests.

[hjohn]

I think a CSR is needed if we're changing the documentation. It specifically says it recreates the cells (although the purpose of that eludes me, aside from badly written cells).

Since cells are supposed to be updated, and not "retain" anything from previous contents, I think only only buggy cells would benefit from recreating. Such buggy cells however would probably have subtle problems already when they're never recreated, like listener leaks.

So although I agree that recreating is not needed as cell functionality is defined by their update methods, I do think this then requires a CSR.

[kevinrushforth]

So although I agree that recreating is not needed as cell functionality is defined by their update methods, I do think this then requires a CSR.

I agree.

/csr needed

[openjdk]

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@Maran23 please create a CSR request for issue JDK-8359599 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[bridgekeeper]

@Maran23 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!