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

[graphics] fix CA1307 and CA1309 for performance #14627

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

jonathanpeppers
Copy link
Member

Context: #12130
Context: https://github.com/angelru/CvSlowJittering

While reviewing the above sample, I saw time spent doing culture-aware string "stuff" in Microsoft.Maui.Graphics:

77.22ms microsoft.maui!Microsoft.Maui.Graphics.MauiDrawable.SetDefaultBackgroundColor()
42.55ms System.Private.CoreLib!System.String.ToLower()

In c40c6e7, I added code analysis rules to the .editorconfig, but it appears we ignored these warnings in dbaeee9.

These are generally easy to fix, we should have just addressed these instead of adding $(NoWarn).

In the future, I will look into adding CA1311:

https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1311

This would find string.ToLower() calls and recommend string.ToLowerInvariant() instead. I fixed the one place I saw it affecting performance in dotnet-trace.

Context: dotnet#12130
Context: https://github.com/angelru/CvSlowJittering

While reviewing the above sample, I saw time spent doing culture-aware
string "stuff" in Microsoft.Maui.Graphics:

    77.22ms microsoft.maui!Microsoft.Maui.Graphics.MauiDrawable.SetDefaultBackgroundColor()
    42.55ms System.Private.CoreLib!System.String.ToLower()

In c40c6e7, I added code analysis rules to the `.editorconfig`, but
it appears we ignored these warnings in dbaeee9.

These are generally easy to fix, we should have just addressed these
instead of adding `$(NoWarn)`.

In the future, I will look into adding `CA1311`:

https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1311

This would find `string.ToLower()` calls and recommend
`string.ToLowerInvariant()` instead. I fixed the one place I saw it
affecting performance in `dotnet-trace`.
bool encode = attributedText.Text.Contains("]]");
bool encode = attributedText.Text.IndexOf("]]", StringComparison.Ordinal) != -1;
Copy link
Member Author

Choose a reason for hiding this comment

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

Switching to IndexOf() for this one felt better than having to do #if NETSTANDARD2_0.

@jonathanpeppers jonathanpeppers marked this pull request as ready for review April 18, 2023 03:10
@jonathanpeppers jonathanpeppers added legacy-area-perf Startup / Runtime performance area-drawing Shapes, Borders, Shadows, Graphics, BoxView, custom drawing labels Apr 18, 2023
@mattleibow mattleibow merged commit 8a58e01 into dotnet:main Apr 18, 2023
@jonathanpeppers jonathanpeppers deleted the GraphicsCA1307CA1309 branch April 18, 2023 13:43
@github-actions github-actions bot locked and limited conversation to collaborators Dec 11, 2023
@Eilon Eilon added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) and removed legacy-area-perf Startup / Runtime performance labels May 10, 2024
@samhouts samhouts added the fixed-in-8.0.0-preview.4.8333 Look for this fix in 8.0.0-preview.4.8333! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-drawing Shapes, Borders, Shadows, Graphics, BoxView, custom drawing fixed-in-8.0.0-preview.4.8333 Look for this fix in 8.0.0-preview.4.8333! t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants