Optimize KnownColor value lookup via a combined color table #50489

L2 · 2021-03-31T15:32:14Z

Fixes: [Perf] Regression in GetHue and GetSaturation #49161 related to commit d21fe17
Brings the following dotnet/performance microbenchmarks back on par
with .NET5
- System.Drawing.Tests.Perf_Color.GetBrightness
- System.Drawing.Tests.Perf_Color.GetSaturation
- System.Drawing.Tests.Perf_Color.GetHue
Use one known color table for both system and non-system
colors (indexed by the corresponding KnownColor enum value).
This table is split into 2 arrays: color value and color kind (category).
Add color category constants for Unknown, System, and Web.
Combined table removes the need to handle mismatches corresponding
with the KnownColor enum value.
Obtaining the value of and verifying if a known color is a system
color is simplified to a single lookup instead of multiple base
bounds comparisons inside the fragmented KnownColor enum table.
These base bounds comparisons would grow linearly as the KnownColor
enum table had more colors added to it, mixed between system and
non-system. Now no comparisons are needed.

* Fixes perf issue dotnet#46153 related to commit d21fe17 * Brings the following dotnet/performance microbenchmarks back on par with .NET5 * System.Drawing.Tests.Perf_Color.GetBrightness * System.Drawing.Tests.Perf_Color.GetSaturation * System.Drawing.Tests.Perf_Color.GetHue * Use one known color table for both system and non-system colors (indexed by the corresponding KnownColor enum value). * This table stores both the color value and color category. * Add color category enum for Unknown, System, and Web. * Combined table removes the need to handle mismatches corresponding with the KnownColor enum value. * Obtaining the value of and verifying if a known color is a system color is simplified to a single lookup instead of multiple base bounds comparisons inside the fragmented KnownColor enum table. * These base bounds comparisons would grow linearly as the KnownColor enum table had more colors added to it, mixed between system and non-system. Now no comparisons are needed.

dnfadmin · 2021-03-31T15:36:41Z

All CLA requirements met.

ghost · 2021-03-31T15:49:17Z

Tagging subscribers to this area: @safern, @tarekgh
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes perf issue [Perf] Changes at 12/9/2020 1:20:52 PM #46153 related to commit d21fe17
Brings the following dotnet/performance microbenchmarks back on par
with .NET5
- System.Drawing.Tests.Perf_Color.GetBrightness
- System.Drawing.Tests.Perf_Color.GetSaturation
- System.Drawing.Tests.Perf_Color.GetHue
Use one known color table for both system and non-system
colors (indexed by the corresponding KnownColor enum value).
This table stores both the color value and color category.
Add color category enum for Unknown, System, and Web.
Combined table removes the need to handle mismatches corresponding
with the KnownColor enum value.
Obtaining the value of and verifying if a known color is a system
color is simplified to a single lookup instead of multiple base
bounds comparisons inside the fragmented KnownColor enum table.
These base bounds comparisons would grow linearly as the KnownColor
enum table had more colors added to it, mixed between system and
non-system. Now no comparisons are needed.

Author:	L2
Assignees:	-
Labels:	`area-System.Drawing`
Milestone:	-

L2 · 2021-03-31T23:25:24Z

dotnet/performance microbenchmarks filtered with System.Drawing.Tests.Perf_Color*:

Base = .NET6 Before this changeset
Diff = .NET6 After this changeset

No Slower results for the provided threshold = 2% and noise filter = 25ns.

Faster	base/diff	Base Median (ns)	Diff Median (ns)
System.Drawing.Tests.Perf_Color.GetBrightness	1.48	1540.92	1038.91
System.Drawing.Tests.Perf_Color.GetSaturation	1.47	1622.30	1104.48
System.Drawing.Tests.Perf_Color.GetHue	1.35	1755.29	1303.64

Also comparing this changeset to .NET5:

Base = .NET5
Diff = .NET6 After this changeset

No differences found between the benchmark results with threshold 2%.

L2 · 2021-04-02T06:45:00Z

What my profiling showed was that Color.IsKnownColorSystem(color) called here was not being inlined anymore due to the extra comparison that was added to the KnownColorToArgb(KnownColor color) method:

runtime/src/libraries/Common/src/System/Drawing/KnownColorTable.cs

Lines 187 to 199 in 5078dd6

    
           public static uint KnownColorToArgb(KnownColor color) 
        
           { 
        
               Debug.Assert(color > 0 && color <= KnownColor.RebeccaPurple); 
        
               if (Color.IsKnownColorSystem(color)) 
        
               { 
        
                   return GetSystemColorArgb(color); 
        
               } 
        
               return color < KnownColor.ButtonFace 
        
                    ? s_colorTable[(int)color - (int)KnownColor.Transparent] 
        
                    : s_colorTable[(int)color - (int)KnownColor.RebeccaPurple + ((int)KnownColor.YellowGreen - (int)KnownColor.WindowText)]; 
        
           }

I also saw that there was a lot of time being spent in IsKnownColorSystem(KnownColor knownColor) due to the extra comparisons that were added:

runtime/src/libraries/System.Drawing.Primitives/src/System/Drawing/Color.cs

Lines 375 to 376 in 5078dd6

    
           internal static bool IsKnownColorSystem(KnownColor knownColor) 
        
               => ((knownColor >= KnownColor.ActiveBorder) && (knownColor <= KnownColor.WindowText)) || ((knownColor >= KnownColor.ButtonFace) && (knownColor <= KnownColor.MenuHighlight));

To mitigate both causes, I combined the color lookup tables inside KnownColorTable.cs, indexed by their enum value in KnownColor.cs. I also augmented this table to include the color category which leads to no base bounds comparisons necessary to check if a color is categorized as system or non-system (web).

Regarding the base bounds comparisons: the existing implementation categorized the color based on its location inside the KnownColor enum table. As this table became more fragmented, more comparisons would be needed to categorize a color.

For example:
Enum {
SystemColor1
SystemColor2
SystemColor3
Web1
Web2
SystemColor4
SystemColor5
SystemColor6
Web3
SystemColor7
Web4}

Since modifications to the KnownColor enum table are breaking changes, new colors are added to the end and the existing implementation checks if a given known color is a system color by checking if it's inside the enum value bounds of (SystemColor1 to SystemColor3 | SystemColor4 to SystemColor6| SystemColor7) in this example. As this becomes more fragmented, the comparisons grow linearly.

As we already have a unique lookup value for each color (their enum value) we can use a secondary table as it already exists in two parts (one for web and another for system) in KnownColorTable, combine it, and also include the category labels for comparison free checking of a known color's category. This brings performance back on par with .NET5.

safern · 2021-04-02T21:26:39Z

src/libraries/Common/src/System/Drawing/KnownColorTable.cs

-            0xFFF5F5F5,     // WhiteSmoke
-            0xFFFFFF00,     // Yellow
-            0xFF9ACD32,     // YellowGreen
+            Unknown,


should we instead introduce 3 constants?

Thanks @safern , yes I did consider that initially but chose the enum route in favor of constants due to easier readability and maybe simpler when using the library? I believe enum compiles to a constant so I went with that currently. Also by using enums we can expand the categories in the future to hold more values other than unknown, system, and web. But you're right, if we use constants directly there will be less casting involved. I will follow your recommendation.

I doubt having more categories. If we end up in the need to using more categories we can go back to an enum.

Yeah, enums are handy cause they compile to a constant, but we are doing a lot of castings for this scenario, so it might just be better to store them as uint constants, or even byte constants given that we don't need 32 bits for the current categories?

Sure, sounds good. I will change the enum categories to byte constants. Just a quick question, I had originally made them uint so I could put them in the same uint array:

public static readonly uint[,] s_colorTable = new uint[knownColorCount, 2]

but it turned out I still needed to cast the enums when placing them inside this array. By using byte constants, I will still need to cast to place them inside right? Or do you recommend something like an array of structs to hold the category byte constant and color value so we don't need to cast when creating the array?

We could have an array of byte, uint tiple. But let's leave it as you have it now using uint for both values. I'm not sure what would be better.

@tannergooding insight might help.

Thanks, I've updated it with the requested changes, keeping constant uint categories as suggested for now.

Hi @safern , anything else I can do to enhance this PR? Thanks for the help.

Sorry, I missed the ping and Santi just pinged me offline.

My recommendation is to split this into a byte[] (for the kind) and uint[] (for the value). The JIT and C# compiler can optimize this scenario and it will save us some space.
The byte[] for the kind could even be further compressed if space was a concern, since its basically a bitmap (0 or 1, per entry)

Hi @L2, I just spoke with @tannergooding and we think the best approach for this would be to have 2 separate arrays.

One byte[] for the color kind and one uint[] for the color itself. Having a multidimensional array might affect startup perf and memory perf vs two separate arrays, plus since these are primitives, the JIT will optimize these 2 arrays.

EDIT: @tannergooding beat me to reply and didn't get the comment until I posted it 😄

Thank you, @tannergooding and @safern . I've updated it with the recommended changes. Looks to have a nice bump in speed with the casting removed and split array.

dotnet/performance microbenchmarks filtered with System.Drawing.Tests.Perf_Color*:

summary:
better: 3, geomean: 1.480
total diff: 3

No Slower results for the provided threshold = 2% and noise filter = 25ns.

Faster base/diff Base Median (ns) Diff Median (ns) Modality

System.Drawing.Tests.Perf_Color.GetBrightness 1.55 1542.74 996.36

System.Drawing.Tests.Perf_Color.GetSaturation 1.50 1597.70 1064.63

System.Drawing.Tests.Perf_Color.GetHue 1.40 1757.32 1258.50

* As recommended by: tannergooding and safern

safern · 2021-04-08T20:08:21Z

src/libraries/Common/src/System/Drawing/KnownColorTable.cs

        internal static Color ArgbToKnownColor(uint argb)
        {
-            // Should be fully opaque (and as such we can skip the first entry
-            // which is transparent).
            Debug.Assert((argb & Color.ARGBAlphaMask) == Color.ARGBAlphaMask);


Maybe add a Debug.Assert here to assert that the two tables are the same length?

Thanks, updated.

src/libraries/Common/src/System/Drawing/KnownColorTable.cs

safern

Thank you very much!

DrewScoggins · 2021-04-14T18:51:50Z

Improvemnts in System.Drawing.Tests.Perf_Color

Benchmark	Baseline	Test	Test/Base	Baseline IR	Compare IR	IR Ratio	Baseline ETL	Compare ETL
GetHue	1.65 μs	1.36 μs	0.83
GetSaturation	1.39 μs	1.15 μs	0.82

![graph]
Historical Data in Reporting System

We are seeing these results in the lab, thanks for tracking down and fixing this issue.

dotnet-issue-labeler bot added the area-System.Drawing label Mar 31, 2021

L2 force-pushed the KnownColorFix branch from fb7b1c7 to 06b7b24 Compare March 31, 2021 15:36

L2 added 2 commits March 31, 2021 10:25

Cast Interop corresponding colors to byte before casting to uint

87e6834

Only consider web colors inside ArgbToKnownColor

b3263b7

L2 mentioned this pull request Apr 2, 2021

[Perf] Changes at 12/9/2020 1:20:52 PM #46153

Closed

safern reviewed Apr 2, 2021

View reviewed changes

Replace known color category enum with constants to reduce casting

a21cf0c

adamsitnik mentioned this pull request Apr 8, 2021

[Perf] Regression in GetHue and GetSaturation #49161

Closed

Split color table into value and kind arrays

b21f20b

* As recommended by: tannergooding and safern

safern reviewed Apr 8, 2021

View reviewed changes

src/libraries/Common/src/System/Drawing/KnownColorTable.cs Outdated Show resolved Hide resolved

L2 added 2 commits April 8, 2021 13:44

Use color array length instead of separate constant

18e45a1

Add test coverage for correct ordering of color kind table

232bb34

This was referenced Apr 9, 2021

Runtime jobs being abandoned due to infra #50746

Closed

Runtime jobs being abandoned due to infra #50747

Closed

safern approved these changes Apr 10, 2021

View reviewed changes

safern merged commit 5f95440 into dotnet:main Apr 14, 2021

L2 deleted the KnownColorFix branch April 16, 2021 07:15

DrewScoggins mentioned this pull request Apr 20, 2021

[Perf] Changes at 4/14/2021 9:38:19 PM DrewScoggins/performance-2#5167

Open

stephentoub mentioned this pull request Apr 23, 2021

Change a byte[] in KnownColorTable to be ReadOnlySpan<byte> #51719

Merged

ghost locked as resolved and limited conversation to collaborators May 16, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize KnownColor value lookup via a combined color table #50489

Optimize KnownColor value lookup via a combined color table #50489

L2 commented Mar 31, 2021 •

edited

Loading

dnfadmin commented Mar 31, 2021 •

edited

Loading

ghost commented Mar 31, 2021

L2 commented Mar 31, 2021 •

edited

Loading

L2 commented Apr 2, 2021 •

edited

Loading

safern Apr 2, 2021

L2 Apr 2, 2021

safern Apr 2, 2021

L2 Apr 2, 2021 •

edited

Loading

safern Apr 3, 2021

L2 Apr 3, 2021

L2 Apr 8, 2021

tannergooding Apr 8, 2021

safern Apr 8, 2021 •

edited

Loading

L2 Apr 8, 2021

safern Apr 8, 2021

L2 Apr 8, 2021

safern left a comment

DrewScoggins commented Apr 14, 2021

Faster	base/diff	Base Median (ns)	Diff Median (ns)
System.Drawing.Tests.Perf_Color.GetBrightness	1.55	1542.74	996.36
System.Drawing.Tests.Perf_Color.GetSaturation	1.50	1597.70	1064.63
System.Drawing.Tests.Perf_Color.GetHue	1.40	1757.32	1258.50

Optimize KnownColor value lookup via a combined color table #50489

Optimize KnownColor value lookup via a combined color table #50489

Conversation

L2 commented Mar 31, 2021 • edited Loading

dnfadmin commented Mar 31, 2021 • edited Loading

ghost commented Mar 31, 2021

L2 commented Mar 31, 2021 • edited Loading

L2 commented Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

L2 Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern left a comment

Choose a reason for hiding this comment

DrewScoggins commented Apr 14, 2021

Improvemnts in System.Drawing.Tests.Perf_Color

L2 commented Mar 31, 2021 •

edited

Loading

dnfadmin commented Mar 31, 2021 •

edited

Loading

L2 commented Mar 31, 2021 •

edited

Loading

L2 commented Apr 2, 2021 •

edited

Loading

L2 Apr 2, 2021 •

edited

Loading

safern Apr 8, 2021 •

edited

Loading