Add a buffer cache to containers in VirtualizingStackPanel #18646
Conversation
timunie
added
needs-api-review
|
First of all, thank you for your contribution! I haven't looked at the code at all yet, but there's a bunch of failing tests after this PR: https://dev.azure.com/AvaloniaUI/AvaloniaUI/_build/results?buildId=56039&view=ms.vss-test-web.build-test-results-tab. Some existing tests might need adjustments while others look like real failures that should be addressed. Moreover, the new feature should come with its own unit tests validating the new expected behaviors regarding realization, scrolling and measuring. |
|
|
@MrJul you are right. Could you please assist me in fixing one more test? [Fact]
public void Recycling_A_Hidden_Control_Shows_It()
{
using var app = App();
var style = CreateIsVisibleBindingStyle();
var items = Enumerable.Range(0, 3).Select(x => new ItemWithIsVisible(x)).ToList();
var (target, scroll, itemsControl) = CreateTarget(items: items, styles: new[] { style });
var container = target.ContainerFromIndex(2)!;
Assert.True(container.IsVisible);
Assert.Equal(20, container.Bounds.Top);
items[2].IsVisible = false;
Layout(target);
Assert.False(container.IsVisible);
items.RemoveAt(2);
items.Add(new ItemWithIsVisible(3));
Layout(target); //THIS DOES NOT CALL MEASUREOVERRIDE ANYMORE!
Assert.True(container.IsVisible);
}For some reason, the But I also do not understand the test. Would it be feasible if I changed this to be an [Fact]
public void Recycling_A_Hidden_Control_Shows_It()
{
using var app = App();
var style = CreateIsVisibleBindingStyle();
var itemsList = Enumerable.Range(0, 3).Select(x => new ItemWithIsVisible(x)).ToList();
var items = new ObservableCollection<ItemWithIsVisible>(itemsList);
var (target, scroll, itemsControl) = CreateTarget(items: items, styles: new[] { style });
var container = target.ContainerFromIndex(2)!;
Assert.True(container.IsVisible);
Assert.Equal(20, container.Bounds.Top);
items[2].IsVisible = false;
Layout(target);
Assert.False(container.IsVisible);
items.RemoveAt(2);
items.Add(new ItemWithIsVisible(3));
Layout(target);
Assert.True(container.IsVisible);
}
I will add the tests for the buffer today then! |
|
You can test this PR using the following package version. |
|
@MrJul please wait with review - I am still adding specialized tests for the bufferfactor! |
gentledepp
force-pushed
the
feature/18626_virtualizingstackpanelperf2
branch
from
b8b1506 to
29d4dd6
Compare
|
ok... so I added a bunch of tests. VirtualizingStackPanelTests.Buffered.cs
I guess I've got everything covered to be honest. The only thing that bothers me still is this test case: |
|
@cla-avalonia agree |
|
You can test this PR using the following package version. |
|
@MrJul some test are failing, but it seems to be an appium issue (not related to my changes - i guess?)
Please advise |
|
If you look at my original PR description now, you'll see I added two simple animations which (I hope) show how my PR works and what improvements it can bring |
|
Thank you for adding the new tests.
Yes, Appium sometimes just randomly crashes while running integration tests. |
|
Are there any plans for reviewing this? Alternatively, is there any guide on how to create your own nuget package of avalonia locally? |
|
Hi @gentledepp - I definitely plan to review this, the only problem is a lack of time - sorry. Thank you for the great PR writeup, it should make reviewing it much easier! |
|
Hi @gentledepp - I hope to review this today.
Yes, you can create local nuget packages quite easily using the |
|
You can test this PR using the following package version. |
There was a problem hiding this comment.
Hi @gentledepp!
First of all sorry that it took me so long to review, you caught me at a time where I was busy with some other stuff and I'm the only one who really knows this part of the codebase well enough to give a review.
Secondly, thank you so much for the fantastic PR description - it's one of the best I've ever seen and was a joy to read ;)
Thirdy, thank you for the code - it's a really useful feature. I've tried to dig into the code as best I can and I think I understand what it's doing reasonably well now. The only real concerns I have about this PR relate to the duplication of tests and whether _unchangedElements is really needed. The rest of the comments are more nits, and I ask forgiveness for these; I'd be quite happy to tidy these up myself before merge (let me know) ;)
tests/Avalonia.Controls.UnitTests/VirtualizingStackPanelTests.Buffered.cs
Outdated
Show resolved
Hide resolved
|
|
||
| return CalculateDesiredSize(orientation, items.Count, viewport); | ||
| } | ||
| finally | ||
| { | ||
| _cacheElementMeasurements = false; |
There was a problem hiding this comment.
Took me a little which to understand why _cacheElementMeasurements is reset here, but if I remove this line, then Focused_Container_Is_Positioned_Correctly_when_Container_Size_Change_Causes_It_To_Be_Moved_Into_Visible_Viewport fails, so I think that the reason for it is that it's only set to true after a viewport change due to scrolling, and if a measure is invoked for any other reason then it needs to be false? If so might be worth adding a quick comment here to explain it (as the rest of the code is so well commented).
There was a problem hiding this comment.
yeah that is exactly the idea.
My problem was, that I wanted to make scrolling fast in a virtualizingstackpanel (as fast as it is in a normal stackpanel) so I tried caching as much as possible.
Doesn't a call to "InvalidateMeasure" invalidate all measurements down the visual tree? At least I thought it did.
One reason why the normal "VirtualizingStackPanel" is so slow (for complex item layouts) is that it measures everything anytime a new item is shown.
So I assume the caching you are talking about is at least not working? (if it is, I am happy to remove that code and the complexity that comes with it)
But: I'm glad you ran into my code coverage when trying to remove this ;-)
tests/Avalonia.Controls.UnitTests/VirtualizingStackPanelTests.Buffered.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// Defines the <see cref="BufferFactor"/> property. | ||
| /// </summary> | ||
| public static readonly StyledProperty<double> BufferFactorProperty = |
There was a problem hiding this comment.
One thing to consider: WinUI/UWP has a similar concept as this in ItemsRepeater and the properties which control this behavior there are called HorizontalCacheLength and VerticalCacheLength.
As VirtualizingStackPanel can only virtualize on one axis, we don't need two properties but we should consider whether we want to adopt similar naming? We'll do an API review on this at the end anyway.
There was a problem hiding this comment.
I do not have a preference on the naming.
I thing you guys are the owners and should therefore tell me how to name stuff.
That being said: I wanted to use a relative value, therefore called "factor"
Length sounds like a fixed pixel length that is used for caching.
So, we could call it VirtualizationFactor or CachingFactor?
There was a problem hiding this comment.
Not sure just yet - we probably need to do a quick API review with the core team.
gentledepp
force-pushed
the
feature/18626_virtualizingstackpanelperf2
branch
2 times, most recently
from
c1d8ee5 to
882fb17
Compare
|
You can test this PR using the following package version. |
grokys
changed the title
|
@grokys I incorporated your feedback.
|
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
We are going to have next api review early next month most at earliest. |
gentledepp
force-pushed
the
feature/18626_virtualizingstackpanelperf2
branch
from
6c8184c to
6432902
Compare
|
You can test this PR using the following package version. |
gentledepp
force-pushed
the
feature/18626_virtualizingstackpanelperf2
branch
from
6432902 to
d2ae1c6
Compare
|
You can test this PR using the following package version. |
|
Hi @gentledepp - we did a quick mini API review and the best name we could collectively think of was
What do you think? If you'd like a second opinion we can wait until @MrJul gets back from his vacation and do one of our proper reviews. |
|
I totally disagree.
My implementation, on the other hand, uses a factor to relatively calculate the area of additionally cached items. Why would you even keep a consistent naming? I guess: To make it easier to developers coming from WinUI and UWP to get used to Avalonia. Should I be, however, wrong and the That being said: It is your framework - so you decide. ;-) |
They are factors unless I'm mistaken?
|
gentledepp
force-pushed
the
feature/18626_virtualizingstackpanelperf2
branch
from
d2ae1c6 to
aa418f5
Compare
|
thanks for clarifying... a terrible name still in my humble opinion, but it means the names should align. |
|
You can test this PR using the following package version. |
MrJul
added
api-approved
|
@grokys has been a while already. Any plans to merge this PR? 😅 |
|
@gentledepp I noticed the code dealing with “UnchangedElements” to skip measuring for items that were previously visible was removed, was this not needed in the end? |
|
Yes it was not needed. I assumed that since the ItemsControl re-measures and arranges all its visible children when the scroll offset changes, that this might cause a performance impact. However, I was told (and I also re-checked) by the makers of avalonia, that those items cache their measurements in case the available size does not change (which it doesn't when you scroll) |
|
@gentledepp is my understanding correct that the performance improvement here is because previously it would do recycling as each item moved passed the viewport, but now it doesnt even have to do any recycling until it scrolls 25% of the items (if 25% were before and 25% after) meaning its doing the recycling in say chunks of say 10 items at once. meaning that you have to run that code less often. (Essentially it ends up doing the recycling in small could you call them pages?) I understand the measure is cached… but why cant the non-visible element arrange be skipped? BTW im asking these questions with a view to port your optimizations over to TreeDataGrid, its code is more or less based on this same code too. |
yes it is.
Interesting question. Would that make a huge difference? |
|
I have two points I'd like to raise. Caching strategiesI implemented caching in a panel I created for my product. But it doesn't work like this: Instead it continues to realise controls one-by-one. It just does so outside the viewport area. This is because each time an item is realised in this panel we start asynchronously loading data from an external source (either a web service or a local cache), and this always takes at least one frame to complete, often many more. With the caching strategy I implemented we can generally avoid the user seeing items before they have finished their async loading process. The strategy demonstrated in the image above is is of little use for the problem my product needs to solve because the cache "buffer" frequently runs out. Then the user starts seeing items before they have fully loaded. You can reduce how frequently the buffer runs out by increasing the cache length, but this is costly as more and more data need to be loaded as the cache lengthens, and it will still hit zero every so often. Is it not enough to avoid re-measuring controls that weren't created/recycled? Do we really reduce overhead within each individual measure call by executing more of them in the same layout pass? Cache Length UnitWPF lets us configure the unit of cache length. You can use |
|
@TomEdwardsEnscape re: your comments: Caching strategiesI think you meant to share a screenshot but nothing is showing up. Anyway I think what you're describing addresses a different use-case. This PR is meant to solve the common problem of scrolling stuttering in a fully-loaded list of items.
I believe the problem is not re-measuring of controls that weren't created/recycled, and instead the fact that every e.g. 25 pixels, a new container is realised, which triggers a layout pass. From a previous comment from the author: "Especially on Android I noted that every measure/arrange causes a lot of GC activity which in the end kills the scrolling performance." Cache Length UnitWhat do you mean by logical scrolling here? This feature is implemented on |
|
@TomEdwardsEnscape this sound a bit confusing to me.
am I missing something? 😅 Anyways, this is about something completely different as @grokys pointed out. 😅 |
So initially, ma plan was to cache any measure arrange calls on all the items that have not been changed (please look at the animated gif in the description) @grokys pointed out, that caching "Measure" calls makes no sense, since the visuals themselves cache their measurements in case the size provided does not change. Still, I re-added the tests to verify that, so that we cannot accidentally change the |
gentledepp
force-pushed
the
feature/18626_virtualizingstackpanelperf2
branch
from
aa418f5 to
d00dd90
Compare
…kPanel.cs by reducing Measure/Arrange calls since they cause heavy GC pressure on constrained devices (Android, iOS) especially with complex item views
gentledepp
force-pushed
the
feature/18626_virtualizingstackpanelperf2
branch
from
d00dd90 to
ae456a1
Compare
|
You can test this PR using the following package version. |
This defeats the purpose of virtualisation. We don't want to download 50 query response pages and 5000 thumbnails from the internet before we display anything to the user, any more than we want to immediately generate 5000 containers. It seems from your other comments that "not measuring" the other containers isn't actually doing anything, so the performance gains do indeed come entirely from batching up container generation. I don't see a way to unify behaviour to satisfy both sets of requirements given this. We'll just have to use different panels for the different scenarios.
Logical scrolling is just an example of a scenario in which caching by item becomes more important. But it's always useful IMO. |
What does the pull request do?
Uses 3 strategies to optimize scrolling performance of a VirtualizedStackPanel
Instead of measuring and arranging every element for every bit of scrolling as depicted here:
We keep additional elements in memory and only measure and arrange when we reach the extended viewport
Also, we only measure and arrange elements that have been changed!
The downside of this approach is, that we use more memory (as we need to keep additional items in memory)
The upside is, that re greatly reduce the number of measure & arrange cycles which reduces the stress on the GC
What is the current behavior?
Scrolling on Android is very edgy (non-smooth) since many Measure/Arrange calls put a lot of pressure on the Garbage Collector:
What is the updated/expected behavior with this PR?
Smooth scrolling on all platforms, even with more complex/deeper nested list item layouts.
How was the solution implemented (if it's not obvious)?
The default implementation of the
VirtualizingStackPanelonly keeps enough visual elements in memory to fill its current bounds. This uses the least amount of memory.On devices such as Android, however, it leads to rough/edgy (non-smooth) scrolling.
This is because for every bit of scrolling, all elements of the
VirtualizingStackPanelneed to be measured and arranged, which causes extensive GC/memory pressure on Android.The approach implemented here tries to keep the number of Measure/Arrange cycles as low as possible.
This is achieved by applying two strategies:
1 - Buffer more items (the first commit of this PR)
In order to not have to recycle elements every time the user scrolls "a bit", we extended the viewport by a
BufferFactor.By default this is 0.5 - so if the
VirtualizingStackPanelscrolls vertically and can show 800 px, we do not only render elements for 800 px, but also 400 (800*BufferFactor) above and belowOnly when reaching the edge of these additional elements, we detect and call
InvalidateMeasure.This causes more memory to be used (since twice as many UI elements are kept in memory), but greatly reduces the number of
Measurecalls.The second commit of this PR handles a corner case:
When we scroll near the end or start of the list, the
VirtualizingStackPanelkeeps trying to fill the available space.Since no more element is there to be rendered, however, this space cannot be filled. Not being aware of this causes the panel to call
InvalidateMeasureon every scroll (towards the start and end of the list)You can see this in the log output below since the last 4 logs call
InvalidateMeasureAfter implementing the strategy for this edge case, it looks like this:
You can see that
InvalidateMeasurewas called and then the effective viewport changed 7 consecutive times without anyInvalidateMeasurecall.This is because the first call detected that it is a t the start of the list and re-measuring would have no effect at all.
2 - Avoiding measuring realized elements when only the scroll position changed
The third commit of this PR implements another optimization:
When we only scroll, it makes no sense to call
Measureon any elements that will be kept in the viewport since they did not change at all.Therefore we try to detect
...and in this case skipt the
Measurecall.NOTE as I am not a 100% "Avalonia native developer", please check this code carefully since I do not know if all edge-cases are handled correctly!!
However, when implemented, the output can look something like:
Finally I thought: Why not also cache the "arranged" elements if we only scroll?
So I added that too. ;-)
NOTE I also introduced a readonly variable
private readonly bool _traceVirtualizationIf you set it to
trueyou get the same debug output as above which will help you understand the implementation.Checklist
BufferFactorneeds to be documented and I'd like to have the implementation and naming accepted firstBreaking changes
None - we can set the default
BufferFactorto0and the implementation will essentially be the same as before this PR.Obsoletions / Deprecations
Fixed issues
Implements #18626