-
Notifications
You must be signed in to change notification settings - Fork 542
8359599: Calling refresh() for all virtualized controls recreates all cells instead of refreshing the cells #1830
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
base: master
Are you sure you want to change the base?
Conversation
…stead of refreshing the cells
|
👋 Welcome back mhanl! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
|
This is a risky change and will need to be carefully tested. Reviewers: @andy-goryachev-oracle @johanvos /reviewers 2 reviewers |
|
@kevinrushforth |
|
It is indeed true that refresh() is often a very expensive operation. Whenever Hence, while I like the idea here (avoiding unneeded heavy-cost operations in VirtualFlow), I would like to avoid a number of follow-up issues once this is merged -- driven by a change in "expected" behavior. |
I completely agree. We need to be careful with such changes.
Since all rows (and cells) are reset and then updated, all changes that were not taken into account by the control are taken into account in any case then. But I think there is one concrete case which breaks now. ExampleHere, I'm aware that But that is still a valid risk that we need to consider. This is the only problem I see right now. |
|
@Maran23 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
My main concern with this PR is that we might get a minute performance improvement, while risking regression. Would it be possible to get some measurements using a modern system? What do you think? |
|
I think the performance improvements due to this PR can be pretty significant. The problem is indeed the risk on regression. I believe we need improvements in 2 areas before we can safely merge this:
|
|
I think a CSR is needed if we're changing the documentation. It specifically says it recreates the cells (although the purpose of that eludes me, aside from badly written cells). Since cells are supposed to be updated, and not "retain" anything from previous contents, I think only only buggy cells would benefit from recreating. Such buggy cells however would probably have subtle problems already when they're never recreated, like listener leaks. So although I agree that recreating is not needed as cell functionality is defined by their |
I agree. /csr needed |
|
@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request. @Maran23 please create a CSR request for issue JDK-8359599 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
|
@Maran23 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
@Maran23 This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
|
/open Will do the CSR + try to benchmark when I have more time! |
|
@Maran23 This pull request is now open |
…refresh-recreates-all
|
I did some benchmarks and attached them to the description. Results look very good and is expected from what I measured myself when I implemented that. Since we do not need to throw away all |
|
@Maran23 Not a developer but I'd just like to say thanks for this. For the longest time I thought it was because of lazy coding on my part(new I'd like also to point out that the terrible performance isn't just from having 100s of rows. Comparing the CPU usage difference between viewing many updating But what's even worse is the garbage allocation rate. Can this please get more attention? |
| /** TreeTableView.refresh() must release all discarded cells JDK-8307538 */ | ||
| @Test | ||
| public void cellsMustBeCollectableAfterRefresh() { | ||
| IndexedCell<?> row = VirtualFlowTestUtils.getCell(treeTableView, 0); | ||
| assertNotNull(row); | ||
| WeakReference<Object> ref = new WeakReference<>(row); | ||
| row = null; | ||
|
|
||
| treeTableView.refresh(); | ||
| Toolkit.getToolkit().firePulse(); | ||
|
|
||
| JMemoryBuddy.assertCollectable(ref); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there another test that verifies that cells are garbage collectable? For example, in the case where a table / list / tree table becomes smaller visually, I think that it should then perhaps discard some cells that then should be collectable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are some when switching the TableRow, as this should remove all old rows and gc them at one point.
Other than that, I think there is no case where we gc cells. When we change the viewport width/height, all rows (cells if a fixedCellSize is set) will be piled / cached, but not destroyed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, as long as there are still some GC checks I think this would be fine. Not sure if I agree with keeping cells/rows around that exceed the amount that are currently displayed, but that's how it already works (my own virtualized controls will only reference the cells displayed, but with non-fixed size cells/rows this may not be optimal).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the changes look good. I'm a bit confused in the performance table with what is meant with the 50 ms -> 0 ms in the "after" cases though?
Every @andy-goryachev-oracle solved that problem in the |
No need to address that in this PR, I was just confused what the numbers meant (shouldn't the As a side note, even 30-40 ms seems incredibly slow, that's bound to create noticeable input lag or frame skips :/ How many cells were visible? 1000 or 100x1000? If the latter, than 30-40 ms seems okayish. |
I agree. One problem here is, that all cells will be updated (via I counted all Code from Benchmark above:
39 rows are displayed, and they are updated twice (first with A |
|
What needs to happen for this to get merged? Testing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any obvious issues with the new code.
Performance wise, feels no different than the master branch. Using the monkey tester with 10,000 rows and 200 columns, both feel sluggish on vertical scrolling, which disappears once a fixedCellSize is set.
| * Calling {@code refresh()} forces the TableView control to recreate and | ||
| * repopulate the cells necessary to populate the visual bounds of the control. | ||
| * Calling {@code refresh()} forces the TableView control to repopulate the | ||
| * cells necessary to populate the visual bounds of the control. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the word 'repopulate' is used twice (here and elsewhere)
I would suggest to rephrase these comments to indicate what happens exactly, possibly borrowing text from the VirtualFlow:
- recreate: a layout pass should be done, and that the cell factory has changed. All cells in the viewport are recreated with the new cell factory.
- rebuild: a layout pass should be done, and cell contents have changed. All cells are removed and then added properly in the viewport
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a minimum, replace the first occurrence of "repopulate" with "rebuild".
* Calling {@code refresh()} forces the TableView control to rebuild the
* cells necessary to populate the visual bounds of the control.
I wouldn't over-specify this by saying what VirtualFlow will do, but if you want to add a sentence saying that this will request a layout that would be fine:
* Calling {@code refresh()} forces the TableView control to rebuild the
* cells necessary to populate the visual bounds of the control.
* This will request a layout of the TableView cells.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, though I would insist on actually explaining what "rebuild" means, as it is not clear from the context.
VirtualFlow offers more detailed explanation, so perhaps we should borrow that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe link to VirtualFlow? I'm not sure the details are important enough to repeat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I see what you are getting at now. Let's take a step back.
What we want is to give the app developer enough information to know what the purpose of calling this method is, and what the effect will be. What we don't want to do is say how that is done. So you are right that we shouldn't appeal to VirtualFlow since it isn't really relevant in this context. Likewise, we don't want to constrain it with implementation details.
The refresh() method was added by JDK-8098085 to allow an app developer with a custom cell to say, in effect, "even if it doesn't look like anything is changed, get the contents of the cells anyway". If that's what updateItem() does, then yes we can say that. Otherwise, we need to find some other way to say it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly, thank you @kevinrushforth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with Kevin yesterday, what do you think of this version:
* Calling {@code refresh()} forces the XXX to update what it is showing to
* the user. This is useful in cases where the underlying data source has
* changed in a way that is not observed by the XXX itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good. You might even simplify it further and remove Calling {@code refresh()}, since it's redundant, and just say Forces the XXX ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Maran23 Can you update the javadoc for the four methods as Andy suggested above (possibly with my simplification) and then update the CSR Specification section to match? I think that's the only remaining thing needed to move this forward.
| } else if (Properties.RECREATE.equals(c.getKey())) { | ||
| needCellsRecreated = true; | ||
| refreshView(); | ||
| if (Properties.RECREATE.equals(c.getKey())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have to raise this concern.
It looks weird to me to use properties as a roundabout way to invoke a hidden method in the skin. Node properties, a public facility, can be easily mutated by unrelated code, making it easy to break the intended functionality.
Why not make these two methods explicitly a part of the public API by replacing requestRebuildCells and requestRecreateCells with
protected VirtualContainerBase.rebuildCells()
protected VirtualContainerBase.recreateCells()
?
(requestXXX is a misnomer - it might suggest an async nature, whereas it these are synchronous methods)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing this is out of scope for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having said that, I've always disliked using user properties for this sort of thing. It feels like a bit of a hack, so it might be worth filing a follow-up to evaluate a different approach in general (not just for this one instance of the pattern). I don't see this as a high priority, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding to what @kevinrushforth said, this is used everywhere. All layout constraints are implemented by using Properties. Changing anything there can break the layout.
If we just talking about the properties calling a method in the skin, I dislike the current way as well, but have no other idea really. Maybe a good topic for the mailing list then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one comment on the docs. I'll leave it to the other reviewers to test and review the implementation changes.
| * Calling {@code refresh()} forces the TableView control to recreate and | ||
| * repopulate the cells necessary to populate the visual bounds of the control. | ||
| * Calling {@code refresh()} forces the TableView control to repopulate the | ||
| * cells necessary to populate the visual bounds of the control. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a minimum, replace the first occurrence of "repopulate" with "rebuild".
* Calling {@code refresh()} forces the TableView control to rebuild the
* cells necessary to populate the visual bounds of the control.
I wouldn't over-specify this by saying what VirtualFlow will do, but if you want to add a sentence saying that this will request a layout that would be fine:
* Calling {@code refresh()} forces the TableView control to rebuild the
* cells necessary to populate the visual bounds of the control.
* This will request a layout of the TableView cells.
Sure. I don't know how easy it is, but it might be worth to consider using |
This is something that worries me. One of the main issues I see with e.g. VirtualFlow, is that some methods can be invoked both during a rendering pulse as well as (indirectly) via explicit invocations, most often by code invoked with Platform.runLater(). I am not saying that this PR makes the situation worse or better, but I think there is a reasonable chance that some applications will behave differently (and show flickering). Having said that, I don't think that this PR can be blamed in case there is a different behavior. |
I'm also not happy with the current situation. There are also some cases where cells request a layout while they are layouted. There is quite some room to optimize several scenarios here. |
|
There seems to be an intermediate test failure, maybe a race condition unrelated to this change: |
Yes, it is already filed and tracked by JDK-8357459. |
When calling
refresh()on virtualized Controls (ListView,TreeView,TableView,TreeTableView), all cells will be recreated completely, instead of just refreshing them.This is because
recreateCells()of theVirtualFlowis called whenrefresh()was called. This is not needed, since refreshing the cells can be done much cheaper withrebuildCells().This will reset all cells (
index = -1), add them to the pile and fill them back in the viewport with an index again. This ensuresupdateItem()is called.The contract of
refresh()is also a big vague, stating:As written above, recreating is not needed in order to fulfull the contract of updating what is shown to the user in case the underlying data source changed without JavaFX noticing (e.g. calling a normal Setter without any Property and therefore listener involved).
Benchmarks
Setup:
TableView100 TableColumns1000 ItemsCalling
refresh()with aButtonpress.Benchmark code
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1830/head:pull/1830$ git checkout pull/1830Update a local copy of the PR:
$ git checkout pull/1830$ git pull https://git.openjdk.org/jfx.git pull/1830/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1830View PR using the GUI difftool:
$ git pr show -t 1830Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1830.diff
Using Webrev
Link to Webrev Comment