Skip to content

8246745: ListCell/Skin: misbehavior on switching skin #251

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kleopatra wants to merge 2 commits into from
Closed

8246745: ListCell/Skin: misbehavior on switching skin #251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c21ed22
8246745: ListCell/Skin: misbehavior on switching skin
kleopatra Jun 10, 2020
75c209a
added code comment as requested in review
kleopatra Sep 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 14 additions & 38 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/ListCellSkin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,13 @@
package javafx.scene.control.skin;

import com.sun.javafx.scene.control.behavior.BehaviorBase;
import javafx.beans.InvalidationListener;
import javafx.beans.Observable;
import com.sun.javafx.scene.control.behavior.ListCellBehavior;

import javafx.geometry.Orientation;
import javafx.scene.Node;
import javafx.scene.control.Accordion;
import javafx.scene.control.Button;
import javafx.scene.control.Control;
import javafx.scene.control.ListCell;
import javafx.scene.control.ListView;

import com.sun.javafx.scene.control.behavior.ListCellBehavior;
import javafx.scene.layout.Region;

/**
* Default skin implementation for the {@link ListCell} control.
Expand All @@ -52,12 +48,8 @@ public class ListCellSkin<T> extends CellSkinBase<ListCell<T>> {
* *
**************************************************************************/

private double fixedCellSize;
private boolean fixedCellSizeEnabled;
private final BehaviorBase<ListCell<T>> behavior;



/***************************************************************************
* *
* Constructors *
Expand All @@ -77,31 +69,8 @@ public ListCellSkin(ListCell<T> control) {
// install default input map for the ListCell control
behavior = new ListCellBehavior<>(control);
// control.setInputMap(behavior.getInputMap());

setupListeners();
}

private void setupListeners() {
ListView listView = getSkinnable().getListView();
if (listView == null) {
getSkinnable().listViewProperty().addListener(new InvalidationListener() {
@Override public void invalidated(Observable observable) {
getSkinnable().listViewProperty().removeListener(this);
setupListeners();
}
});
} else {
this.fixedCellSize = listView.getFixedCellSize();
this.fixedCellSizeEnabled = fixedCellSize > 0;
registerChangeListener(listView.fixedCellSizeProperty(), e -> {
this.fixedCellSize = getSkinnable().getListView().getFixedCellSize();
this.fixedCellSizeEnabled = fixedCellSize > 0;
});
}
}



/***************************************************************************
* *
* Public API *
Expand All @@ -127,7 +96,8 @@ private void setupListeners() {

/** {@inheritDoc} */
@Override protected double computePrefHeight(double width, double topInset, double rightInset, double bottomInset, double leftInset) {
if (fixedCellSizeEnabled) {
double fixedCellSize = getFixedCellSize();
if (fixedCellSize > 0) {
Copy link
Member

@arapte arapte Jun 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These compute methods get invoked multiple times during each layout pass(10s of times). Fetching the fixed cell size on each call to these methods seems to be repeated and costly operation compared to previous boolean check. I think we should keep the previous way of handling it: registering the change listener to listView.fixedCellSizeProperty().

Copy link
Collaborator Author

@kleopatra kleopatra Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ehh .. last time I did such micro-optimization was in the 80ies of last century ;)

Are there any performance measurements anywhere to demonstrate the impact?

Copy link
Collaborator Author

@kleopatra kleopatra Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit less flippant: really interested in the measurements - certainly, they are somewhere but can't find anything. Any pointer where to look?

Copy link
Member

@arapte arapte Jun 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even I do not know if there are any performance measurements for such scenario. :) May be @kevinrushforth could help in that.
I just counted the calls by adding a log, and observed that for a ListView with only one item, these three methods combinely are called 23 times when the Stage is displayed for the first time and 5 times when the item is selected. (updated the counts after rechecking, looks like the methods are invoked for empty cells too, which seems correct)

Copy link
Collaborator Author

@kleopatra kleopatra Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, do you mean a list with height of one cell only?

My experiment (having counters per instance and per computeXXHeight), logs 1 (or 2) for computePref with fixed size and 5 (1 each for min/max and 3 for pref) without fixedSize per layout pass. So it's 5* access of either the local field or the listView method .. wouldn't expect any problems, but then assumed performance is reading in coffee ground :)

Let's hope there are some metrics to use :)

Copy link
Member

@arapte arapte Jun 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, do you mean a list with height of one cell only?

Yes, List with one cell height.
In continuation to my previous comment, looks like 8 of the first 23 calls when window is displayed for the first time are NOT for the one visible cell. These 8 calls are for some intermediate cell.
I added a single counter for all three computeXXHeight methods and verified with a list of height of one cell.
When fixedCellSize is not set, total combined number of calls for a cell are,

  1. 15, when Stage is displayed for the first time.(+8 for an intermediate cell, not sure why)
  2. 5, when the list item is selected.
  3. 5, when the Window is resized.

Copy link
Collaborator Author

@kleopatra kleopatra Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something similar as I'm seeing - that's good :) Uploaded my play code to gist: the idea is to do something (like f.i. moving the selection), then press f1 to log the calls to each cell (the skin has a final instance counter and counters for calling the compute methods).

As much fun as this is (really like to dig until I'm dirty all over :) - at the end of the day we'll need some measurements (doing these is not so much fun, still hoping something already available)

Copy link
Member

@kevinrushforth kevinrushforth Sep 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally we implement this sort of lazy evaluation (caching + invalidation) when there is a computation that is being saved, or when a micro-benchmark shows that something is called 1000s of times (e.g., in the inner loop of some common operation).

I don't see either being the case here, but it's possible I missed something. The method in question, getFixedCellSize() just calls ListView::getFixedCellSize which just returns the value of a property. I suspect that any performance hit would be down in the noise.

So I think the complexity of the existing mechanism isn't justified. Removing the listener as the PR proposes to do, which is possible since the listener isn't being used to trigger an operation, seems like a win as well.

I am inclined to approve this fix even without performance measurements, since it seems unlikely they would show a problem.

@arapte if you want to measure it further before you approve it, that would be fine.

Copy link
Member

@arapte arapte Sep 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The few additional computation would not cause any harm to performance. It just increases few computations in the compute* methods. Just for sanity verified a program with ListView of 10000000. ListView gets scrolled just as smooth.
Approving.

return fixedCellSize;
}

Expand All @@ -140,7 +110,8 @@ private void setupListeners() {

/** {@inheritDoc} */
@Override protected double computeMinHeight(double width, double topInset, double rightInset, double bottomInset, double leftInset) {
if (fixedCellSizeEnabled) {
double fixedCellSize = getFixedCellSize();
if (fixedCellSize > 0) {
return fixedCellSize;
}

Expand All @@ -149,10 +120,15 @@ private void setupListeners() {

/** {@inheritDoc} */
@Override protected double computeMaxHeight(double width, double topInset, double rightInset, double bottomInset, double leftInset) {
if (fixedCellSizeEnabled) {
double fixedCellSize = getFixedCellSize();
if (fixedCellSize > 0) {
return fixedCellSize;
}

return super.computeMaxHeight(width, topInset, rightInset, bottomInset, leftInset);
}

private double getFixedCellSize() {
ListView<?> listView = getSkinnable().getListView();
return listView != null ? listView.getFixedCellSize() : Region.USE_COMPUTED_SIZE;
}
}
25 changes: 25 additions & 0 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import org.junit.Ignore;
import org.junit.Test;

import static javafx.scene.control.ControlShim.*;
import static test.com.sun.javafx.scene.control.infrastructure.ControlTestUtils.*;
import static org.junit.Assert.*;

Expand Down Expand Up @@ -723,4 +724,28 @@ private final class FocusModelMock extends FocusModel {
ListCell cell = new ListCell();
cell.setSkin(new ListCellSkin(cell));
}

/**
* Test that min/max/pref height respect fixedCellSize.
* Sanity test when fixing JDK-8246745.
*/
@Test
public void testListCellHeights() {
ListCell<Object> cell = new ListCell<>();
ListView<Object> listView = new ListView<>();
cell.updateListView(listView);
installDefaultSkin(cell);
listView.setFixedCellSize(100);
assertEquals("pref height must be fixedCellSize",
listView.getFixedCellSize(),
cell.prefHeight(-1), 1);
assertEquals("min height must be fixedCellSize",
listView.getFixedCellSize(),
cell.minHeight(-1), 1);
assertEquals("max height must be fixedCellSize",
listView.getFixedCellSize(),
cell.maxHeight(-1), 1);
}


}
27 changes: 27 additions & 0 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import javafx.scene.control.Button;
import javafx.scene.control.ChoiceBox;
import javafx.scene.control.Control;
import javafx.scene.control.ListCell;
import javafx.scene.control.ListView;
import javafx.scene.control.ToolBar;
import javafx.scene.layout.Pane;
Expand All @@ -55,6 +56,32 @@ public class SkinCleanupTest {
private Stage stage;
private Pane root;

// ------------------ ListCell

@Test
public void testListCellReplaceListViewWithNull() {
ListCell<Object> cell = new ListCell<>();
ListView<Object> listView = new ListView<>();
cell.updateListView(listView);
installDefaultSkin(cell);
cell.updateListView(null);
// 8246745: updating the old listView must not throw NPE in skin
listView.setFixedCellSize(100);
}

@Test
public void testListCellPrefHeightOnReplaceListView() {
ListCell<Object> cell = new ListCell<>();
cell.updateListView(new ListView<>());
installDefaultSkin(cell);
ListView<Object> listView = new ListView<>();
listView.setFixedCellSize(100);
cell.updateListView(listView);
assertEquals("fixed cell set to value of new listView",
cell.getListView().getFixedCellSize(),
cell.prefHeight(-1), 1);
}

//-------------- listView

@Test
Expand Down
1 change: 0 additions & 1 deletion modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,6 @@ public static Collection<Object[]> data() {
ColorPicker.class,
ComboBox.class,
DatePicker.class,
ListCell.class,
MenuBar.class,
MenuButton.class,
Pagination.class,
Expand Down