Improve RGB Min Max evaluation performance by using 2 or 3 comparison…

[L2]

Fixes: #46153

…s instead of 4

• Replaces 2 calls each to Math.Min and Math.Max (a total of 4 required
  comparisons) with a single call to MinMaxRgb (at least 2 required
  comparisons and at worst 3).

[ghost]

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

Issue Details

---

…s instead of 4

• Replaces 2 calls each to Math.Min and Math.Max (a total of 4 required
  comparisons) with a single call to MinMaxRgb (at least 2 required
  comparisons and at worst 3).

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

[L2]

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)	Modality
System.Drawing.Tests.Perf_Color.GetHue	1.23	1756.53	1432.47
System.Drawing.Tests.Perf_Color.GetBrightness	1.09	1544.74	1415.17
System.Drawing.Tests.Perf_Color.GetSaturation	1.09	1602.51	1476.70

[iamcarbon]

Move trailing braces to their own lines for consistency with existing .NET code.

[L2]

Thanks, updated.

[danmoseley]

Do we have sufficient functional test coverage?

[L2]

Do we have sufficient functional test coverage?

Thanks @danmoseley . There were a couple of test cases missing which I've just added:

All Possible Cases:

label for existing test	case
(1)	r == g == b
(2)	r == g < b
(3)	r == g > b
(4)	r < g == b
(missing)	r < g < b
	r < g > b
(5)	where [r == b]
(missing)	where [r > b]
(missing)	where [r < b]
(7)	r > g == b
	r > g < b
(6)	where [r == b]
(missing)	where [r > b]
(missing)	where [r < b]
(missing)	r > g > b

Existing Test Coverage:

test label	case
(1)	[InlineData(0, 0, 0, 0f)]
	[InlineData(255, 255, 255, 1f)]
(7)	[InlineData(255, 0, 0, 0.5f)]
(5)	[InlineData(0, 255, 0, 0.5f)]
(2)	[InlineData(0, 0, 255, 0.5f)]
(3)	[InlineData(255, 255, 0, 0.5f)]
(6)	[InlineData(255, 0, 255, 0.5f)]
(4)	[InlineData(0, 255, 255, 0.5f)]
	[InlineData(51, 255, 255, 0.6f)]
	[InlineData(255, 51, 255, 0.6f)]
	[InlineData(255, 255, 51, 0.6f)]
	[InlineData(255, 51, 51, 0.6f)]
	[InlineData(51, 255, 51, 0.6f)]
	[InlineData(51, 51, 255, 0.6f)]
	[InlineData(51, 51, 51, 0.2f)]

For:

public void GetBrightness(int r, int g, int b, float expected)
{
Assert.Equal(expected, Color.FromArgb(r, g, b).GetBrightness());
}

[L2]

Also I wasn't sure if I should put this new method in Math.cs and make it a general use case to find the min and max of three values so I've left it inside Color.cs for now.

[danmoseley]

wasn't sure if I should put this new method in Math.cs

Drawing wouldn't be able to use anything from Math unless it was public: the libraries can only use each other's public surface. And to make new public API involves a review process, which would include consideration of how broadly useful the API would be. So I think it is in the right place.

[safern]

We could simplify this to use ? assignment?

Suggested change

	if(r<g)
	max=g;
	else
	max=r;
	max=r<g?g:r;

[L2]

Thanks, updated.

[safern]

Ditto:

Suggested change

	if(g<r)
	min=g;
	else
	min=r;
	min=g<r?g:r;

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Other than small style improvements, looks good. Thanks!

[L2]

Other than small style improvements, looks good. Thanks!

Thanks @safern , I've updated it with the requested changes.

[ghost]

Hello @safern!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[stephentoub]

This can be simplified to instead be:

Suggested change

	privatevoidMinMaxRgb(outintmin,outintmax,intr,intg,intb)
	{
	if(b<g)
	{
	if(b<r)
	{
	min=b;
	max=r<g?g:r;
	}
	else
	{
	min=r;
	max=g;
	}
	}
	else
	{
	if(r<b)
	{
	max=b;
	min=g<r?g:r;
	}
	else
	{
	max=r;
	min=g;
	}
	}
	}
	[MethodImpl(MethodImplOptions.AggressiveInlining)]
	privatestaticvoidMinMaxRgb(outintmin,outintmax,intr,intg,intb)
	{
	if(r>g)
	{
	max=r;
	min=g;
	}
	else
	{
	max=g;
	min=r;
	}
	if(b>max)
	{
	max=b;
	}
	elseif(b<min)
	{
	min=b;
	}
	}

[L2]

Thanks @stephentoub , I ran the perf_color microbenchmarks against 1) before this PR changeset, 2) the current PR's changeset, 3) proposed simplification

**.NET6 Before VS .NET6 with current PR changeset: Run1**

summary: better: 3, geomean: 1.151 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.23	1549.96	1259.50
System.Drawing.Tests.Perf_Color.GetHue	1.18	1753.24	1484.57
System.Drawing.Tests.Perf_Color.GetSaturation	1.05	1451.59	1383.79

**.NET6 Before VS .NET6 with current PR changeset: Run2**

summary: better: 3, geomean: 1.208 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.23	1543.19	1259.71
System.Drawing.Tests.Perf_Color.GetSaturation	1.22	1682.03	1384.17
System.Drawing.Tests.Perf_Color.GetHue	1.18	1758.68	1485.53

**.NET6 Before VS .NET6 with current PR changeset: Run3**

summary: better: 3, geomean: 1.189 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.GetSaturation	1.22	1682.42	1384.03
System.Drawing.Tests.Perf_Color.GetBrightness	1.19	1497.18	1259.28
System.Drawing.Tests.Perf_Color.GetHue	1.16	1725.25	1483.41

---

---

**.NET6 Before VS .NET6 with proposed simplification: Run1**

summary: better: 2, geomean: 1.123 total diff: 2

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.13	1549.96	1375.52
System.Drawing.Tests.Perf_Color.GetHue	1.12	1753.24	1565.75

**.NET6 Before VS .NET6 with proposed simplification: Run2**

summary: better: 3, geomean: 1.135 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.GetSaturation	1.16	1682.03	1448.45
System.Drawing.Tests.Perf_Color.GetHue	1.12	1758.68	1566.17
System.Drawing.Tests.Perf_Color.GetBrightness	1.12	1543.19	1375.17

**.NET6 Before VS .NET6 with proposed simplification: Run3**

summary: better: 2, geomean: 1.132 worse: 1, geomean: 1.050 total diff: 3

Slower	diff/base	Base Median (ns)	Diff Median (ns)	Modality
System.Drawing.Tests.Perf_Color.GetBrightness	1.05	1497.18	1572.36

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
System.Drawing.Tests.Perf_Color.GetSaturation	1.16	1682.42	1446.59
System.Drawing.Tests.Perf_Color.GetHue	1.10	1725.25	1565.42

---------------------- ----------------------

Some of the benchmarks seem to get a bit slower on average after the simplification. I think what is happening may be two causes:

1. There are paths with an extra store (we store max & min in the first conditional and then if the second conditional is taken one of these is overwritten).
2. Just my guess, but processor pipelining with branch prediction may find it easier to go through each of the conditionals in the current implementation as there are no dependencies. The simplification introduces a dependency on the second conditional as it needs the values from the first? Just a guess here.

I will follow yours and @safern 's recommendation on which to keep. Thanks again.

[KalleOlaviNiemitalo]

I didn't look at the disassembly but there might be an aliasing complication also, if the JIT compiler has to account for the possibility that min and max refer to the same location. Although perhaps it can disprove that after inlining.

[stephentoub]

I actually see them get faster on my machine. Regardless, though, I'd rather keep the code smaller, especially with it triplicated due to the aggressive inlining.

[hypeartist]

One more possible improvement - using local valiables instead of out parameters:

privatestaticvoidMinMaxRgb(outintmin,outintmax,intr,intg,intb){intminLoc,maxLoc;if(r>g){maxLoc=r;minLoc=g;}else{maxLoc=g;minLoc=r;}if(b>maxLoc){maxLoc=b;}elseif(b<minLoc){minLoc=b;}max=maxLoc;min=minLoc;}

[L2]

I actually see them get faster on my machine. Regardless, though, I'd rather keep the code smaller, especially with it triplicated due to the aggressive inlining.

Sounds good @stephentoub , I've updated it with the requested changes. (I'm not sure how crediting works in this project as this is my second pull request, so I've added your username to the commit details as the originator of the simplification).

One more possible improvement - using local valiables instead of out parameters:

Thanks @hypeartist, I'm wondering if these local variables will be eliminated due to inlining, so performance will be about the same?

[stephentoub]

I'm not sure how crediting works in this project as this is my second pull request, so I've added your username to the commit details as the originator of the simplification

Thanks, but not necessary... I comment on a lot of PRs :)

[stephentoub]

I'm wondering if these local variables will be eliminated due to inlining, so performance will be about the same?

Yes, e.g.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gOgCUBXAOwwEt8YLAMIR8AB14AbGFADKMgG68wMXAG4aNYgGYmpBkIYBvGgzNNdvHgwCy5ABRWMDKGgZOGAczcfgASmNTc2CbKxtsBDZPYAcIDmcPfCs3OITrfAi3Vy83fw1qYODiAHYGDIQGODKrfOCAXyCzRot3axtSR2tsj29W539AgsKzUK5wyOiO1L7qrhT4mfKst1685qLS8srZ2vMGoaaDhgBtGxgMAAsIABMASXFJezPLm/uxSQB5MT4ILlwWACCnk8sFwuF4Chgty4kisVk8fgAus0xFAIdgMDAmOQkEwULYwhEojF7NNEskGGT0pkZt1rL1fH5miYjsFeAAzBj2KAMAB8XiZrPMLOGwy2AF4XLtReYklwGJLPNLCvsZTBJLgYOthdqxREFV5lTK5QaoEa9rr3Jz7MA+WUIoKZSKZbL9ZLgOazKrRerNVaubaADyzR2i50u2YGj2W71e5rNU7nK53B5PJOvB5fH5/QHA0HgyHQ2FceFIlFohQYrHMXHEfGjcbEqYLclzSkt6kIHxdbvOBnWfzMy2tgAyEDAbnKY7Anv93LtCMt4ZdU/HptnwTl04NSpjlt9WqFZmXxoi28VG9lVnPUr31GH1tt/NXYFDwxPopfUdnscKB7nQazNOb6FB+YrXmu7o/poR72hUkovrOJoIRBM7NPsdRAA===

[danmoseley]

Thanks for the contribution @L2 - perhaps you'd like to pick up something else?

[L2]

Fixes: #46153

…s instead of 4

• Replaces 2 calls each to Math.Min and Math.Max (a total of 4 required
  comparisons) with a single call to MinMaxRgb (at least 2 required
  comparisons and at worst 3).

[ghost]

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

Issue Details

---

…s instead of 4

• Replaces 2 calls each to Math.Min and Math.Max (a total of 4 required
  comparisons) with a single call to MinMaxRgb (at least 2 required
  comparisons and at worst 3).

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

[L2]

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)	Modality
System.Drawing.Tests.Perf_Color.GetHue	1.23	1756.53	1432.47
System.Drawing.Tests.Perf_Color.GetBrightness	1.09	1544.74	1415.17
System.Drawing.Tests.Perf_Color.GetSaturation	1.09	1602.51	1476.70

[iamcarbon]

Move trailing braces to their own lines for consistency with existing .NET code.

[L2]

Thanks, updated.

[danmoseley]

Do we have sufficient functional test coverage?

[L2]

Do we have sufficient functional test coverage?

Thanks @danmoseley . There were a couple of test cases missing which I've just added:

All Possible Cases:

label for existing test	case
(1)	r == g == b
(2)	r == g < b
(3)	r == g > b
(4)	r < g == b
(missing)	r < g < b
	r < g > b
(5)	where [r == b]
(missing)	where [r > b]
(missing)	where [r < b]
(7)	r > g == b
	r > g < b
(6)	where [r == b]
(missing)	where [r > b]
(missing)	where [r < b]
(missing)	r > g > b

Existing Test Coverage:

test label	case
(1)	[InlineData(0, 0, 0, 0f)]
	[InlineData(255, 255, 255, 1f)]
(7)	[InlineData(255, 0, 0, 0.5f)]
(5)	[InlineData(0, 255, 0, 0.5f)]
(2)	[InlineData(0, 0, 255, 0.5f)]
(3)	[InlineData(255, 255, 0, 0.5f)]
(6)	[InlineData(255, 0, 255, 0.5f)]
(4)	[InlineData(0, 255, 255, 0.5f)]
	[InlineData(51, 255, 255, 0.6f)]
	[InlineData(255, 51, 255, 0.6f)]
	[InlineData(255, 255, 51, 0.6f)]
	[InlineData(255, 51, 51, 0.6f)]
	[InlineData(51, 255, 51, 0.6f)]
	[InlineData(51, 51, 255, 0.6f)]
	[InlineData(51, 51, 51, 0.2f)]

For:

public void GetBrightness(int r, int g, int b, float expected)
{
Assert.Equal(expected, Color.FromArgb(r, g, b).GetBrightness());
}

[L2]

Also I wasn't sure if I should put this new method in Math.cs and make it a general use case to find the min and max of three values so I've left it inside Color.cs for now.

[danmoseley]

wasn't sure if I should put this new method in Math.cs

Drawing wouldn't be able to use anything from Math unless it was public: the libraries can only use each other's public surface. And to make new public API involves a review process, which would include consideration of how broadly useful the API would be. So I think it is in the right place.

[safern]

We could simplify this to use ? assignment?

Suggested change

	if(r<g)
	max=g;
	else
	max=r;
	max=r<g?g:r;

[L2]

Thanks, updated.

[safern]

Ditto:

Suggested change

	if(g<r)
	min=g;
	else
	min=r;
	min=g<r?g:r;

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Other than small style improvements, looks good. Thanks!

[L2]

Other than small style improvements, looks good. Thanks!

Thanks @safern , I've updated it with the requested changes.

[ghost]

Hello @safern!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[stephentoub]

This can be simplified to instead be:

Suggested change

	privatevoidMinMaxRgb(outintmin,outintmax,intr,intg,intb)
	{
	if(b<g)
	{
	if(b<r)
	{
	min=b;
	max=r<g?g:r;
	}
	else
	{
	min=r;
	max=g;
	}
	}
	else
	{
	if(r<b)
	{
	max=b;
	min=g<r?g:r;
	}
	else
	{
	max=r;
	min=g;
	}
	}
	}
	[MethodImpl(MethodImplOptions.AggressiveInlining)]
	privatestaticvoidMinMaxRgb(outintmin,outintmax,intr,intg,intb)
	{
	if(r>g)
	{
	max=r;
	min=g;
	}
	else
	{
	max=g;
	min=r;
	}
	if(b>max)
	{
	max=b;
	}
	elseif(b<min)
	{
	min=b;
	}
	}

[L2]

Thanks @stephentoub , I ran the perf_color microbenchmarks against 1) before this PR changeset, 2) the current PR's changeset, 3) proposed simplification

**.NET6 Before VS .NET6 with current PR changeset: Run1**

summary: better: 3, geomean: 1.151 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.23	1549.96	1259.50
System.Drawing.Tests.Perf_Color.GetHue	1.18	1753.24	1484.57
System.Drawing.Tests.Perf_Color.GetSaturation	1.05	1451.59	1383.79

**.NET6 Before VS .NET6 with current PR changeset: Run2**

summary: better: 3, geomean: 1.208 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.23	1543.19	1259.71
System.Drawing.Tests.Perf_Color.GetSaturation	1.22	1682.03	1384.17
System.Drawing.Tests.Perf_Color.GetHue	1.18	1758.68	1485.53

**.NET6 Before VS .NET6 with current PR changeset: Run3**

summary: better: 3, geomean: 1.189 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.GetSaturation	1.22	1682.42	1384.03
System.Drawing.Tests.Perf_Color.GetBrightness	1.19	1497.18	1259.28
System.Drawing.Tests.Perf_Color.GetHue	1.16	1725.25	1483.41

---

---

**.NET6 Before VS .NET6 with proposed simplification: Run1**

summary: better: 2, geomean: 1.123 total diff: 2

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.13	1549.96	1375.52
System.Drawing.Tests.Perf_Color.GetHue	1.12	1753.24	1565.75

**.NET6 Before VS .NET6 with proposed simplification: Run2**

summary: better: 3, geomean: 1.135 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.GetSaturation	1.16	1682.03	1448.45
System.Drawing.Tests.Perf_Color.GetHue	1.12	1758.68	1566.17
System.Drawing.Tests.Perf_Color.GetBrightness	1.12	1543.19	1375.17

**.NET6 Before VS .NET6 with proposed simplification: Run3**

summary: better: 2, geomean: 1.132 worse: 1, geomean: 1.050 total diff: 3

Slower	diff/base	Base Median (ns)	Diff Median (ns)	Modality
System.Drawing.Tests.Perf_Color.GetBrightness	1.05	1497.18	1572.36

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
System.Drawing.Tests.Perf_Color.GetSaturation	1.16	1682.42	1446.59
System.Drawing.Tests.Perf_Color.GetHue	1.10	1725.25	1565.42

---------------------- ----------------------

Some of the benchmarks seem to get a bit slower on average after the simplification. I think what is happening may be two causes:

1. There are paths with an extra store (we store max & min in the first conditional and then if the second conditional is taken one of these is overwritten).
2. Just my guess, but processor pipelining with branch prediction may find it easier to go through each of the conditionals in the current implementation as there are no dependencies. The simplification introduces a dependency on the second conditional as it needs the values from the first? Just a guess here.

I will follow yours and @safern 's recommendation on which to keep. Thanks again.

[KalleOlaviNiemitalo]

I didn't look at the disassembly but there might be an aliasing complication also, if the JIT compiler has to account for the possibility that min and max refer to the same location. Although perhaps it can disprove that after inlining.

[stephentoub]

I actually see them get faster on my machine. Regardless, though, I'd rather keep the code smaller, especially with it triplicated due to the aggressive inlining.

[hypeartist]

One more possible improvement - using local valiables instead of out parameters:

privatestaticvoidMinMaxRgb(outintmin,outintmax,intr,intg,intb){intminLoc,maxLoc;if(r>g){maxLoc=r;minLoc=g;}else{maxLoc=g;minLoc=r;}if(b>maxLoc){maxLoc=b;}elseif(b<minLoc){minLoc=b;}max=maxLoc;min=minLoc;}

[L2]

I actually see them get faster on my machine. Regardless, though, I'd rather keep the code smaller, especially with it triplicated due to the aggressive inlining.

Sounds good @stephentoub , I've updated it with the requested changes. (I'm not sure how crediting works in this project as this is my second pull request, so I've added your username to the commit details as the originator of the simplification).

One more possible improvement - using local valiables instead of out parameters:

Thanks @hypeartist, I'm wondering if these local variables will be eliminated due to inlining, so performance will be about the same?

[stephentoub]

I'm not sure how crediting works in this project as this is my second pull request, so I've added your username to the commit details as the originator of the simplification

Thanks, but not necessary... I comment on a lot of PRs :)

[stephentoub]

I'm wondering if these local variables will be eliminated due to inlining, so performance will be about the same?

Yes, e.g.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gOgCUBXAOwwEt8YLAMIR8AB14AbGFADKMgG68wMXAG4aNYgGYmpBkIYBvGgzNNdvHgwCy5ABRWMDKGgZOGAczcfgASmNTc2CbKxtsBDZPYAcIDmcPfCs3OITrfAi3Vy83fw1qYODiAHYGDIQGODKrfOCAXyCzRot3axtSR2tsj29W539AgsKzUK5wyOiO1L7qrhT4mfKst1685qLS8srZ2vMGoaaDhgBtGxgMAAsIABMASXFJezPLm/uxSQB5MT4ILlwWACCnk8sFwuF4Chgty4kisVk8fgAus0xFAIdgMDAmOQkEwULYwhEojF7NNEskGGT0pkZt1rL1fH5miYjsFeAAzBj2KAMAB8XiZrPMLOGwy2AF4XLtReYklwGJLPNLCvsZTBJLgYOthdqxREFV5lTK5QaoEa9rr3Jz7MA+WUIoKZSKZbL9ZLgOazKrRerNVaubaADyzR2i50u2YGj2W71e5rNU7nK53B5PJOvB5fH5/QHA0HgyHQ2FceFIlFohQYrHMXHEfGjcbEqYLclzSkt6kIHxdbvOBnWfzMy2tgAyEDAbnKY7Anv93LtCMt4ZdU/HptnwTl04NSpjlt9WqFZmXxoi28VG9lVnPUr31GH1tt/NXYFDwxPopfUdnscKB7nQazNOb6FB+YrXmu7o/poR72hUkovrOJoIRBM7NPsdRAA===

[danmoseley]

Thanks for the contribution @L2 - perhaps you'd like to pick up something else?

[L2]

Fixes: #46153

…s instead of 4

• Replaces 2 calls each to Math.Min and Math.Max (a total of 4 required
  comparisons) with a single call to MinMaxRgb (at least 2 required
  comparisons and at worst 3).

[ghost]

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

Issue Details

---

…s instead of 4

• Replaces 2 calls each to Math.Min and Math.Max (a total of 4 required
  comparisons) with a single call to MinMaxRgb (at least 2 required
  comparisons and at worst 3).

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

[L2]

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)	Modality
System.Drawing.Tests.Perf_Color.GetHue	1.23	1756.53	1432.47
System.Drawing.Tests.Perf_Color.GetBrightness	1.09	1544.74	1415.17
System.Drawing.Tests.Perf_Color.GetSaturation	1.09	1602.51	1476.70

[iamcarbon]

Move trailing braces to their own lines for consistency with existing .NET code.

[L2]

Thanks, updated.

[danmoseley]

Do we have sufficient functional test coverage?

[L2]

Do we have sufficient functional test coverage?

Thanks @danmoseley . There were a couple of test cases missing which I've just added:

All Possible Cases:

label for existing test	case
(1)	r == g == b
(2)	r == g < b
(3)	r == g > b
(4)	r < g == b
(missing)	r < g < b
	r < g > b
(5)	where [r == b]
(missing)	where [r > b]
(missing)	where [r < b]
(7)	r > g == b
	r > g < b
(6)	where [r == b]
(missing)	where [r > b]
(missing)	where [r < b]
(missing)	r > g > b

Existing Test Coverage:

test label	case
(1)	[InlineData(0, 0, 0, 0f)]
	[InlineData(255, 255, 255, 1f)]
(7)	[InlineData(255, 0, 0, 0.5f)]
(5)	[InlineData(0, 255, 0, 0.5f)]
(2)	[InlineData(0, 0, 255, 0.5f)]
(3)	[InlineData(255, 255, 0, 0.5f)]
(6)	[InlineData(255, 0, 255, 0.5f)]
(4)	[InlineData(0, 255, 255, 0.5f)]
	[InlineData(51, 255, 255, 0.6f)]
	[InlineData(255, 51, 255, 0.6f)]
	[InlineData(255, 255, 51, 0.6f)]
	[InlineData(255, 51, 51, 0.6f)]
	[InlineData(51, 255, 51, 0.6f)]
	[InlineData(51, 51, 255, 0.6f)]
	[InlineData(51, 51, 51, 0.2f)]

For:

public void GetBrightness(int r, int g, int b, float expected)
{
Assert.Equal(expected, Color.FromArgb(r, g, b).GetBrightness());
}

[L2]

Also I wasn't sure if I should put this new method in Math.cs and make it a general use case to find the min and max of three values so I've left it inside Color.cs for now.

[danmoseley]

wasn't sure if I should put this new method in Math.cs

Drawing wouldn't be able to use anything from Math unless it was public: the libraries can only use each other's public surface. And to make new public API involves a review process, which would include consideration of how broadly useful the API would be. So I think it is in the right place.

[safern]

We could simplify this to use ? assignment?

Suggested change

	if(r<g)
	max=g;
	else
	max=r;
	max=r<g?g:r;

[L2]

Thanks, updated.

[safern]

Ditto:

Suggested change

	if(g<r)
	min=g;
	else
	min=r;
	min=g<r?g:r;

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Other than small style improvements, looks good. Thanks!

[L2]

Other than small style improvements, looks good. Thanks!

Thanks @safern , I've updated it with the requested changes.

[ghost]

Hello @safern!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[stephentoub]

This can be simplified to instead be:

Suggested change

	privatevoidMinMaxRgb(outintmin,outintmax,intr,intg,intb)
	{
	if(b<g)
	{
	if(b<r)
	{
	min=b;
	max=r<g?g:r;
	}
	else
	{
	min=r;
	max=g;
	}
	}
	else
	{
	if(r<b)
	{
	max=b;
	min=g<r?g:r;
	}
	else
	{
	max=r;
	min=g;
	}
	}
	}
	[MethodImpl(MethodImplOptions.AggressiveInlining)]
	privatestaticvoidMinMaxRgb(outintmin,outintmax,intr,intg,intb)
	{
	if(r>g)
	{
	max=r;
	min=g;
	}
	else
	{
	max=g;
	min=r;
	}
	if(b>max)
	{
	max=b;
	}
	elseif(b<min)
	{
	min=b;
	}
	}

[L2]

Thanks @stephentoub , I ran the perf_color microbenchmarks against 1) before this PR changeset, 2) the current PR's changeset, 3) proposed simplification

**.NET6 Before VS .NET6 with current PR changeset: Run1**

summary: better: 3, geomean: 1.151 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.23	1549.96	1259.50
System.Drawing.Tests.Perf_Color.GetHue	1.18	1753.24	1484.57
System.Drawing.Tests.Perf_Color.GetSaturation	1.05	1451.59	1383.79

**.NET6 Before VS .NET6 with current PR changeset: Run2**

summary: better: 3, geomean: 1.208 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.23	1543.19	1259.71
System.Drawing.Tests.Perf_Color.GetSaturation	1.22	1682.03	1384.17
System.Drawing.Tests.Perf_Color.GetHue	1.18	1758.68	1485.53

**.NET6 Before VS .NET6 with current PR changeset: Run3**

summary: better: 3, geomean: 1.189 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.GetSaturation	1.22	1682.42	1384.03
System.Drawing.Tests.Perf_Color.GetBrightness	1.19	1497.18	1259.28
System.Drawing.Tests.Perf_Color.GetHue	1.16	1725.25	1483.41

---

---

**.NET6 Before VS .NET6 with proposed simplification: Run1**

summary: better: 2, geomean: 1.123 total diff: 2

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.13	1549.96	1375.52
System.Drawing.Tests.Perf_Color.GetHue	1.12	1753.24	1565.75

**.NET6 Before VS .NET6 with proposed simplification: Run2**

summary: better: 3, geomean: 1.135 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.GetSaturation	1.16	1682.03	1448.45
System.Drawing.Tests.Perf_Color.GetHue	1.12	1758.68	1566.17
System.Drawing.Tests.Perf_Color.GetBrightness	1.12	1543.19	1375.17

**.NET6 Before VS .NET6 with proposed simplification: Run3**

summary: better: 2, geomean: 1.132 worse: 1, geomean: 1.050 total diff: 3

Slower	diff/base	Base Median (ns)	Diff Median (ns)	Modality
System.Drawing.Tests.Perf_Color.GetBrightness	1.05	1497.18	1572.36

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
System.Drawing.Tests.Perf_Color.GetSaturation	1.16	1682.42	1446.59
System.Drawing.Tests.Perf_Color.GetHue	1.10	1725.25	1565.42

---------------------- ----------------------

Some of the benchmarks seem to get a bit slower on average after the simplification. I think what is happening may be two causes:

1. There are paths with an extra store (we store max & min in the first conditional and then if the second conditional is taken one of these is overwritten).
2. Just my guess, but processor pipelining with branch prediction may find it easier to go through each of the conditionals in the current implementation as there are no dependencies. The simplification introduces a dependency on the second conditional as it needs the values from the first? Just a guess here.

I will follow yours and @safern 's recommendation on which to keep. Thanks again.

[KalleOlaviNiemitalo]

I didn't look at the disassembly but there might be an aliasing complication also, if the JIT compiler has to account for the possibility that min and max refer to the same location. Although perhaps it can disprove that after inlining.

[stephentoub]

I actually see them get faster on my machine. Regardless, though, I'd rather keep the code smaller, especially with it triplicated due to the aggressive inlining.

[hypeartist]

One more possible improvement - using local valiables instead of out parameters:

privatestaticvoidMinMaxRgb(outintmin,outintmax,intr,intg,intb){intminLoc,maxLoc;if(r>g){maxLoc=r;minLoc=g;}else{maxLoc=g;minLoc=r;}if(b>maxLoc){maxLoc=b;}elseif(b<minLoc){minLoc=b;}max=maxLoc;min=minLoc;}

[L2]

I actually see them get faster on my machine. Regardless, though, I'd rather keep the code smaller, especially with it triplicated due to the aggressive inlining.

Sounds good @stephentoub , I've updated it with the requested changes. (I'm not sure how crediting works in this project as this is my second pull request, so I've added your username to the commit details as the originator of the simplification).

One more possible improvement - using local valiables instead of out parameters:

Thanks @hypeartist, I'm wondering if these local variables will be eliminated due to inlining, so performance will be about the same?

[stephentoub]

I'm not sure how crediting works in this project as this is my second pull request, so I've added your username to the commit details as the originator of the simplification

Thanks, but not necessary... I comment on a lot of PRs :)

[stephentoub]

I'm wondering if these local variables will be eliminated due to inlining, so performance will be about the same?

Yes, e.g.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gOgCUBXAOwwEt8YLAMIR8AB14AbGFADKMgG68wMXAG4aNYgGYmpBkIYBvGgzNNdvHgwCy5ABRWMDKGgZOGAczcfgASmNTc2CbKxtsBDZPYAcIDmcPfCs3OITrfAi3Vy83fw1qYODiAHYGDIQGODKrfOCAXyCzRot3axtSR2tsj29W539AgsKzUK5wyOiO1L7qrhT4mfKst1685qLS8srZ2vMGoaaDhgBtGxgMAAsIABMASXFJezPLm/uxSQB5MT4ILlwWACCnk8sFwuF4Chgty4kisVk8fgAus0xFAIdgMDAmOQkEwULYwhEojF7NNEskGGT0pkZt1rL1fH5miYjsFeAAzBj2KAMAB8XiZrPMLOGwy2AF4XLtReYklwGJLPNLCvsZTBJLgYOthdqxREFV5lTK5QaoEa9rr3Jz7MA+WUIoKZSKZbL9ZLgOazKrRerNVaubaADyzR2i50u2YGj2W71e5rNU7nK53B5PJOvB5fH5/QHA0HgyHQ2FceFIlFohQYrHMXHEfGjcbEqYLclzSkt6kIHxdbvOBnWfzMy2tgAyEDAbnKY7Anv93LtCMt4ZdU/HptnwTl04NSpjlt9WqFZmXxoi28VG9lVnPUr31GH1tt/NXYFDwxPopfUdnscKB7nQazNOb6FB+YrXmu7o/poR72hUkovrOJoIRBM7NPsdRAA===

[danmoseley]

Thanks for the contribution @L2 - perhaps you'd like to pick up something else?

[L2]

Fixes: #46153

…s instead of 4

• Replaces 2 calls each to Math.Min and Math.Max (a total of 4 required
  comparisons) with a single call to MinMaxRgb (at least 2 required
  comparisons and at worst 3).

[ghost]

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

Issue Details

---

…s instead of 4

• Replaces 2 calls each to Math.Min and Math.Max (a total of 4 required
  comparisons) with a single call to MinMaxRgb (at least 2 required
  comparisons and at worst 3).

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

[L2]

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)	Modality
System.Drawing.Tests.Perf_Color.GetHue	1.23	1756.53	1432.47
System.Drawing.Tests.Perf_Color.GetBrightness	1.09	1544.74	1415.17
System.Drawing.Tests.Perf_Color.GetSaturation	1.09	1602.51	1476.70

[iamcarbon]

Move trailing braces to their own lines for consistency with existing .NET code.

[L2]

Thanks, updated.

[danmoseley]

Do we have sufficient functional test coverage?

[L2]

Do we have sufficient functional test coverage?

Thanks @danmoseley . There were a couple of test cases missing which I've just added:

All Possible Cases:

label for existing test	case
(1)	r == g == b
(2)	r == g < b
(3)	r == g > b
(4)	r < g == b
(missing)	r < g < b
	r < g > b
(5)	where [r == b]
(missing)	where [r > b]
(missing)	where [r < b]
(7)	r > g == b
	r > g < b
(6)	where [r == b]
(missing)	where [r > b]
(missing)	where [r < b]
(missing)	r > g > b

Existing Test Coverage:

test label	case
(1)	[InlineData(0, 0, 0, 0f)]
	[InlineData(255, 255, 255, 1f)]
(7)	[InlineData(255, 0, 0, 0.5f)]
(5)	[InlineData(0, 255, 0, 0.5f)]
(2)	[InlineData(0, 0, 255, 0.5f)]
(3)	[InlineData(255, 255, 0, 0.5f)]
(6)	[InlineData(255, 0, 255, 0.5f)]
(4)	[InlineData(0, 255, 255, 0.5f)]
	[InlineData(51, 255, 255, 0.6f)]
	[InlineData(255, 51, 255, 0.6f)]
	[InlineData(255, 255, 51, 0.6f)]
	[InlineData(255, 51, 51, 0.6f)]
	[InlineData(51, 255, 51, 0.6f)]
	[InlineData(51, 51, 255, 0.6f)]
	[InlineData(51, 51, 51, 0.2f)]

For:

public void GetBrightness(int r, int g, int b, float expected)
{
Assert.Equal(expected, Color.FromArgb(r, g, b).GetBrightness());
}

[L2]

Also I wasn't sure if I should put this new method in Math.cs and make it a general use case to find the min and max of three values so I've left it inside Color.cs for now.

[danmoseley]

wasn't sure if I should put this new method in Math.cs

Drawing wouldn't be able to use anything from Math unless it was public: the libraries can only use each other's public surface. And to make new public API involves a review process, which would include consideration of how broadly useful the API would be. So I think it is in the right place.

[safern]

We could simplify this to use ? assignment?

Suggested change

	if(r<g)
	max=g;
	else
	max=r;
	max=r<g?g:r;

[L2]

Thanks, updated.

[safern]

Ditto:

Suggested change

	if(g<r)
	min=g;
	else
	min=r;
	min=g<r?g:r;

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Other than small style improvements, looks good. Thanks!

[L2]

Other than small style improvements, looks good. Thanks!

Thanks @safern , I've updated it with the requested changes.

[ghost]

Hello @safern!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[stephentoub]

This can be simplified to instead be:

Suggested change

	privatevoidMinMaxRgb(outintmin,outintmax,intr,intg,intb)
	{
	if(b<g)
	{
	if(b<r)
	{
	min=b;
	max=r<g?g:r;
	}
	else
	{
	min=r;
	max=g;
	}
	}
	else
	{
	if(r<b)
	{
	max=b;
	min=g<r?g:r;
	}
	else
	{
	max=r;
	min=g;
	}
	}
	}
	[MethodImpl(MethodImplOptions.AggressiveInlining)]
	privatestaticvoidMinMaxRgb(outintmin,outintmax,intr,intg,intb)
	{
	if(r>g)
	{
	max=r;
	min=g;
	}
	else
	{
	max=g;
	min=r;
	}
	if(b>max)
	{
	max=b;
	}
	elseif(b<min)
	{
	min=b;
	}
	}

[L2]

Thanks @stephentoub , I ran the perf_color microbenchmarks against 1) before this PR changeset, 2) the current PR's changeset, 3) proposed simplification

**.NET6 Before VS .NET6 with current PR changeset: Run1**

summary: better: 3, geomean: 1.151 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.23	1549.96	1259.50
System.Drawing.Tests.Perf_Color.GetHue	1.18	1753.24	1484.57
System.Drawing.Tests.Perf_Color.GetSaturation	1.05	1451.59	1383.79

**.NET6 Before VS .NET6 with current PR changeset: Run2**

summary: better: 3, geomean: 1.208 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.23	1543.19	1259.71
System.Drawing.Tests.Perf_Color.GetSaturation	1.22	1682.03	1384.17
System.Drawing.Tests.Perf_Color.GetHue	1.18	1758.68	1485.53

**.NET6 Before VS .NET6 with current PR changeset: Run3**

summary: better: 3, geomean: 1.189 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.GetSaturation	1.22	1682.42	1384.03
System.Drawing.Tests.Perf_Color.GetBrightness	1.19	1497.18	1259.28
System.Drawing.Tests.Perf_Color.GetHue	1.16	1725.25	1483.41

---

---

**.NET6 Before VS .NET6 with proposed simplification: Run1**

summary: better: 2, geomean: 1.123 total diff: 2

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.13	1549.96	1375.52
System.Drawing.Tests.Perf_Color.GetHue	1.12	1753.24	1565.75

**.NET6 Before VS .NET6 with proposed simplification: Run2**

summary: better: 3, geomean: 1.135 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.GetSaturation	1.16	1682.03	1448.45
System.Drawing.Tests.Perf_Color.GetHue	1.12	1758.68	1566.17
System.Drawing.Tests.Perf_Color.GetBrightness	1.12	1543.19	1375.17

**.NET6 Before VS .NET6 with proposed simplification: Run3**

summary: better: 2, geomean: 1.132 worse: 1, geomean: 1.050 total diff: 3

Slower	diff/base	Base Median (ns)	Diff Median (ns)	Modality
System.Drawing.Tests.Perf_Color.GetBrightness	1.05	1497.18	1572.36

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
System.Drawing.Tests.Perf_Color.GetSaturation	1.16	1682.42	1446.59
System.Drawing.Tests.Perf_Color.GetHue	1.10	1725.25	1565.42

---------------------- ----------------------

Some of the benchmarks seem to get a bit slower on average after the simplification. I think what is happening may be two causes:

1. There are paths with an extra store (we store max & min in the first conditional and then if the second conditional is taken one of these is overwritten).
2. Just my guess, but processor pipelining with branch prediction may find it easier to go through each of the conditionals in the current implementation as there are no dependencies. The simplification introduces a dependency on the second conditional as it needs the values from the first? Just a guess here.

I will follow yours and @safern 's recommendation on which to keep. Thanks again.

[KalleOlaviNiemitalo]

I didn't look at the disassembly but there might be an aliasing complication also, if the JIT compiler has to account for the possibility that min and max refer to the same location. Although perhaps it can disprove that after inlining.

[stephentoub]

I actually see them get faster on my machine. Regardless, though, I'd rather keep the code smaller, especially with it triplicated due to the aggressive inlining.

[hypeartist]

One more possible improvement - using local valiables instead of out parameters:

privatestaticvoidMinMaxRgb(outintmin,outintmax,intr,intg,intb){intminLoc,maxLoc;if(r>g){maxLoc=r;minLoc=g;}else{maxLoc=g;minLoc=r;}if(b>maxLoc){maxLoc=b;}elseif(b<minLoc){minLoc=b;}max=maxLoc;min=minLoc;}

[L2]

I actually see them get faster on my machine. Regardless, though, I'd rather keep the code smaller, especially with it triplicated due to the aggressive inlining.

Sounds good @stephentoub , I've updated it with the requested changes. (I'm not sure how crediting works in this project as this is my second pull request, so I've added your username to the commit details as the originator of the simplification).

One more possible improvement - using local valiables instead of out parameters:

Thanks @hypeartist, I'm wondering if these local variables will be eliminated due to inlining, so performance will be about the same?

[stephentoub]

I'm not sure how crediting works in this project as this is my second pull request, so I've added your username to the commit details as the originator of the simplification

Thanks, but not necessary... I comment on a lot of PRs :)

[stephentoub]

I'm wondering if these local variables will be eliminated due to inlining, so performance will be about the same?

Yes, e.g.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gOgCUBXAOwwEt8YLAMIR8AB14AbGFADKMgG68wMXAG4aNYgGYmpBkIYBvGgzNNdvHgwCy5ABRWMDKGgZOGAczcfgASmNTc2CbKxtsBDZPYAcIDmcPfCs3OITrfAi3Vy83fw1qYODiAHYGDIQGODKrfOCAXyCzRot3axtSR2tsj29W539AgsKzUK5wyOiO1L7qrhT4mfKst1685qLS8srZ2vMGoaaDhgBtGxgMAAsIABMASXFJezPLm/uxSQB5MT4ILlwWACCnk8sFwuF4Chgty4kisVk8fgAus0xFAIdgMDAmOQkEwULYwhEojF7NNEskGGT0pkZt1rL1fH5miYjsFeAAzBj2KAMAB8XiZrPMLOGwy2AF4XLtReYklwGJLPNLCvsZTBJLgYOthdqxREFV5lTK5QaoEa9rr3Jz7MA+WUIoKZSKZbL9ZLgOazKrRerNVaubaADyzR2i50u2YGj2W71e5rNU7nK53B5PJOvB5fH5/QHA0HgyHQ2FceFIlFohQYrHMXHEfGjcbEqYLclzSkt6kIHxdbvOBnWfzMy2tgAyEDAbnKY7Anv93LtCMt4ZdU/HptnwTl04NSpjlt9WqFZmXxoi28VG9lVnPUr31GH1tt/NXYFDwxPopfUdnscKB7nQazNOb6FB+YrXmu7o/poR72hUkovrOJoIRBM7NPsdRAA===

[danmoseley]

Thanks for the contribution @L2 - perhaps you'd like to pick up something else?

[L2]

Fixes: #46153

…s instead of 4

• Replaces 2 calls each to Math.Min and Math.Max (a total of 4 required
  comparisons) with a single call to MinMaxRgb (at least 2 required
  comparisons and at worst 3).

[ghost]

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

Issue Details

---

…s instead of 4

• Replaces 2 calls each to Math.Min and Math.Max (a total of 4 required
  comparisons) with a single call to MinMaxRgb (at least 2 required
  comparisons and at worst 3).

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

[L2]

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)	Modality
System.Drawing.Tests.Perf_Color.GetHue	1.23	1756.53	1432.47
System.Drawing.Tests.Perf_Color.GetBrightness	1.09	1544.74	1415.17
System.Drawing.Tests.Perf_Color.GetSaturation	1.09	1602.51	1476.70

[iamcarbon]

Move trailing braces to their own lines for consistency with existing .NET code.

[L2]

Thanks, updated.

[danmoseley]

Do we have sufficient functional test coverage?

[L2]

Do we have sufficient functional test coverage?

Thanks @danmoseley . There were a couple of test cases missing which I've just added:

All Possible Cases:

label for existing test	case
(1)	r == g == b
(2)	r == g < b
(3)	r == g > b
(4)	r < g == b
(missing)	r < g < b
	r < g > b
(5)	where [r == b]
(missing)	where [r > b]
(missing)	where [r < b]
(7)	r > g == b
	r > g < b
(6)	where [r == b]
(missing)	where [r > b]
(missing)	where [r < b]
(missing)	r > g > b

Existing Test Coverage:

test label	case
(1)	[InlineData(0, 0, 0, 0f)]
	[InlineData(255, 255, 255, 1f)]
(7)	[InlineData(255, 0, 0, 0.5f)]
(5)	[InlineData(0, 255, 0, 0.5f)]
(2)	[InlineData(0, 0, 255, 0.5f)]
(3)	[InlineData(255, 255, 0, 0.5f)]
(6)	[InlineData(255, 0, 255, 0.5f)]
(4)	[InlineData(0, 255, 255, 0.5f)]
	[InlineData(51, 255, 255, 0.6f)]
	[InlineData(255, 51, 255, 0.6f)]
	[InlineData(255, 255, 51, 0.6f)]
	[InlineData(255, 51, 51, 0.6f)]
	[InlineData(51, 255, 51, 0.6f)]
	[InlineData(51, 51, 255, 0.6f)]
	[InlineData(51, 51, 51, 0.2f)]

For:

public void GetBrightness(int r, int g, int b, float expected)
{
Assert.Equal(expected, Color.FromArgb(r, g, b).GetBrightness());
}

[L2]

Also I wasn't sure if I should put this new method in Math.cs and make it a general use case to find the min and max of three values so I've left it inside Color.cs for now.

[danmoseley]

wasn't sure if I should put this new method in Math.cs

Drawing wouldn't be able to use anything from Math unless it was public: the libraries can only use each other's public surface. And to make new public API involves a review process, which would include consideration of how broadly useful the API would be. So I think it is in the right place.

[safern]

We could simplify this to use ? assignment?

Suggested change

	if(r<g)
	max=g;
	else
	max=r;
	max=r<g?g:r;

[L2]

Thanks, updated.

[safern]

Ditto:

Suggested change

	if(g<r)
	min=g;
	else
	min=r;
	min=g<r?g:r;

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Other than small style improvements, looks good. Thanks!

[L2]

Other than small style improvements, looks good. Thanks!

Thanks @safern , I've updated it with the requested changes.

[ghost]

Hello @safern!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[stephentoub]

This can be simplified to instead be:

Suggested change

	privatevoidMinMaxRgb(outintmin,outintmax,intr,intg,intb)
	{
	if(b<g)
	{
	if(b<r)
	{
	min=b;
	max=r<g?g:r;
	}
	else
	{
	min=r;
	max=g;
	}
	}
	else
	{
	if(r<b)
	{
	max=b;
	min=g<r?g:r;
	}
	else
	{
	max=r;
	min=g;
	}
	}
	}
	[MethodImpl(MethodImplOptions.AggressiveInlining)]
	privatestaticvoidMinMaxRgb(outintmin,outintmax,intr,intg,intb)
	{
	if(r>g)
	{
	max=r;
	min=g;
	}
	else
	{
	max=g;
	min=r;
	}
	if(b>max)
	{
	max=b;
	}
	elseif(b<min)
	{
	min=b;
	}
	}

[L2]

Thanks @stephentoub , I ran the perf_color microbenchmarks against 1) before this PR changeset, 2) the current PR's changeset, 3) proposed simplification

**.NET6 Before VS .NET6 with current PR changeset: Run1**

summary: better: 3, geomean: 1.151 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.23	1549.96	1259.50
System.Drawing.Tests.Perf_Color.GetHue	1.18	1753.24	1484.57
System.Drawing.Tests.Perf_Color.GetSaturation	1.05	1451.59	1383.79

**.NET6 Before VS .NET6 with current PR changeset: Run2**

summary: better: 3, geomean: 1.208 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.23	1543.19	1259.71
System.Drawing.Tests.Perf_Color.GetSaturation	1.22	1682.03	1384.17
System.Drawing.Tests.Perf_Color.GetHue	1.18	1758.68	1485.53

**.NET6 Before VS .NET6 with current PR changeset: Run3**

summary: better: 3, geomean: 1.189 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.GetSaturation	1.22	1682.42	1384.03
System.Drawing.Tests.Perf_Color.GetBrightness	1.19	1497.18	1259.28
System.Drawing.Tests.Perf_Color.GetHue	1.16	1725.25	1483.41

---

---

**.NET6 Before VS .NET6 with proposed simplification: Run1**

summary: better: 2, geomean: 1.123 total diff: 2

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.13	1549.96	1375.52
System.Drawing.Tests.Perf_Color.GetHue	1.12	1753.24	1565.75

**.NET6 Before VS .NET6 with proposed simplification: Run2**

summary: better: 3, geomean: 1.135 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.GetSaturation	1.16	1682.03	1448.45
System.Drawing.Tests.Perf_Color.GetHue	1.12	1758.68	1566.17
System.Drawing.Tests.Perf_Color.GetBrightness	1.12	1543.19	1375.17

**.NET6 Before VS .NET6 with proposed simplification: Run3**

summary: better: 2, geomean: 1.132 worse: 1, geomean: 1.050 total diff: 3

Slower	diff/base	Base Median (ns)	Diff Median (ns)	Modality
System.Drawing.Tests.Perf_Color.GetBrightness	1.05	1497.18	1572.36

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
System.Drawing.Tests.Perf_Color.GetSaturation	1.16	1682.42	1446.59
System.Drawing.Tests.Perf_Color.GetHue	1.10	1725.25	1565.42

---------------------- ----------------------

Some of the benchmarks seem to get a bit slower on average after the simplification. I think what is happening may be two causes:

1. There are paths with an extra store (we store max & min in the first conditional and then if the second conditional is taken one of these is overwritten).
2. Just my guess, but processor pipelining with branch prediction may find it easier to go through each of the conditionals in the current implementation as there are no dependencies. The simplification introduces a dependency on the second conditional as it needs the values from the first? Just a guess here.

I will follow yours and @safern 's recommendation on which to keep. Thanks again.

[KalleOlaviNiemitalo]

I didn't look at the disassembly but there might be an aliasing complication also, if the JIT compiler has to account for the possibility that min and max refer to the same location. Although perhaps it can disprove that after inlining.

[stephentoub]

I actually see them get faster on my machine. Regardless, though, I'd rather keep the code smaller, especially with it triplicated due to the aggressive inlining.

[hypeartist]

One more possible improvement - using local valiables instead of out parameters:

privatestaticvoidMinMaxRgb(outintmin,outintmax,intr,intg,intb){intminLoc,maxLoc;if(r>g){maxLoc=r;minLoc=g;}else{maxLoc=g;minLoc=r;}if(b>maxLoc){maxLoc=b;}elseif(b<minLoc){minLoc=b;}max=maxLoc;min=minLoc;}

[L2]

I actually see them get faster on my machine. Regardless, though, I'd rather keep the code smaller, especially with it triplicated due to the aggressive inlining.

Sounds good @stephentoub , I've updated it with the requested changes. (I'm not sure how crediting works in this project as this is my second pull request, so I've added your username to the commit details as the originator of the simplification).

One more possible improvement - using local valiables instead of out parameters:

Thanks @hypeartist, I'm wondering if these local variables will be eliminated due to inlining, so performance will be about the same?

[stephentoub]

I'm not sure how crediting works in this project as this is my second pull request, so I've added your username to the commit details as the originator of the simplification

Thanks, but not necessary... I comment on a lot of PRs :)

[stephentoub]

I'm wondering if these local variables will be eliminated due to inlining, so performance will be about the same?

Yes, e.g.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gOgCUBXAOwwEt8YLAMIR8AB14AbGFADKMgG68wMXAG4aNYgGYmpBkIYBvGgzNNdvHgwCy5ABRWMDKGgZOGAczcfgASmNTc2CbKxtsBDZPYAcIDmcPfCs3OITrfAi3Vy83fw1qYODiAHYGDIQGODKrfOCAXyCzRot3axtSR2tsj29W539AgsKzUK5wyOiO1L7qrhT4mfKst1685qLS8srZ2vMGoaaDhgBtGxgMAAsIABMASXFJezPLm/uxSQB5MT4ILlwWACCnk8sFwuF4Chgty4kisVk8fgAus0xFAIdgMDAmOQkEwULYwhEojF7NNEskGGT0pkZt1rL1fH5miYjsFeAAzBj2KAMAB8XiZrPMLOGwy2AF4XLtReYklwGJLPNLCvsZTBJLgYOthdqxREFV5lTK5QaoEa9rr3Jz7MA+WUIoKZSKZbL9ZLgOazKrRerNVaubaADyzR2i50u2YGj2W71e5rNU7nK53B5PJOvB5fH5/QHA0HgyHQ2FceFIlFohQYrHMXHEfGjcbEqYLclzSkt6kIHxdbvOBnWfzMy2tgAyEDAbnKY7Anv93LtCMt4ZdU/HptnwTl04NSpjlt9WqFZmXxoi28VG9lVnPUr31GH1tt/NXYFDwxPopfUdnscKB7nQazNOb6FB+YrXmu7o/poR72hUkovrOJoIRBM7NPsdRAA===

[danmoseley]

Thanks for the contribution @L2 - perhaps you'd like to pick up something else?

[L2]

Fixes: #46153

…s instead of 4

• Replaces 2 calls each to Math.Min and Math.Max (a total of 4 required
  comparisons) with a single call to MinMaxRgb (at least 2 required
  comparisons and at worst 3).

[ghost]

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

Issue Details

---

…s instead of 4

• Replaces 2 calls each to Math.Min and Math.Max (a total of 4 required
  comparisons) with a single call to MinMaxRgb (at least 2 required
  comparisons and at worst 3).

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

[L2]

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)	Modality
System.Drawing.Tests.Perf_Color.GetHue	1.23	1756.53	1432.47
System.Drawing.Tests.Perf_Color.GetBrightness	1.09	1544.74	1415.17
System.Drawing.Tests.Perf_Color.GetSaturation	1.09	1602.51	1476.70

[iamcarbon]

Move trailing braces to their own lines for consistency with existing .NET code.

[L2]

Thanks, updated.

[danmoseley]

Do we have sufficient functional test coverage?

[L2]

Do we have sufficient functional test coverage?

Thanks @danmoseley . There were a couple of test cases missing which I've just added:

All Possible Cases:

label for existing test	case
(1)	r == g == b
(2)	r == g < b
(3)	r == g > b
(4)	r < g == b
(missing)	r < g < b
	r < g > b
(5)	where [r == b]
(missing)	where [r > b]
(missing)	where [r < b]
(7)	r > g == b
	r > g < b
(6)	where [r == b]
(missing)	where [r > b]
(missing)	where [r < b]
(missing)	r > g > b

Existing Test Coverage:

test label	case
(1)	[InlineData(0, 0, 0, 0f)]
	[InlineData(255, 255, 255, 1f)]
(7)	[InlineData(255, 0, 0, 0.5f)]
(5)	[InlineData(0, 255, 0, 0.5f)]
(2)	[InlineData(0, 0, 255, 0.5f)]
(3)	[InlineData(255, 255, 0, 0.5f)]
(6)	[InlineData(255, 0, 255, 0.5f)]
(4)	[InlineData(0, 255, 255, 0.5f)]
	[InlineData(51, 255, 255, 0.6f)]
	[InlineData(255, 51, 255, 0.6f)]
	[InlineData(255, 255, 51, 0.6f)]
	[InlineData(255, 51, 51, 0.6f)]
	[InlineData(51, 255, 51, 0.6f)]
	[InlineData(51, 51, 255, 0.6f)]
	[InlineData(51, 51, 51, 0.2f)]

For:

public void GetBrightness(int r, int g, int b, float expected)
{
Assert.Equal(expected, Color.FromArgb(r, g, b).GetBrightness());
}

[L2]

Also I wasn't sure if I should put this new method in Math.cs and make it a general use case to find the min and max of three values so I've left it inside Color.cs for now.

[danmoseley]

wasn't sure if I should put this new method in Math.cs

Drawing wouldn't be able to use anything from Math unless it was public: the libraries can only use each other's public surface. And to make new public API involves a review process, which would include consideration of how broadly useful the API would be. So I think it is in the right place.

[safern]

We could simplify this to use ? assignment?

Suggested change

	if(r<g)
	max=g;
	else
	max=r;
	max=r<g?g:r;

[L2]

Thanks, updated.

[safern]

Ditto:

Suggested change

	if(g<r)
	min=g;
	else
	min=r;
	min=g<r?g:r;

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Other than small style improvements, looks good. Thanks!

[L2]

Other than small style improvements, looks good. Thanks!

Thanks @safern , I've updated it with the requested changes.

[ghost]

Hello @safern!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[stephentoub]

This can be simplified to instead be:

Suggested change

	privatevoidMinMaxRgb(outintmin,outintmax,intr,intg,intb)
	{
	if(b<g)
	{
	if(b<r)
	{
	min=b;
	max=r<g?g:r;
	}
	else
	{
	min=r;
	max=g;
	}
	}
	else
	{
	if(r<b)
	{
	max=b;
	min=g<r?g:r;
	}
	else
	{
	max=r;
	min=g;
	}
	}
	}
	[MethodImpl(MethodImplOptions.AggressiveInlining)]
	privatestaticvoidMinMaxRgb(outintmin,outintmax,intr,intg,intb)
	{
	if(r>g)
	{
	max=r;
	min=g;
	}
	else
	{
	max=g;
	min=r;
	}
	if(b>max)
	{
	max=b;
	}
	elseif(b<min)
	{
	min=b;
	}
	}

[L2]

Thanks @stephentoub , I ran the perf_color microbenchmarks against 1) before this PR changeset, 2) the current PR's changeset, 3) proposed simplification

**.NET6 Before VS .NET6 with current PR changeset: Run1**

summary: better: 3, geomean: 1.151 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.23	1549.96	1259.50
System.Drawing.Tests.Perf_Color.GetHue	1.18	1753.24	1484.57
System.Drawing.Tests.Perf_Color.GetSaturation	1.05	1451.59	1383.79

**.NET6 Before VS .NET6 with current PR changeset: Run2**

summary: better: 3, geomean: 1.208 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.23	1543.19	1259.71
System.Drawing.Tests.Perf_Color.GetSaturation	1.22	1682.03	1384.17
System.Drawing.Tests.Perf_Color.GetHue	1.18	1758.68	1485.53

**.NET6 Before VS .NET6 with current PR changeset: Run3**

summary: better: 3, geomean: 1.189 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.GetSaturation	1.22	1682.42	1384.03
System.Drawing.Tests.Perf_Color.GetBrightness	1.19	1497.18	1259.28
System.Drawing.Tests.Perf_Color.GetHue	1.16	1725.25	1483.41

---

---

**.NET6 Before VS .NET6 with proposed simplification: Run1**

summary: better: 2, geomean: 1.123 total diff: 2

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.13	1549.96	1375.52
System.Drawing.Tests.Perf_Color.GetHue	1.12	1753.24	1565.75

**.NET6 Before VS .NET6 with proposed simplification: Run2**

summary: better: 3, geomean: 1.135 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.GetSaturation	1.16	1682.03	1448.45
System.Drawing.Tests.Perf_Color.GetHue	1.12	1758.68	1566.17
System.Drawing.Tests.Perf_Color.GetBrightness	1.12	1543.19	1375.17

**.NET6 Before VS .NET6 with proposed simplification: Run3**

summary: better: 2, geomean: 1.132 worse: 1, geomean: 1.050 total diff: 3

Slower	diff/base	Base Median (ns)	Diff Median (ns)	Modality
System.Drawing.Tests.Perf_Color.GetBrightness	1.05	1497.18	1572.36

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
System.Drawing.Tests.Perf_Color.GetSaturation	1.16	1682.42	1446.59
System.Drawing.Tests.Perf_Color.GetHue	1.10	1725.25	1565.42

---------------------- ----------------------

Some of the benchmarks seem to get a bit slower on average after the simplification. I think what is happening may be two causes:

1. There are paths with an extra store (we store max & min in the first conditional and then if the second conditional is taken one of these is overwritten).
2. Just my guess, but processor pipelining with branch prediction may find it easier to go through each of the conditionals in the current implementation as there are no dependencies. The simplification introduces a dependency on the second conditional as it needs the values from the first? Just a guess here.

I will follow yours and @safern 's recommendation on which to keep. Thanks again.

[KalleOlaviNiemitalo]

I didn't look at the disassembly but there might be an aliasing complication also, if the JIT compiler has to account for the possibility that min and max refer to the same location. Although perhaps it can disprove that after inlining.

[stephentoub]

I actually see them get faster on my machine. Regardless, though, I'd rather keep the code smaller, especially with it triplicated due to the aggressive inlining.

[hypeartist]

One more possible improvement - using local valiables instead of out parameters:

privatestaticvoidMinMaxRgb(outintmin,outintmax,intr,intg,intb){intminLoc,maxLoc;if(r>g){maxLoc=r;minLoc=g;}else{maxLoc=g;minLoc=r;}if(b>maxLoc){maxLoc=b;}elseif(b<minLoc){minLoc=b;}max=maxLoc;min=minLoc;}

[L2]

I actually see them get faster on my machine. Regardless, though, I'd rather keep the code smaller, especially with it triplicated due to the aggressive inlining.

Sounds good @stephentoub , I've updated it with the requested changes. (I'm not sure how crediting works in this project as this is my second pull request, so I've added your username to the commit details as the originator of the simplification).

One more possible improvement - using local valiables instead of out parameters:

Thanks @hypeartist, I'm wondering if these local variables will be eliminated due to inlining, so performance will be about the same?

[stephentoub]

I'm not sure how crediting works in this project as this is my second pull request, so I've added your username to the commit details as the originator of the simplification

Thanks, but not necessary... I comment on a lot of PRs :)

[stephentoub]

I'm wondering if these local variables will be eliminated due to inlining, so performance will be about the same?

Yes, e.g.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gOgCUBXAOwwEt8YLAMIR8AB14AbGFADKMgG68wMXAG4aNYgGYmpBkIYBvGgzNNdvHgwCy5ABRWMDKGgZOGAczcfgASmNTc2CbKxtsBDZPYAcIDmcPfCs3OITrfAi3Vy83fw1qYODiAHYGDIQGODKrfOCAXyCzRot3axtSR2tsj29W539AgsKzUK5wyOiO1L7qrhT4mfKst1685qLS8srZ2vMGoaaDhgBtGxgMAAsIABMASXFJezPLm/uxSQB5MT4ILlwWACCnk8sFwuF4Chgty4kisVk8fgAus0xFAIdgMDAmOQkEwULYwhEojF7NNEskGGT0pkZt1rL1fH5miYjsFeAAzBj2KAMAB8XiZrPMLOGwy2AF4XLtReYklwGJLPNLCvsZTBJLgYOthdqxREFV5lTK5QaoEa9rr3Jz7MA+WUIoKZSKZbL9ZLgOazKrRerNVaubaADyzR2i50u2YGj2W71e5rNU7nK53B5PJOvB5fH5/QHA0HgyHQ2FceFIlFohQYrHMXHEfGjcbEqYLclzSkt6kIHxdbvOBnWfzMy2tgAyEDAbnKY7Anv93LtCMt4ZdU/HptnwTl04NSpjlt9WqFZmXxoi28VG9lVnPUr31GH1tt/NXYFDwxPopfUdnscKB7nQazNOb6FB+YrXmu7o/poR72hUkovrOJoIRBM7NPsdRAA===

[danmoseley]

Thanks for the contribution @L2 - perhaps you'd like to pick up something else?

[L2]

Fixes: #46153

…s instead of 4

• Replaces 2 calls each to Math.Min and Math.Max (a total of 4 required
  comparisons) with a single call to MinMaxRgb (at least 2 required
  comparisons and at worst 3).

[ghost]

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

Issue Details

---

…s instead of 4

• Replaces 2 calls each to Math.Min and Math.Max (a total of 4 required
  comparisons) with a single call to MinMaxRgb (at least 2 required
  comparisons and at worst 3).

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

[L2]

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)	Modality
System.Drawing.Tests.Perf_Color.GetHue	1.23	1756.53	1432.47
System.Drawing.Tests.Perf_Color.GetBrightness	1.09	1544.74	1415.17
System.Drawing.Tests.Perf_Color.GetSaturation	1.09	1602.51	1476.70

[iamcarbon]

Move trailing braces to their own lines for consistency with existing .NET code.

[L2]

Thanks, updated.

[danmoseley]

Do we have sufficient functional test coverage?

[L2]

Do we have sufficient functional test coverage?

Thanks @danmoseley . There were a couple of test cases missing which I've just added:

All Possible Cases:

label for existing test	case
(1)	r == g == b
(2)	r == g < b
(3)	r == g > b
(4)	r < g == b
(missing)	r < g < b
	r < g > b
(5)	where [r == b]
(missing)	where [r > b]
(missing)	where [r < b]
(7)	r > g == b
	r > g < b
(6)	where [r == b]
(missing)	where [r > b]
(missing)	where [r < b]
(missing)	r > g > b

Existing Test Coverage:

test label	case
(1)	[InlineData(0, 0, 0, 0f)]
	[InlineData(255, 255, 255, 1f)]
(7)	[InlineData(255, 0, 0, 0.5f)]
(5)	[InlineData(0, 255, 0, 0.5f)]
(2)	[InlineData(0, 0, 255, 0.5f)]
(3)	[InlineData(255, 255, 0, 0.5f)]
(6)	[InlineData(255, 0, 255, 0.5f)]
(4)	[InlineData(0, 255, 255, 0.5f)]
	[InlineData(51, 255, 255, 0.6f)]
	[InlineData(255, 51, 255, 0.6f)]
	[InlineData(255, 255, 51, 0.6f)]
	[InlineData(255, 51, 51, 0.6f)]
	[InlineData(51, 255, 51, 0.6f)]
	[InlineData(51, 51, 255, 0.6f)]
	[InlineData(51, 51, 51, 0.2f)]

For:

public void GetBrightness(int r, int g, int b, float expected)
{
Assert.Equal(expected, Color.FromArgb(r, g, b).GetBrightness());
}

[L2]

Also I wasn't sure if I should put this new method in Math.cs and make it a general use case to find the min and max of three values so I've left it inside Color.cs for now.

[danmoseley]

wasn't sure if I should put this new method in Math.cs

Drawing wouldn't be able to use anything from Math unless it was public: the libraries can only use each other's public surface. And to make new public API involves a review process, which would include consideration of how broadly useful the API would be. So I think it is in the right place.

[safern]

We could simplify this to use ? assignment?

Suggested change

	if(r<g)
	max=g;
	else
	max=r;
	max=r<g?g:r;

[L2]

Thanks, updated.

[safern]

Ditto:

Suggested change

	if(g<r)
	min=g;
	else
	min=r;
	min=g<r?g:r;

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Other than small style improvements, looks good. Thanks!

[L2]

Other than small style improvements, looks good. Thanks!

Thanks @safern , I've updated it with the requested changes.

[ghost]

Hello @safern!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[stephentoub]

This can be simplified to instead be:

Suggested change

	privatevoidMinMaxRgb(outintmin,outintmax,intr,intg,intb)
	{
	if(b<g)
	{
	if(b<r)
	{
	min=b;
	max=r<g?g:r;
	}
	else
	{
	min=r;
	max=g;
	}
	}
	else
	{
	if(r<b)
	{
	max=b;
	min=g<r?g:r;
	}
	else
	{
	max=r;
	min=g;
	}
	}
	}
	[MethodImpl(MethodImplOptions.AggressiveInlining)]
	privatestaticvoidMinMaxRgb(outintmin,outintmax,intr,intg,intb)
	{
	if(r>g)
	{
	max=r;
	min=g;
	}
	else
	{
	max=g;
	min=r;
	}
	if(b>max)
	{
	max=b;
	}
	elseif(b<min)
	{
	min=b;
	}
	}

[L2]

Thanks @stephentoub , I ran the perf_color microbenchmarks against 1) before this PR changeset, 2) the current PR's changeset, 3) proposed simplification

**.NET6 Before VS .NET6 with current PR changeset: Run1**

summary: better: 3, geomean: 1.151 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.23	1549.96	1259.50
System.Drawing.Tests.Perf_Color.GetHue	1.18	1753.24	1484.57
System.Drawing.Tests.Perf_Color.GetSaturation	1.05	1451.59	1383.79

**.NET6 Before VS .NET6 with current PR changeset: Run2**

summary: better: 3, geomean: 1.208 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.23	1543.19	1259.71
System.Drawing.Tests.Perf_Color.GetSaturation	1.22	1682.03	1384.17
System.Drawing.Tests.Perf_Color.GetHue	1.18	1758.68	1485.53

**.NET6 Before VS .NET6 with current PR changeset: Run3**

summary: better: 3, geomean: 1.189 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.GetSaturation	1.22	1682.42	1384.03
System.Drawing.Tests.Perf_Color.GetBrightness	1.19	1497.18	1259.28
System.Drawing.Tests.Perf_Color.GetHue	1.16	1725.25	1483.41

---

---

**.NET6 Before VS .NET6 with proposed simplification: Run1**

summary: better: 2, geomean: 1.123 total diff: 2

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.13	1549.96	1375.52
System.Drawing.Tests.Perf_Color.GetHue	1.12	1753.24	1565.75

**.NET6 Before VS .NET6 with proposed simplification: Run2**

summary: better: 3, geomean: 1.135 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.GetSaturation	1.16	1682.03	1448.45
System.Drawing.Tests.Perf_Color.GetHue	1.12	1758.68	1566.17
System.Drawing.Tests.Perf_Color.GetBrightness	1.12	1543.19	1375.17

**.NET6 Before VS .NET6 with proposed simplification: Run3**

summary: better: 2, geomean: 1.132 worse: 1, geomean: 1.050 total diff: 3

Slower	diff/base	Base Median (ns)	Diff Median (ns)	Modality
System.Drawing.Tests.Perf_Color.GetBrightness	1.05	1497.18	1572.36

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
System.Drawing.Tests.Perf_Color.GetSaturation	1.16	1682.42	1446.59
System.Drawing.Tests.Perf_Color.GetHue	1.10	1725.25	1565.42

---------------------- ----------------------

Some of the benchmarks seem to get a bit slower on average after the simplification. I think what is happening may be two causes:

1. There are paths with an extra store (we store max & min in the first conditional and then if the second conditional is taken one of these is overwritten).
2. Just my guess, but processor pipelining with branch prediction may find it easier to go through each of the conditionals in the current implementation as there are no dependencies. The simplification introduces a dependency on the second conditional as it needs the values from the first? Just a guess here.

I will follow yours and @safern 's recommendation on which to keep. Thanks again.

[KalleOlaviNiemitalo]

I didn't look at the disassembly but there might be an aliasing complication also, if the JIT compiler has to account for the possibility that min and max refer to the same location. Although perhaps it can disprove that after inlining.

[stephentoub]

I actually see them get faster on my machine. Regardless, though, I'd rather keep the code smaller, especially with it triplicated due to the aggressive inlining.

[hypeartist]

One more possible improvement - using local valiables instead of out parameters:

privatestaticvoidMinMaxRgb(outintmin,outintmax,intr,intg,intb){intminLoc,maxLoc;if(r>g){maxLoc=r;minLoc=g;}else{maxLoc=g;minLoc=r;}if(b>maxLoc){maxLoc=b;}elseif(b<minLoc){minLoc=b;}max=maxLoc;min=minLoc;}

[L2]

I actually see them get faster on my machine. Regardless, though, I'd rather keep the code smaller, especially with it triplicated due to the aggressive inlining.

Sounds good @stephentoub , I've updated it with the requested changes. (I'm not sure how crediting works in this project as this is my second pull request, so I've added your username to the commit details as the originator of the simplification).

One more possible improvement - using local valiables instead of out parameters:

Thanks @hypeartist, I'm wondering if these local variables will be eliminated due to inlining, so performance will be about the same?

[stephentoub]

I'm not sure how crediting works in this project as this is my second pull request, so I've added your username to the commit details as the originator of the simplification

Thanks, but not necessary... I comment on a lot of PRs :)

[stephentoub]

I'm wondering if these local variables will be eliminated due to inlining, so performance will be about the same?

Yes, e.g.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gOgCUBXAOwwEt8YLAMIR8AB14AbGFADKMgG68wMXAG4aNYgGYmpBkIYBvGgzNNdvHgwCy5ABRWMDKGgZOGAczcfgASmNTc2CbKxtsBDZPYAcIDmcPfCs3OITrfAi3Vy83fw1qYODiAHYGDIQGODKrfOCAXyCzRot3axtSR2tsj29W539AgsKzUK5wyOiO1L7qrhT4mfKst1685qLS8srZ2vMGoaaDhgBtGxgMAAsIABMASXFJezPLm/uxSQB5MT4ILlwWACCnk8sFwuF4Chgty4kisVk8fgAus0xFAIdgMDAmOQkEwULYwhEojF7NNEskGGT0pkZt1rL1fH5miYjsFeAAzBj2KAMAB8XiZrPMLOGwy2AF4XLtReYklwGJLPNLCvsZTBJLgYOthdqxREFV5lTK5QaoEa9rr3Jz7MA+WUIoKZSKZbL9ZLgOazKrRerNVaubaADyzR2i50u2YGj2W71e5rNU7nK53B5PJOvB5fH5/QHA0HgyHQ2FceFIlFohQYrHMXHEfGjcbEqYLclzSkt6kIHxdbvOBnWfzMy2tgAyEDAbnKY7Anv93LtCMt4ZdU/HptnwTl04NSpjlt9WqFZmXxoi28VG9lVnPUr31GH1tt/NXYFDwxPopfUdnscKB7nQazNOb6FB+YrXmu7o/poR72hUkovrOJoIRBM7NPsdRAA===

[danmoseley]

Thanks for the contribution @L2 - perhaps you'd like to pick up something else?

[L2]

Fixes: #46153

…s instead of 4

• Replaces 2 calls each to Math.Min and Math.Max (a total of 4 required
  comparisons) with a single call to MinMaxRgb (at least 2 required
  comparisons and at worst 3).

[ghost]

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

Issue Details

---

…s instead of 4

• Replaces 2 calls each to Math.Min and Math.Max (a total of 4 required
  comparisons) with a single call to MinMaxRgb (at least 2 required
  comparisons and at worst 3).

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

[L2]

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)	Modality
System.Drawing.Tests.Perf_Color.GetHue	1.23	1756.53	1432.47
System.Drawing.Tests.Perf_Color.GetBrightness	1.09	1544.74	1415.17
System.Drawing.Tests.Perf_Color.GetSaturation	1.09	1602.51	1476.70

[iamcarbon]

Move trailing braces to their own lines for consistency with existing .NET code.

[L2]

Thanks, updated.

[danmoseley]

Do we have sufficient functional test coverage?

[L2]

Do we have sufficient functional test coverage?

Thanks @danmoseley . There were a couple of test cases missing which I've just added:

All Possible Cases:

label for existing test	case
(1)	r == g == b
(2)	r == g < b
(3)	r == g > b
(4)	r < g == b
(missing)	r < g < b
	r < g > b
(5)	where [r == b]
(missing)	where [r > b]
(missing)	where [r < b]
(7)	r > g == b
	r > g < b
(6)	where [r == b]
(missing)	where [r > b]
(missing)	where [r < b]
(missing)	r > g > b

Existing Test Coverage:

test label	case
(1)	[InlineData(0, 0, 0, 0f)]
	[InlineData(255, 255, 255, 1f)]
(7)	[InlineData(255, 0, 0, 0.5f)]
(5)	[InlineData(0, 255, 0, 0.5f)]
(2)	[InlineData(0, 0, 255, 0.5f)]
(3)	[InlineData(255, 255, 0, 0.5f)]
(6)	[InlineData(255, 0, 255, 0.5f)]
(4)	[InlineData(0, 255, 255, 0.5f)]
	[InlineData(51, 255, 255, 0.6f)]
	[InlineData(255, 51, 255, 0.6f)]
	[InlineData(255, 255, 51, 0.6f)]
	[InlineData(255, 51, 51, 0.6f)]
	[InlineData(51, 255, 51, 0.6f)]
	[InlineData(51, 51, 255, 0.6f)]
	[InlineData(51, 51, 51, 0.2f)]

For:

public void GetBrightness(int r, int g, int b, float expected)
{
Assert.Equal(expected, Color.FromArgb(r, g, b).GetBrightness());
}

[L2]

Also I wasn't sure if I should put this new method in Math.cs and make it a general use case to find the min and max of three values so I've left it inside Color.cs for now.

[danmoseley]

wasn't sure if I should put this new method in Math.cs

Drawing wouldn't be able to use anything from Math unless it was public: the libraries can only use each other's public surface. And to make new public API involves a review process, which would include consideration of how broadly useful the API would be. So I think it is in the right place.

[safern]

We could simplify this to use ? assignment?

Suggested change

	if(r<g)
	max=g;
	else
	max=r;
	max=r<g?g:r;

[L2]

Thanks, updated.

[safern]

Ditto:

Suggested change

	if(g<r)
	min=g;
	else
	min=r;
	min=g<r?g:r;

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Suggested change

	intmin,max;
	MinMaxRgb(outmin,outmax,r,g,b);
	MinMaxRgb(outintmin,outintmax,r,g,b);

[L2]

Thanks, updated.

[safern]

Other than small style improvements, looks good. Thanks!

[L2]

Other than small style improvements, looks good. Thanks!

Thanks @safern , I've updated it with the requested changes.

[ghost]

Hello @safern!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[stephentoub]

This can be simplified to instead be:

Suggested change

	privatevoidMinMaxRgb(outintmin,outintmax,intr,intg,intb)
	{
	if(b<g)
	{
	if(b<r)
	{
	min=b;
	max=r<g?g:r;
	}
	else
	{
	min=r;
	max=g;
	}
	}
	else
	{
	if(r<b)
	{
	max=b;
	min=g<r?g:r;
	}
	else
	{
	max=r;
	min=g;
	}
	}
	}
	[MethodImpl(MethodImplOptions.AggressiveInlining)]
	privatestaticvoidMinMaxRgb(outintmin,outintmax,intr,intg,intb)
	{
	if(r>g)
	{
	max=r;
	min=g;
	}
	else
	{
	max=g;
	min=r;
	}
	if(b>max)
	{
	max=b;
	}
	elseif(b<min)
	{
	min=b;
	}
	}

[L2]

Thanks @stephentoub , I ran the perf_color microbenchmarks against 1) before this PR changeset, 2) the current PR's changeset, 3) proposed simplification

**.NET6 Before VS .NET6 with current PR changeset: Run1**

summary: better: 3, geomean: 1.151 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.23	1549.96	1259.50
System.Drawing.Tests.Perf_Color.GetHue	1.18	1753.24	1484.57
System.Drawing.Tests.Perf_Color.GetSaturation	1.05	1451.59	1383.79

**.NET6 Before VS .NET6 with current PR changeset: Run2**

summary: better: 3, geomean: 1.208 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.23	1543.19	1259.71
System.Drawing.Tests.Perf_Color.GetSaturation	1.22	1682.03	1384.17
System.Drawing.Tests.Perf_Color.GetHue	1.18	1758.68	1485.53

**.NET6 Before VS .NET6 with current PR changeset: Run3**

summary: better: 3, geomean: 1.189 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.GetSaturation	1.22	1682.42	1384.03
System.Drawing.Tests.Perf_Color.GetBrightness	1.19	1497.18	1259.28
System.Drawing.Tests.Perf_Color.GetHue	1.16	1725.25	1483.41

---

---

**.NET6 Before VS .NET6 with proposed simplification: Run1**

summary: better: 2, geomean: 1.123 total diff: 2

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.13	1549.96	1375.52
System.Drawing.Tests.Perf_Color.GetHue	1.12	1753.24	1565.75

**.NET6 Before VS .NET6 with proposed simplification: Run2**

summary: better: 3, geomean: 1.135 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.GetSaturation	1.16	1682.03	1448.45
System.Drawing.Tests.Perf_Color.GetHue	1.12	1758.68	1566.17
System.Drawing.Tests.Perf_Color.GetBrightness	1.12	1543.19	1375.17

**.NET6 Before VS .NET6 with proposed simplification: Run3**

summary: better: 2, geomean: 1.132 worse: 1, geomean: 1.050 total diff: 3

Slower	diff/base	Base Median (ns)	Diff Median (ns)	Modality
System.Drawing.Tests.Perf_Color.GetBrightness	1.05	1497.18	1572.36

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
System.Drawing.Tests.Perf_Color.GetSaturation	1.16	1682.42	1446.59
System.Drawing.Tests.Perf_Color.GetHue	1.10	1725.25	1565.42

---------------------- ----------------------

Some of the benchmarks seem to get a bit slower on average after the simplification. I think what is happening may be two causes:

1. There are paths with an extra store (we store max & min in the first conditional and then if the second conditional is taken one of these is overwritten).
2. Just my guess, but processor pipelining with branch prediction may find it easier to go through each of the conditionals in the current implementation as there are no dependencies. The simplification introduces a dependency on the second conditional as it needs the values from the first? Just a guess here.

I will follow yours and @safern 's recommendation on which to keep. Thanks again.

[KalleOlaviNiemitalo]

I didn't look at the disassembly but there might be an aliasing complication also, if the JIT compiler has to account for the possibility that min and max refer to the same location. Although perhaps it can disprove that after inlining.

[stephentoub]

I actually see them get faster on my machine. Regardless, though, I'd rather keep the code smaller, especially with it triplicated due to the aggressive inlining.

[hypeartist]

One more possible improvement - using local valiables instead of out parameters:

privatestaticvoidMinMaxRgb(outintmin,outintmax,intr,intg,intb){intminLoc,maxLoc;if(r>g){maxLoc=r;minLoc=g;}else{maxLoc=g;minLoc=r;}if(b>maxLoc){maxLoc=b;}elseif(b<minLoc){minLoc=b;}max=maxLoc;min=minLoc;}

[L2]

I actually see them get faster on my machine. Regardless, though, I'd rather keep the code smaller, especially with it triplicated due to the aggressive inlining.

Sounds good @stephentoub , I've updated it with the requested changes. (I'm not sure how crediting works in this project as this is my second pull request, so I've added your username to the commit details as the originator of the simplification).

One more possible improvement - using local valiables instead of out parameters:

Thanks @hypeartist, I'm wondering if these local variables will be eliminated due to inlining, so performance will be about the same?

[stephentoub]

I'm not sure how crediting works in this project as this is my second pull request, so I've added your username to the commit details as the originator of the simplification

Thanks, but not necessary... I comment on a lot of PRs :)

[stephentoub]

I'm wondering if these local variables will be eliminated due to inlining, so performance will be about the same?

Yes, e.g.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gOgCUBXAOwwEt8YLAMIR8AB14AbGFADKMgG68wMXAG4aNYgGYmpBkIYBvGgzNNdvHgwCy5ABRWMDKGgZOGAczcfgASmNTc2CbKxtsBDZPYAcIDmcPfCs3OITrfAi3Vy83fw1qYODiAHYGDIQGODKrfOCAXyCzRot3axtSR2tsj29W539AgsKzUK5wyOiO1L7qrhT4mfKst1685qLS8srZ2vMGoaaDhgBtGxgMAAsIABMASXFJezPLm/uxSQB5MT4ILlwWACCnk8sFwuF4Chgty4kisVk8fgAus0xFAIdgMDAmOQkEwULYwhEojF7NNEskGGT0pkZt1rL1fH5miYjsFeAAzBj2KAMAB8XiZrPMLOGwy2AF4XLtReYklwGJLPNLCvsZTBJLgYOthdqxREFV5lTK5QaoEa9rr3Jz7MA+WUIoKZSKZbL9ZLgOazKrRerNVaubaADyzR2i50u2YGj2W71e5rNU7nK53B5PJOvB5fH5/QHA0HgyHQ2FceFIlFohQYrHMXHEfGjcbEqYLclzSkt6kIHxdbvOBnWfzMy2tgAyEDAbnKY7Anv93LtCMt4ZdU/HptnwTl04NSpjlt9WqFZmXxoi28VG9lVnPUr31GH1tt/NXYFDwxPopfUdnscKB7nQazNOb6FB+YrXmu7o/poR72hUkovrOJoIRBM7NPsdRAA===

[danmoseley]

Thanks for the contribution @L2 - perhaps you'd like to pick up something else?