Skip to content
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.

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

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

Closed
wants to merge 2 commits into from

Conversation

@kleopatra
Copy link
Collaborator

kleopatra commented Jun 10, 2020

ListCellSkin installs listeners to the ListView/fixedCellSize that introduce a memory leak, NPE on replacing the listView and incorrect update of internal state (see bug report for details)

Fixed by removing the listeners (and the internal state had been copied from listView on change) and access of listView state when needed.

Added tests that failed before and pass after the fix, plus a sanity test to guarantee same (correct) behavior before/after.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8246745: ListCell/Skin: misbehavior on switching skin

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jfx pull/251/head:pull/251
$ git checkout pull/251

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 10, 2020

👋 Welcome back fastegal! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Jun 10, 2020
@mlbridge
Copy link

mlbridge bot commented Jun 10, 2020

Webrevs

@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 10, 2020

@arapte can you review?

@@ -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) {

This comment has been minimized.

@arapte

arapte Jun 15, 2020 Member

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

This comment has been minimized.

@kleopatra

kleopatra Jun 15, 2020 Author Collaborator

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?

This comment has been minimized.

@kleopatra

kleopatra Jun 15, 2020 Author Collaborator

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

This comment has been minimized.

@arapte

arapte Jun 15, 2020 Member

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)

This comment has been minimized.

@kleopatra

kleopatra Jun 15, 2020 Author Collaborator

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

This comment has been minimized.

@arapte

arapte Jun 16, 2020 Member

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.

This comment has been minimized.

@kleopatra

kleopatra Jun 16, 2020 Author Collaborator

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)

This comment has been minimized.

@kevinrushforth

kevinrushforth Sep 10, 2020 Member

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.

This comment has been minimized.

@arapte

arapte Sep 11, 2020 Member

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.

@kleopatra
Copy link
Collaborator Author

kleopatra commented Jun 26, 2020

Hmm .. bottleneck seems to be layout as such (including css), accessing an instance field vs. querying a property doesn't make a difference (at least none I could see).

@kevinrushforth
Copy link
Member

kevinrushforth commented Jul 22, 2020

/reviewers 2

@kevinrushforth kevinrushforth self-requested a review Jul 22, 2020
@openjdk
Copy link

openjdk bot commented Jul 22, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

Copy link
Member

kevinrushforth left a comment

(catching up on some overdue reviews)

The fix and tests look fine to me. I left one minor suggestion to add a comment in one of the tests.

@@ -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) {

This comment has been minimized.

@kevinrushforth

kevinrushforth Sep 10, 2020 Member

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.

@arapte
arapte approved these changes Sep 11, 2020
@openjdk
Copy link

openjdk bot commented Sep 11, 2020

@kleopatra This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8246745: ListCell/Skin: misbehavior on switching skin

Reviewed-by: kcr, arapte
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 74 commits pushed to 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 automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 75065542aaf6fd8acdaf287e52ab146fc0a9f947.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Sep 11, 2020
@kleopatra
Copy link
Collaborator Author

kleopatra commented Sep 11, 2020

/integrate

@openjdk openjdk bot closed this Sep 11, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 11, 2020
@openjdk
Copy link

openjdk bot commented Sep 11, 2020

@kleopatra Since your change was applied there have been 74 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 0514116.

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

@kleopatra kleopatra deleted the kleopatra:bug-fix-8246745 branch Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.