8325154: resizeColumnToFitContent is slower than it needs to be

[effad]

The PR simply moves column and view-updates outside the loop. Since the column or view never changes within the for-loop it is not necessary to call these again and again.

---

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 1 Reviewer, 1 Author)

Issue

• JDK-8325154: resizeColumnToFitContent is slower than it needs to be (Bug - P3)

Reviewers

• Andy Goryachev (@andy-goryachev-oracle - Reviewer)
• Marius Hanl (@Maran23 - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1358

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back rlichten! 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 (ad906b5d)

[kevinrushforth]

@andy-goryachev-oracle Can you review this?

/reviewers 2

[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 1 Reviewer, 1 Author).

[andy-goryachev-oracle]

I see it working right in macOS 14.2.1, by double-clicking on the column divider in the table header.

Question to @effad : could you provide the measurements for this fix (before/after) please?

A side comment: the default implementation of resizeColumnToFitContent() is to iterate over all the rows, and I think this is not right. The UI locks up if the table has, let's say, 10,000,000 rows. I think in this case it should instead pick a reasonable limited range (maybe a max 1000 rows or so) of rows based on what's currently visible, so as not to lock up the UI. This might be a separate improvement.

[effad]

I see it working right in macOS 14.2.1, by double-clicking on the column divider in the table header.

Question to @effad : could you provide the measurements for this fix (before/after) please?

Applying the patch improves performance (on my Threadripper 2950X)from:
JFX 21.0.2+5 average run time: 2460
to:
JFX 23-internal+0-2024-02-01-061822 average run time: 1119

so the speedup is quite remarkable and will probably be even better, if I follow @Maran23 advise to also pull cell.updateTableRow(tableRow); out of the loop. I will hopefully be able to provide an improved PR tomorrow.

A side comment: the default implementation of resizeColumnToFitContent() is to iterate over all the rows, and I think this is not right. The UI locks up if the table has, let's say, 10,000,000 rows. I think in this case it should instead pick a reasonable limited range (maybe a max 1000 rows or so) of rows based on what's currently visible, so as not to lock up the UI. This might be a separate improvement.

I agree. However, in order to "not break things" we would have to introduce an API to users to be able to control the number of rows they want to be taken into account. Definitely a separate improvement.

[Maran23]

A side comment: the default implementation of resizeColumnToFitContent() is to iterate over all the rows, and I think this is not right. The UI locks up if the table has, let's say, 10,000,000 rows. I think in this case it should instead pick a reasonable limited range (maybe a max 1000 rows or so) of rows based on what's currently visible, so as not to lock up the UI. This might be a separate improvement.

I agree as well. This is on my todo list for quite a while.
The default auto size is done with 30 rows. If you double click the edge of the header, it will take ALL rows, which can be very slow.
So +1 here. Also another idea I had is to take 15 top rows and 15 bttom rows into account. In case the table is sorted.
We can disuss this more in a seperate issue.

I will review this PR soon as well.

[andy-goryachev-oracle]

, in order to "not break things" we would have to introduce an API to users to be able to control the number of rows they want to be taken into account.

I think the UI "breaking itself" is much worse than us changing the (buggy) behavior. The user gains very little if the UI locks up for several seconds (or minutes) trying to size every row.

At the same time the user might simply double click on the header again (or better, just resize the offending column like they used to do since time immemorial) once a wide cell comes into view.

Basically, I don't think the new API is required in this particular case; I think it's a bug and it should either size the visible rows or (visible + some margin). Of course, there is one more operation which guarantees a lock up with large models - sorting. There is no easy solution for sorting, and that's where we might need new APIs. And it is a totally separate issue.

[bourgesl]

A side comment: the default implementation of resizeColumnToFitContent() is to iterate over all the rows, and I think this is not right. The UI locks up if the table has, let's say, 10,000,000 rows. I think in this case it should instead pick a reasonable limited range (maybe a max 1000 rows or so) of rows based on what's currently visible, so as not to lock up the UI. This might be a separate improvement.

Here is my jtable autofit column that uses sampling to avoid processing all rows:
https://github.com/JMMC-OpenDev/jmcs/blob/61db740407dce5b38301ff67a8a3d77aa84ad3fe/src/main/java/fr/jmmc/jmcs/gui/util/AutofitTableColumns.java#L199

[mlbridge]

Mailing list message from John Hendrikx on openjfx-dev:

Hi, just wanted to add my 2 cents,

In my opinion, no operation in List/Table/TreeTable should be doing such
measurements on all rows or columns (where cells need to be loaded, CSS
aplied, etc) -- it just won't scale.? In my application many of my lists
contain images, and pre-loading all of these to do measurements is just
not possible (luckily I can use fixed size cells).

So, I think that by default it should work on *visible* cells only, and
use estimates for the rest of the cells.? This is how browsers deal with
huge tables (they for example won't preload every image in a table to
"measure" the size of cells).? The estimates can be adjusted as more
cells have become visible (or nothing happens at all if the user is not
interacting with the control).

As I think there are wildly different requirements for each view, I
think it may be a good idea to implement size estimations using a
Strategy pattern (like the FocusModel and SelectionModel), where the
default strategy is based on visible cells, and can be replaced with
other strategies that measure or estimate differently.? Already there
are implementations and ideas for:

- Fixed size
- Visible cells only + estimates
- All cells
- First 15, bottom 15
- First 1000 rows

I don't think there will be a one size fits all, so I think
externalizing this problem and having the user choose or even implement
their own would be a more tenable solution.? I feel this problem has
gone back and forth several times now during the life time of TableView :)

--John

On 05/02/2024 10:48, Marius Hanl wrote:

[effad]

, in order to "not break things" we would have to introduce an API to users to be able to control the number of rows they want to be taken into account.

I think the UI "breaking itself" is much worse than us changing the (buggy) behavior. The user gains very little if the UI locks up for several seconds (or minutes) trying to size every row.

I don't agree. Our production application usually loads 200 rows and lets the user load as many more rows as they see fit. It also provides a "fit all columns" function which will fit all columns of the table.
Users are fine with a few seconds of blockage if they have loaded thousands of rows. But if the "fit all columns" function will no longer work, they will report a bug which I will not be able to fix, if JavaFX suddenly refuses to iterate over all the rows as it does now.

So maybe we should:

• Provide an API that let's the client specify how many rows should be taken into account
• Make resizeColumnToFitContent available for clients

At the same time the user might simply double click on the header again (or better, just resize the offending column like they used to do since time immemorial) once a wide cell comes into view.

Basically, I don't think the new API is required in this particular case; I think it's a bug and it should either size the visible rows or (visible + some margin). Of course, there is one more operation which guarantees a lock up with large models - sorting. There is no easy solution for sorting, and that's where we might need new APIs. And it is a totally separate issue.

[andy-goryachev-oracle]

So maybe we should:

• Provide an API that let's the client specify how many rows should be taken into account
• Make resizeColumnToFitContent available for clients

Let's create an RFE.
What would be your recommendation for the specific API?

[effad]

So maybe we should:

• Provide an API that let's the client specify how many rows should be taken into account
• Make resizeColumnToFitContent available for clients

Let's create an RFE. What would be your recommendation for the specific API?

At first I had only thought about a simple "setColumnFitSamplSize(int sampleSize)" method. However, @hjohn s suggestion of a strategy pattern seems convincing to me: It allows the client to specify how the required column width is calculated.

I'm not quite sure how to approach this however. If we have a "ColumnFitStrategy" interface/baseclass with just a simple "calculateColumnWidth(int column)" this would not help clients since they don't have access to a lot of the stuff that's happening inside resizeColumnToFitContent.
If, on the other hand, we only allow a "ColumnFitRowSelection" interface/baseclass that will return the rows to be used for measuring, the client code will not be able to optimize in some cases (e.g. you have graphics in your table; loading them is expensive; but you know the size of the graphics and could thus give a "correct" required size without really loading/applying css).

[effad]

I will review this PR soon as well.

The discussion about a "bigger" solution aside I think the PR is still of value to improve performance, so I look forward to your review.

[effad]

I've filed https://bugs.openjdk.org/browse/JDK-8325390 to prevent this PR from getting too much clutter. I hope this is the right approach for an RFE ...

[Maran23]

we could also check if we need/can call tableRow.applyCss(); just only one time (below) (e.g. at first?),
May works and improve performance, or may not work

[effad]

That doesn't work unfortunately. I tried this:

    boolean tableRowHasCss = false;
    for (int row = 0; row < rows; row++) {
        tableRow.updateIndex(row);
        cell.updateIndex(row);

        if ((cell.getText() != null && !cell.getText().isEmpty()) || cell.getGraphic() != null) {
            if (!tableRowHasCss) {
                tableRow.applyCss();
                tableRowHasCss = true;
            }
            maxWidth = Math.max(maxWidth, cell.prefWidth(-1));
        }
    }

but if you have a table with this css:

.oddbig .table-row-cell:odd{
    -fx-font-size: 36;
}

Then only the css of the first row will be taken into account, i.e. the column wont size correctly.
I've recorded a little video of the effect at https://youtu.be/-p0pv-i4K2s

[Maran23]

Thanks for checking!

[Maran23]

Looks good to me.

Checking updateTableColumn and updateTableView, I can see that those operation will clean up and register a lot of properties and listener, so it is a good thing to do that just once (since we never change the column or table inbetween).

[openjdk]

@effad This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8325154: resizeColumnToFitContent is slower than it needs to be

Reviewed-by: angorya, mhanl

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 7 new commits pushed to the master branch:

• de0255d: 8307117: TextArea: wrapText property ignored when changing font
• a7f6de8: 8325258: Additional WebKit 617.1 fixes from WebKitGTK 2.42.5
• a39732a: 8311492: FontSmoothingType LCD produces wrong color when transparency is used
• 172c491: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0
• 17dfab0: 8320912: IME should commit on focus change
• 12816b5: 8325093: Update CONTRIBUTING.md for build jdk version
• aac2df1: 8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@andy-goryachev-oracle, @Maran23) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[andy-goryachev-oracle]

I hope this is the right approach for an RFE ...

thank you for filing an RFE, @effad

[effad]

/integrate

[openjdk]

@effad
Your change (at version ad906b5) is now ready to be sponsored by a Committer.

[andy-goryachev-oracle]

/sponsor

[openjdk]

Going to push as commit e2f42c5.
Since your change was applied there have been 7 commits pushed to the master branch:

• de0255d: 8307117: TextArea: wrapText property ignored when changing font
• a7f6de8: 8325258: Additional WebKit 617.1 fixes from WebKitGTK 2.42.5
• a39732a: 8311492: FontSmoothingType LCD produces wrong color when transparency is used
• 172c491: 8312603: ArrayIndexOutOfBoundsException in Marlin when scaleX is 0
• 17dfab0: 8320912: IME should commit on focus change
• 12816b5: 8325093: Update CONTRIBUTING.md for build jdk version
• aac2df1: 8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin

Your commit was automatically rebased without conflicts.

[openjdk]

@andy-goryachev-oracle @effad Pushed as commit e2f42c5.

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