RFR: 8309470: Potential performance improvements in VirtualFlow [v3]

[Karthik P K]

On Thu, 8 Jun 2023 07:18:22 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

>> This PR does two small improvements to `VirtualFlow`.
>> Until now, the `VirtualFlow` sometimes called the `computeHeight` or `computeWidth` methods, although a fixed cell size is set and we therefore don't need to call those method (and never should do, as we may don't get the expected result and mix computed sizes with the fixed cell size).
>> 
>> Added tests that fail before and pass now. They check that the `computeHeight` or `computeWidth` (non vertical flow) are never called when a fixed cell size is set.
>
> Marius Hanl has updated the pull request incrementally with one additional commit since the last revision:
> 
>   JDK-8309470: Improved Tests

Tested the changes in MacOS 13.3.1, I'm seeing failure of `testComputeHeightShouldNotBeCalledWhenFixedCellSizeIsSet` and `testComputeWidthShouldNotBeCalledWhenFixedCellSizeIsSet` with and without the changes of this PR.

VirtualFlowTest > testComputeHeightShouldNotBeCalledWhenFixedCellSizeIsSet FAILED
    java.lang.AssertionError: expected:<24.0> but was:<1337.0>
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:555)
        at org.junit.Assert.assertEquals(Assert.java:685)
        at test.javafx.scene.control.skin.VirtualFlowTest.testComputeHeightShouldNotBeCalledWhenFixedCellSizeIsSet(VirtualFlowTest.java:1566)

VirtualFlowTest > testComputeWidthShouldNotBeCalledWhenFixedCellSizeIsSet FAILED
    java.lang.AssertionError: expected:<24.0> but was:<1337.0>
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:555)
        at org.junit.Assert.assertEquals(Assert.java:685)
        at test.javafx.scene.control.skin.VirtualFlowTest.testComputeWidthShouldNotBeCalledWhenFixedCellSizeIsSet(VirtualFlowTest.java:1622)

Changes requested by kpk (Committer).

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 3065:

> 3063:             }
> 3064: 
> 3065:             // if we have a valid cell, we can populate the cache

Do we need this comment now?

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java line 1542:

> 1540:             @Override
> 1541:             protected double computeMinHeight(double width) {
> 1542:                 return 1337;

Can we add a final variable for this number since it is repeated many times?

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

Changes requested by kpk (Committer).

PR Review: https://git.openjdk.org/jfx/pull/1150#pullrequestreview-1469946391
PR Review: https://git.openjdk.org/jfx/pull/1150#pullrequestreview-1469948040
PR Review Comment: https://git.openjdk.org/jfx/pull/1150#discussion_r1223061740
PR Review Comment: https://git.openjdk.org/jfx/pull/1150#discussion_r1223062537