Skip to content

Commit 9ad0e90

Browse files
author
Marius Hanl
committed
8309470: Potential performance improvements in VirtualFlow
Reviewed-by: kpk, angorya
1 parent 6a159e9 commit 9ad0e90

File tree

2 files changed

+203
-26
lines changed

2 files changed

+203
-26
lines changed

‎modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java‎

Lines changed: 12 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2010, 2023, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -1117,7 +1117,7 @@ void adjustPosition() {
11171117

11181118
if (needsCellsLayout) {
11191119
for (int i = 0, max = cells.size(); i < max; i++) {
1120-
Cell<?> cell = cells.get(i);
1120+
T cell = cells.get(i);
11211121
if (cell != null) {
11221122
cell.requestLayout();
11231123
}
@@ -1161,7 +1161,7 @@ void adjustPosition() {
11611161

11621162
if (!cellNeedsLayout) {
11631163
for (int i = 0; i < cells.size(); i++) {
1164-
Cell<?> cell = cells.get(i);
1164+
T cell = cells.get(i);
11651165
cellNeedsLayout = cell.isNeedsLayout();
11661166
if (cellNeedsLayout) break;
11671167
}
@@ -1951,7 +1951,7 @@ void setViewportLength(double value) {
19511951
/**
19521952
* Gets the breadth of a specific cell
19531953
*/
1954-
double getCellBreadth(Cell cell) {
1954+
double getCellBreadth(T cell) {
19551955
return isVertical() ?
19561956
cell.prefWidth(-1)
19571957
: cell.prefHeight(-1);
@@ -1991,12 +1991,11 @@ private void positionCell(T cell, double position) {
19911991
protected void resizeCell(T cell) {
19921992
if (cell == null) return;
19931993

1994+
double size = Math.max(getMaxPrefBreadth(), getViewportBreadth());
19941995
if (isVertical()) {
1995-
double width = Math.max(getMaxPrefBreadth(), getViewportBreadth());
1996-
cell.resize(width, fixedCellSizeEnabled ? getFixedCellSize() : Utils.boundedSize(cell.prefHeight(width), cell.minHeight(width), cell.maxHeight(width)));
1996+
cell.resize(size, fixedCellSizeEnabled ? getFixedCellSize() : Utils.boundedSize(cell.prefHeight(size), cell.minHeight(size), cell.maxHeight(size)));
19971997
} else {
1998-
double height = Math.max(getMaxPrefBreadth(), getViewportBreadth());
1999-
cell.resize(fixedCellSizeEnabled ? getFixedCellSize() : Utils.boundedSize(cell.prefWidth(height), cell.minWidth(height), cell.maxWidth(height)), height);
1998+
cell.resize(fixedCellSizeEnabled ? getFixedCellSize() : Utils.boundedSize(cell.prefWidth(size), cell.minWidth(size), cell.maxWidth(size)), size);
20001999
}
20012000
}
20022001

@@ -2693,18 +2692,11 @@ private void updateScrollBarsAndCells(boolean recreate) {
26932692
* offset.
26942693
*/
26952694
private void fitCells() {
2696-
double size = Math.max(getMaxPrefBreadth(), getViewportBreadth());
2697-
boolean isVertical = isVertical();
2698-
26992695
// Note: Do not optimise this loop by pre-calculating the cells size and
27002696
// storing that into a int value - this can lead to RT-32828
27012697
for (int i = 0; i < cells.size(); i++) {
2702-
Cell<?> cell = cells.get(i);
2703-
if (isVertical) {
2704-
cell.resize(size, cell.prefHeight(size));
2705-
} else {
2706-
cell.resize(cell.prefWidth(size), size);
2707-
}
2698+
T cell = cells.get(i);
2699+
resizeCell(cell);
27082700
}
27092701
}
27102702

@@ -2823,7 +2815,7 @@ private void cleanPile() {
28232815
}
28242816
}
28252817

2826-
private boolean doesCellContainFocus(Cell<?> c) {
2818+
private boolean doesCellContainFocus(T c) {
28272819
Scene scene = c.getScene();
28282820
final Node focusOwner = scene == null ? null : scene.getFocusOwner();
28292821

@@ -3070,12 +3062,7 @@ private double getOrCreateCellSize (int idx, boolean create) {
30703062
doRelease = true;
30713063
}
30723064

3073-
// if we have a valid cell, we can populate the cache
3074-
if (isVertical()) {
3075-
answer = cell.getLayoutBounds().getHeight();
3076-
} else {
3077-
answer = cell.getLayoutBounds().getWidth();
3078-
}
3065+
answer = getCellLength(cell);
30793066
itemSizeCache.set(idx, answer);
30803067

30813068
if (doRelease) { // we need to release the accumcell
@@ -3103,7 +3090,7 @@ void updateCellSize(T cell) {
31033090

31043091
if (itemSizeCache.size() > cellIndex) {
31053092
Double oldSize = itemSizeCache.get(cellIndex);
3106-
double newSize = isVertical() ? cell.getLayoutBounds().getHeight() : cell.getLayoutBounds().getWidth();
3093+
double newSize = getCellLength(cell);
31073094
itemSizeCache.set(cellIndex, newSize);
31083095
if ((oldSize != null) && !oldSize.equals(newSize)) {
31093096
int currentIndex = computeCurrentIndex();

‎modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java‎

Lines changed: 191 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2010, 2023, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -28,10 +28,12 @@
2828
import java.util.AbstractList;
2929
import static org.junit.Assert.assertEquals;
3030
import static org.junit.Assert.assertFalse;
31+
import static org.junit.Assert.assertNotEquals;
3132
import static org.junit.Assert.assertNotNull;
3233
import static org.junit.Assert.assertNull;
3334
import static org.junit.Assert.assertSame;
3435
import static org.junit.Assert.assertTrue;
36+
import static org.junit.Assert.fail;
3537

3638
import java.util.Iterator;
3739
import java.util.LinkedList;
@@ -1523,6 +1525,194 @@ protected double computeMaxHeight(double width) {
15231525
assertEquals(3, firstCell.getIndex());
15241526
assertEquals(-10, firstCell.getLayoutY(),1);
15251527
}
1528+
1529+
/**
1530+
* The VirtualFlow should never call the compute height methods when a fixed cell size is set.
1531+
* If it is called the height will be wrong for some cells.
1532+
*/
1533+
@Test
1534+
public void testComputeHeightShouldNotBeUsedWhenFixedCellSizeIsSet() {
1535+
int cellSize = 24;
1536+
1537+
flow = new VirtualFlowShim<>();
1538+
flow.setFixedCellSize(cellSize);
1539+
flow.setCellFactory(p -> new CellStub(flow) {
1540+
1541+
@Override
1542+
protected double computeMinHeight(double width) {
1543+
return 1337;
1544+
}
1545+
1546+
@Override
1547+
protected double computeMaxHeight(double width) {
1548+
return 1337;
1549+
}
1550+
1551+
@Override
1552+
protected double computePrefHeight(double width) {
1553+
return 1337;
1554+
}
1555+
});
1556+
flow.setCellCount(100);
1557+
flow.resize(cellSize * 10, cellSize * 10);
1558+
1559+
pulse();
1560+
pulse();
1561+
1562+
for (int i = 0; i < 10; i++) {
1563+
IndexedCell<?> cell = flow.getCell(i);
1564+
double cellPosition = flow.getCellPosition(cell);
1565+
int expectedPosition = i * cellSize;
1566+
assertEquals(expectedPosition, cellPosition, 0d);
1567+
assertEquals(cellSize, cell.getHeight(), 0d);
1568+
1569+
assertNotEquals(cellSize, cell.getWidth(), 0d);
1570+
}
1571+
1572+
flow.scrollPixels(cellSize * 10);
1573+
1574+
for (int i = 10; i < 20; i++) {
1575+
IndexedCell<?> cell = flow.getCell(i);
1576+
double cellPosition = flow.getCellPosition(cell);
1577+
int expectedPosition = (i - 10) * cellSize;
1578+
assertEquals(expectedPosition, cellPosition, 0d);
1579+
assertEquals(cellSize, cell.getHeight(), 0d);
1580+
1581+
assertNotEquals(cellSize, cell.getWidth(), 0d);
1582+
}
1583+
}
1584+
1585+
/**
1586+
* The VirtualFlow should never call the compute width methods when a fixed cell size is set.
1587+
* If it is called the width will be wrong for some cells.
1588+
*/
1589+
@Test
1590+
public void testComputeWidthShouldNotBeUsedWhenFixedCellSizeIsSet() {
1591+
int cellSize = 24;
1592+
1593+
flow = new VirtualFlowShim<>();
1594+
flow.setVertical(false);
1595+
flow.setFixedCellSize(cellSize);
1596+
flow.setCellFactory(p -> new CellStub(flow) {
1597+
1598+
@Override
1599+
protected double computeMinWidth(double height) {
1600+
return 1337;
1601+
}
1602+
1603+
@Override
1604+
protected double computeMaxWidth(double height) {
1605+
return 1337;
1606+
}
1607+
1608+
@Override
1609+
protected double computePrefWidth(double height) {
1610+
return 1337;
1611+
}
1612+
});
1613+
flow.setCellCount(100);
1614+
flow.resize(cellSize * 10, cellSize * 10);
1615+
1616+
pulse();
1617+
pulse();
1618+
1619+
for (int i = 0; i < 10; i++) {
1620+
IndexedCell<?> cell = flow.getCell(i);
1621+
double cellPosition = flow.getCellPosition(cell);
1622+
int expectedPosition = i * cellSize;
1623+
assertEquals(expectedPosition, cellPosition, 0d);
1624+
assertEquals(cellSize, cell.getWidth(), 0d);
1625+
1626+
assertNotEquals(cellSize, cell.getHeight(), 0d);
1627+
}
1628+
1629+
flow.scrollPixels(cellSize * 10);
1630+
1631+
for (int i = 10; i < 20; i++) {
1632+
IndexedCell<?> cell = flow.getCell(i);
1633+
double cellPosition = flow.getCellPosition(cell);
1634+
int expectedPosition = (i - 10) * cellSize;
1635+
assertEquals(expectedPosition, cellPosition, 0d);
1636+
assertEquals(cellSize, cell.getWidth(), 0d);
1637+
1638+
assertNotEquals(cellSize, cell.getHeight(), 0d);
1639+
}
1640+
}
1641+
1642+
/**
1643+
* The VirtualFlow should never call the compute height methods when a fixed cell size is set.
1644+
*/
1645+
@Test
1646+
public void testComputeHeightShouldNotBeCalledWhenFixedCellSizeIsSet() {
1647+
int cellSize = 24;
1648+
1649+
flow = new VirtualFlowShim<>();
1650+
flow.setFixedCellSize(cellSize);
1651+
flow.setCellFactory(p -> new CellStub(flow) {
1652+
1653+
@Override
1654+
protected double computeMinHeight(double width) {
1655+
fail();
1656+
return 1337;
1657+
}
1658+
1659+
@Override
1660+
protected double computeMaxHeight(double width) {
1661+
fail();
1662+
return 1337;
1663+
}
1664+
1665+
@Override
1666+
protected double computePrefHeight(double width) {
1667+
fail();
1668+
return 1337;
1669+
}
1670+
});
1671+
flow.setCellCount(100);
1672+
flow.resize(cellSize * 10, cellSize * 10);
1673+
1674+
// Trigger layout and see if the computeXXX method are called above.
1675+
pulse();
1676+
pulse();
1677+
}
1678+
1679+
/**
1680+
* The VirtualFlow should never call the compute width methods when a fixed cell size is set.
1681+
*/
1682+
@Test
1683+
public void testComputeWidthShouldNotBeCalledWhenFixedCellSizeIsSet() {
1684+
int cellSize = 24;
1685+
1686+
flow = new VirtualFlowShim<>();
1687+
flow.setVertical(false);
1688+
flow.setFixedCellSize(cellSize);
1689+
flow.setCellFactory(p -> new CellStub(flow) {
1690+
1691+
@Override
1692+
protected double computeMinWidth(double height) {
1693+
fail();
1694+
return 1337;
1695+
}
1696+
1697+
@Override
1698+
protected double computeMaxWidth(double height) {
1699+
fail();
1700+
return 1337;
1701+
}
1702+
1703+
@Override
1704+
protected double computePrefWidth(double height) {
1705+
fail();
1706+
return 1337;
1707+
}
1708+
});
1709+
flow.setCellCount(100);
1710+
flow.resize(cellSize * 10, cellSize * 10);
1711+
1712+
// Trigger layout and see if the computeXXX method are called above.
1713+
pulse();
1714+
pulse();
1715+
}
15261716
}
15271717

15281718
class GraphicalCellStub extends IndexedCellShim<Node> {

0 commit comments

Comments
 (0)