Skip to content

Remove KnownColors static constructor #9661

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

Conversation

ThomasGoulet73
Copy link
Contributor

@ThomasGoulet73 ThomasGoulet73 commented Aug 28, 2024

Description

Removes KnownColors static constructor and replaces it with uint parsing and calling Enum.IsDefined. The static constructor is called at the startup of every WPF app AFAIK (It is called even with an app created from the empty WPF template). The size of the dictionary at runtime is about 10 Kb so not much but it is still unnecessary used memory.

Benchmark results of KnownColors.ArgbStringToKnownColor
Method argbString Mean Error StdDev Ratio RatioSD Allocated Alloc Ratio
Old #FFDA70D6 15.2223 ns 0.2156 ns 0.2017 ns 1.00 0.02 - NA
New #FFDA70D6 13.6033 ns 0.0437 ns 0.0365 ns 0.89 0.01 - NA
Old #XYZ 9.1676 ns 0.0399 ns 0.0353 ns 1.00 0.01 - NA
New #XYZ 5.4110 ns 0.0116 ns 0.0097 ns 0.59 0.00 - NA
Old FFDA70D6 11.5086 ns 0.0509 ns 0.0476 ns 1.00 0.01 - NA
New FFDA70D6 0.8499 ns 0.0034 ns 0.0028 ns 0.07 0.00 - NA

Customer Impact

Better perf, better startup perf and lower static memory.

Regression

No.

Testing

Local testing.

Risk

Low.

Microsoft Reviewers: Open in CodeFlow

@ThomasGoulet73 ThomasGoulet73 requested review from a team as code owners August 28, 2024 23:28
@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 Aug 28, 2024
@h3xds1nz
Copy link
Member

h3xds1nz commented Aug 29, 2024

Faster solution can be achieved by using AlternateLookup for trimmed string and actually using StringComparer.OrdinalIgnoreCase for the dictionary itself instead of uppering it.

But that ~10 kB per app, the creation time of the dictionary, vs. 2ns gained per compare on a function that's only called during serialization; this is clearly a better solution for the case, so yeah, once again, well done, Thomas.

@ThomasGoulet73
Copy link
Contributor Author

Faster solution can be achieved by using AlternateLookup for trimmed string and actually using StringComparer.OrdinalIgnoreCase for the dictionary itself instead of uppering it.

But that ~10 kB per app, the creation time of the dictionary, vs. 2ns gained per compare on a function that's only called during serialization; this is clearly a better solution for the case, so yeah, once again, well done, Thomas.

Yes I initially had the same thought but while working on it I found that, when comparing my current changes vs doing a more optimized dictionary lookup, my current changes were cleaner, faster (I didn't see the 2ns gain you mention) and used less memory .

But as you said, even if there was a 2ns gain by doing a more optimized lookup I think we should go with my current changes since it's cleaner and it removes the need for a 10 Kb dictionary.

@ThomasGoulet73 ThomasGoulet73 force-pushed the remove-KnownColors-static-constructor branch from 67666ab to 29c9b42 Compare September 14, 2024 06:28
@ThomasGoulet73
Copy link
Contributor Author

I've rebased on main to add a unit test that validates my changes.

I also pushed a change to "fix" the method when argbValue is null. In reality the method is not reachable when argbValue is null because it would throw before the method is called but I wanted to keep parity with main. I only changed the thrown exception from NullReferenceException on main to ArgumentNullException with my changes. But, as I said, it's not reachable at runtime, it only affects the new unit test.

@ThomasGoulet73 ThomasGoulet73 force-pushed the remove-KnownColors-static-constructor branch from 59ac317 to 59604ea Compare January 23, 2025 23:56
@ThomasGoulet73
Copy link
Contributor Author

I rebased on main to fix the conflicts.

@dipeshmsft dipeshmsft merged commit 1f21aef into dotnet:main May 16, 2025
8 checks passed
@dipeshmsft
Copy link
Member

Thanks @ThomasGoulet73, good contribution.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 15, 2025
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 Included in test pass PR metadata: Label to tag PRs, to facilitate with triage Status:Completed Status:Proposed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants