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

Optimize GetVisualTreeElementsWindowsInternal #19984

Merged
merged 2 commits into from
Feb 12, 2024

Conversation

symbiogenesis
Copy link
Contributor

@symbiogenesis symbiogenesis commented Jan 18, 2024

There were multiple enumerations occurring in the Windows implementation of GetVisualTreeElements.

Using a HashSet, in particular, for the Contains() check within the other enumeration should especially help speed things up. And adding everything in reverse order in a single enumeration.

@symbiogenesis symbiogenesis requested a review from a team as a code owner January 18, 2024 19:37
@ghost ghost added the community ✨ Community Contribution label Jan 18, 2024
@ghost
Copy link

ghost commented Jan 18, 2024

Hey there @symbiogenesis! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@symbiogenesis symbiogenesis force-pushed the windows-visual-tree-perf branch 2 times, most recently from 784e5cc to 9ba2780 Compare January 18, 2024 19:47
@mattleibow
Copy link
Member

Is it possible to add a benchmark for this? In particular, the allocations may be interesting - or maybe no.

@mattleibow
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@symbiogenesis symbiogenesis force-pushed the windows-visual-tree-perf branch 2 times, most recently from 1999fb2 to e6ddfeb Compare January 19, 2024 12:34
@symbiogenesis symbiogenesis force-pushed the windows-visual-tree-perf branch 2 times, most recently from 447d33e to 529a500 Compare January 19, 2024 13:05
@symbiogenesis
Copy link
Contributor Author

symbiogenesis commented Jan 19, 2024

Benchmarks for latest version that just uses a for loop:

Before:

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
GetVisualTreeElements 134.6 ms 2.64 ms 2.60 ms 6750.0000 2000.0000 250.0000 58.46 MB

After:

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
GetVisualTreeElements 123.2 ms 2.32 ms 2.27 ms 6800.0000 1800.0000 400.0000 58.46 MB

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

Ok, the new code does look much better (performance-wise) to me!

I'll lean on the MAUI team's knowledge, if we have enough tests to know this doesn't break anything.

@mattleibow mattleibow enabled auto-merge (squash) January 19, 2024 19:26
@symbiogenesis
Copy link
Contributor Author

Maybe retry the failed jobs?

@samhouts samhouts added the legacy-area-perf Startup / Runtime performance label Jan 25, 2024
@rmarinho rmarinho merged commit 49be7e3 into dotnet:main Feb 12, 2024
42 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 15, 2024
@Eilon Eilon added the t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) label May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community ✨ Community Contribution fixed-in-8.0.10 fixed-in-9.0.0-preview.2.10247 fixed-in-9.0.0-preview.2.10293 legacy-area-perf Startup / Runtime performance t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants