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

FindCommandBinding improvements #5693

Merged
merged 2 commits into from
Jan 3, 2022
Merged

Conversation

deeprobin
Copy link
Contributor

@deeprobin deeprobin commented Nov 14, 2021

Might fix Issue #4869

Description

The main focus was on the FindCommandBinding method in CommandManager.cs. Here I decided to use a ValueTuple instead of a Tuple as well as to do the check using a switch-is block, as this is most likely easier to optimize through the JIT.

Furthermore, I made other code improvements in related methods, like reducing code-nesting and using new C# features.

I don't think these make a big difference to the generated JIT asm, but still hope to have made the code a bit more readable.

/cc @IAmTheCShark

@deeprobin deeprobin requested a review from a team as a code owner November 14, 2021 16:00
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Nov 14, 2021
@ghost ghost requested review from fabiant3, ryalanms and SamBent November 14, 2021 16:00
@deeprobin deeprobin changed the title Issue 4869 FindCommandBinding improvements Nov 14, 2021
@IAmTheCShark
Copy link

IAmTheCShark commented Nov 14, 2021

I do not believe this fixes #4869 because the algorithm itself is quite inefficient to my understanding.
Looking at your changes those are mosly refactorings in favour of new language features. Or did I miss something?

@deeprobin
Copy link
Contributor Author

I do not believe this fixes #4869 because the algorithm itself is quite inefficient to my understanding. Looking at your changes those are mosly refactorings in favour of new language features. Or did I miss something?

The usage of ValueTuples instead of Tuples might improve the performance a little bit. But I don't know how to benchmark WPF Code because BenchmarkDotNet does not work with this workset

@lindexi
Copy link
Member

lindexi commented Nov 15, 2021

BenchmarkDotNet

See dotnet/BenchmarkDotNet#1616

@deeprobin
Copy link
Contributor Author

@anjalisheel-wpf What is the current status on this?

@anjali-wpf
Copy link
Member

@deeprobin we ran into .NET 7 migration issues. So, taking time to merge community test pass this month.

@dipeshmsft dipeshmsft merged commit fdc7654 into dotnet:main Jan 3, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Apr 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants