8000 Fix ListView/GridView scroll performance when automation peers are created by h3xds1nz · Pull Request #9182 · dotnet/wpf · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix ListView/GridView scroll performance when automation peers are created #9182

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

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

h3xds1nz
Copy link
Member
@h3xds1nz h3xds1nz commented May 30, 2024

Fixes #9181

Description

I was running some tests with ListView (using GridView) and when I loaded 1 000 000 items (yes, an obscure use case but that's why we got virtualization, no?), the scrolling performance would degrade and keyboard scrolling felt almost unusable.

During a quick rundown, I've discovered the culprit is in override of GetChildrenCore of GridViewItemAutomationPeer, which performs IndexOf (linear) search in the Items collection of the underlying ItemsSource to find the respective item's Row.

Reproduction Steps

The easiest step to reproduce is to add a ListView with GridView plus a ComboBox, after opening the ComboBox's dropdown (clicking it), Accessibility.dll gets loaded and the party gets started. You can find a demo repository on Github - here.

Behavior

The scrolling performance becomes worse to unusable with increasing number of items.

Known Workarounds

The "workaround" is to subclass GridView, overriding GetAutomationPeer to return NULL. Not really a solution though.

Risk

None.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested a review from a team as a code owner May 30, 2024 18:34
@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels May 30, 2024
@h3xds1nz
Copy link
Member Author

@dotnet-policy-service agree

@miloush
Copy link
Contributor
miloush commented May 30, 2024

Tested?

Code changes look good to me. Would it make sense to have a reference to something holding the cached row number so that it doesn't have to be searched for each cell in a row?

@h3xds1nz h3xds1nz force-pushed the fix-gridview-scroll-performance branch from e6be461 to 1c5f446 Compare May 30, 2024 18:47
@h3xds1nz
Copy link
Member Author
h3xds1nz commented May 30, 2024

@miloush Yeah, tested, it works as expected.

It would definitely make more sense to cache the row index for all the cells, however I don't see an easy way how to do it here, given that we would load the index lazily.

@h3xds1nz
Copy link
Member Author

@miloush I have actually thought of a much simpler solution that didn't occur to me before, we can just retrieve the index from the realized item in ItemGenerator. That way the change is pretty much a one-liner.

I have also thought about your caching proposal, and my original solution does actually ruin the UAI performance with multiple columns indeed. This solution on the other hand is rather smooth.

@miloush
Copy link
Contributor
miloush commented May 30, 2024

For one liner I think it would appreciated if the PR didn't contain all the other stylistic changes.

8000 @miloush
Copy link
Contributor
miloush commented May 30, 2024

Doesn't the IndexFromContainer do the same linear search?

@h3xds1nz
Copy link
Member Author
h3xds1nz commented May 30, 2024

Agreed, I'll push a commit with only the one-liner.

No, ItemGenerator contains always just the realized items (block) and it does a linear search but only in the bunch, so in case of virtualization that's only a handful few that are currently realized (it has a starting index). I use it a lot in other projects to draw over controls so I'm aware of the performance. I ran it with the build of the new PresentationFramework as well just to be sure before committing.

@Wil-fried
Copy link
Wil-fried commented Jul 1, 2024

Hello, i'm following this conversation since the start because i ran into this issue.
I was very glad when @h3xds1nz came up with a solution and seeing the progress on the reviews.
Now the downside, there is not much activity since 30th may. Is there a way upvote / make the owner aware of this issue please?
When "Accessibility.dll" load it greatly impact the datagrid /listview negatively and it doesn't require 1 000 000 items to notice it. In my usecase, it happen with100

@pchaurasia14
Copy link
Member

Thanks @h3xds1nz.
We will include this PR in our upcoming test pass.

@h3xds1nz
Copy link
Member Author
h3xds1nz commented Jul 3, 2024

@pchaurasia14 Thank you.

@singhashish-wpf
Copy link
Member

@h3xds1nz I was trying to check your sample repro by replacing the locally built PresentationFramework.dll and could experience similar scrolling performance. Couldn't observe something noticeable. I might be missing a step here, Could you please share the exact steps to verify?

@h3xds1nz
Copy link
Member Author
h3xds1nz commented Jul 19, 2024

@singhashish-wpf Thanks for checking, I've included sample videos for you with the repro that I've originally recorded. First I'm regularly scrolling when peers are not being created.

Notice what happens in the first/original video when Accessibility gets loaded (VS output) - e.g. clicking dropdown, and automation peers start being created. The scrolling up that takes like 250ms for each item (because for each item in the view it does linear search (IndexOf) in the entire Items collection) is me actually holding Up arrow key.

Same goes for the scrollbar being laggy after I reach the bottom.

By the way, the fact that automation peers are created after interacting in with dropdowns, popups, even though there's no client subbed, that's a long-standing issue since the beginning, I believe there's a tracking for this as well in this repo but that's just a side note.

@h3xds1nz h3xds1nz force-pushed the fix-gridview-scroll-performance branch from ce98a71 to d212d66 Compare July 20, 2024 19:41
@h3xds1nz h3xds1nz requested a review from a team as a code owner July 20, 2024 19:41
@h3xds1nz
Copy link
Member Author

I have rebased and cleaned up the commit history.

@harshit7962 harshit7962 merged commit cf5a4f4 into dotnet:main Sep 6, 2024
8 checks passed
@harshit7962
Copy link
Member

@h3xds1nz Thank you for your contributions.

@h3xds1nz
Copy link
Member Author
h3xds1nz commented Sep 6, 2024

@harshit7962 Thanks for including it, finally can use UIA again in the upcoming project.

@github-actions github-actions bot locked and limited conversation to collaborators Oct 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ListView/GridView scroll performance is O(n) when automation peers are created
8 participants
0