Fix blittable array instead of copying #30099

JeremyKuhne · 2018-06-03T00:03:46Z

System.Drawing is doing a lot of unnecessary work. This is a start on trying to remove some of it. This change is meant to show the general pattern I intend to follow.

Fixes the managed array and uses it directly instead of copying multiple times
Adds some basic sanity tests to validate that we're getting the expected output
Splits existing negative bezier tests into new file
Adds a performance project

Output tests were generated and run before making the functional change.

JeremyKuhne · 2018-06-03T00:06:51Z

Drawing Beziers before:

  System.Drawing.Common.Performance.Tests.dll                      | Metric                                        | Unit  | Iterations |    Average | STDEV.S |        Min |        Max
  :---------------------------------------------------------------- |:--------------------------------------------- |:-----:|:----------:| ----------:| -------:| ----------:| ----------:
   System.Drawing.Tests.Perf_Graphics_DrawBeziers.DrawBezier_Points | Duration                                      | msec  |    100     |     38.219 |   0.565 |     36.993 |     40.016
   System.Drawing.Tests.Perf_Graphics_DrawBeziers.DrawBezier_Points | Allocation Size on Benchmark Execution Thread | bytes |    100     | 1.680E+006 |   0.000 | 1.680E+006 | 1.680E+006

After:

  System.Drawing.Common.Performance.Tests.dll                      | Metric                                        | Unit  | Iterations | Average | STDEV.S |    Min |    Max
  :---------------------------------------------------------------- |:--------------------------------------------- |:-----:|:----------:| -------:| -------:| ------:| ------:
   System.Drawing.Tests.Perf_Graphics_DrawBeziers.DrawBezier_Points | Duration                                      | msec  |    100     |  34.866 |   0.980 | 33.530 | 40.966
   System.Drawing.Tests.Perf_Graphics_DrawBeziers.DrawBezier_Points | Allocation Size on Benchmark Execution Thread | bytes |    100     |   0.000 |   0.000 |  0.000 |  0.000

stephentoub · 2018-06-03T00:17:23Z

src/System.Drawing.Common/src/System/Drawing/Graphics.Windows.cs

                throw new ArgumentNullException(nameof(pen));
-            }


I'm not sure why these braces are being removed.

Not needed per our guidelines, pulls the argument validation in a bit.

But also at the cost of making the code bigger in collapsed view 😉

But also at the cost of making the code bigger in collapsed view

I'd call it a plus that you don't have to expand for one line.

It really comes down to personal preference 😄 . For example, I find the native code in CoreCLR significantly harder to navigate because of all the one liners that don't have braces.

the style guidelines don't say that this new version is preferred

Perhaps they should so we don't rathole on nits...

it's not actually fixing anything, just changing it to your preference

The diff isn't hard to follow. I think I'm entitled to cleaning up the methods I'm changing here for style consistency that I prefer and that I personally think makes an improvement in readability. I've made these methods concise & clean and made a major improvement. I'll end up changing most of the file, so Jeremy's style will be predominant in this one file. And if you look at the other changes I've made outside of this one, you'll see my style is predominant in the other files in this project. Shouldn't there be an argument for dll consistency as well?

the next person who comes along to edit this file might change it back

If someone else comes in and makes these methods even better than I did (improves them functionally), more power to them- I think they've earned the right to make a minor style tweak in their favor.

Quite frankly this is really disheartening. I've made a big improvement on the way to even more big improvements and the key feedback I've gotten is a long debate over whether I should have removed an unnecessary set of brackets.

All that said, I do understand the intent of the rules and your concerns. I do think about the impact of what I do under those specific criteria (among others) and I try to exercise my best judgement in balancing trade-offs. I think I've chosen well here, or at least well enough that further arguments for alternatives are just niggling.

Quite frankly this is really disheartening. I've made a big improvement on the way to even more big improvements and the key feedback I've gotten is a long debate over whether I should have removed an unnecessary set of brackets.

I'm sorry it's disheartening. That's certainly not the intent. I provided feedback on everything I had to provide feedback on: this, validating the HandleRef usage, and questioning the interop. Your original change (prior to updating it after questions) was only a few lines of product src, so not a ton to comment on, and this style change was a large portion of the diff. You chose to push back on the feedback, from multiple people. When I've received such feedback, I've accepted it and moved on.

@JeremyKuhne Feedback similar to what you got here was given on number of occasions, by multiple different reviewers. We are just being consistent here. For example, here is my comment where I am asking Ashon to submit formatting changes separately from actual code changes: dotnet/coreclr#16223 (comment)

It is fine with me if you plan to make significant improvements in these files, want to claim their ownership and change them to your liking. Such style changes should be done in separate PR, and ideally the entire files should be changed to have consistent style, not just a few functions. Here are examples of PRs where component owners are changing the code to their liking #22471 or dotnet/coreclr#17451

@JeremyKuhne I suggest moving formatting changes in a separate PR. You're right that this code could be better formatted but reviewers are asking not to do it in this change.

We are just being consistent here. For example, here is my comment where I am asking Ashon to submit formatting changes separately from actual code changes

And in that comment you explicitly said to restrict formatting changes to the areas where you're actually changing code. Which I agree with fully and make every attempt to do and I believe I've done in this case. (And I've pushed others to do.)

It is fine with me if you plan to make significant improvements in these files, want to claim their ownership and change them to your liking. Such style changes should be done in separate PR, and ideally the entire files should be changed to have consistent style, not just a few functions.

I'm happy to do a follow up that makes a pass through the file(s) for consistency.

This caught me off guard. This is the first time I've been tagged for changing the formatting of the code local to what I'm working on (honestly, it is just one line away from "real" changes). It really bothers me as it seems super nitpicky/dogmatic- did you really have a hard time understanding what changed? To me I can't see how it is in the least bit confusing- perhaps I just don't have the right perspective.

I'll put back the brackets. Hopefully my time explaining my perspective isn't wasted. I think we need to be careful about dogma and it worries me that we make things unnecessarily difficult.

stephentoub · 2018-06-03T01:35:07Z

src/System.Drawing.Common/src/System/Drawing/Graphics.Windows.cs

+                int status = SafeNativeMethods.Gdip.GdipDrawBeziersI(
+                    new HandleRef(this, NativeGraphics),
+                    new HandleRef(pen, pen.NativePen),
+                    new HandleRef(this, new IntPtr(p)), points.Length);


Do you plan to also address use of HandleRef? That's an old pattern prior to SafeHandles.

Do you plan to also address use of HandleRef?

I'm cognizant about it. I'm not sure that I'll deal with it myself, but I'll at least open a follow-up issue.

I'll at least clean out the HandleRef around the pinned arrays in the initial work.

stephentoub · 2018-06-03T01:37:27Z

src/System.Drawing.Common/src/System/Drawing/Graphics.Windows.cs


-            IntPtr buf = SafeNativeMethods.Gdip.ConvertPointToMemory(points);
-            try
+            fixed (Point* p = points)


It looks like ConvertPointToMemory is doing some complicated marshaling. That's all unnecessary, and Point is blittable and matching the corresponding native types in layout?

Yes, Point matches the native type and is blittable. I'm not sure how they stumbled into that loop originally.

The same pattern is used at about 60 places. It would be nice to get all of them fixed up.

The same pattern is used at about 60 places.

Yep. Following up on it right now. Just wanted to get an initial change up to show the general pattern of what I'm doing.

JeremyKuhne · 2018-06-03T01:48:23Z

@dotnet-bot test OSX x64 Debug Build

Failed with #29940

danmoseley · 2018-06-03T03:19:39Z

src/System.Drawing.Common/tests/Graphics_DrawBezierTests.cs

+            using (var image = new Bitmap(10, 10))
+            using (Graphics graphics = Graphics.FromImage(image))
+            using (var pen = new Pen(Color.Red))
+            {


This hdc ought to have a disposable wrapper perhaps

The finally releases it, but it might be nice to add a helper.

JeremyKuhne · 2018-06-03T05:55:08Z

Did another update:

Converted more usages
Stopped wrapping the pointers
Fully removed some of the helpers

Should be enough to see the pattern pretty clearly. There is a lot left to remove, but this is a good start I think. Significant cut in memory usage and should get at least 10% better performance with methods that work with arrays. That gain will get bigger the larger the arrays are.

JeremyKuhne · 2018-06-03T18:51:29Z

On the upper end, transforming points is wayyy faster (26X).

Before:

   System.Drawing.Common.Performance.Tests.dll                          | Metric                                        | Unit  | Iterations |    Average | STDEV.S |        Min |        Max
  :-------------------------------------------------------------------- |:--------------------------------------------- |:-----:|:----------:| ----------:| -------:| ----------:| ----------:
   System.Drawing.Tests.Perf_Graphics_TransformPoints.Transform_Points  | Duration                                      | msec  |    100     |     94.878 |   5.830 |     91.804 |    140.345
   System.Drawing.Tests.Perf_Graphics_TransformPoints.Transform_Points  | Allocation Size on Benchmark Execution Thread | bytes |    100     | 1.248E+007 |   0.000 | 1.248E+007 | 1.248E+007

After:

   System.Drawing.Common.Performance.Tests.dll                          | Metric                                        | Unit  | Iterations | Average | STDEV.S |    Min |    Max
  :-------------------------------------------------------------------- |:--------------------------------------------- |:-----:|:----------:| -------:| -------:| ------:| ------:
   System.Drawing.Tests.Perf_Graphics_TransformPoints.Transform_Points  | Duration                                      | msec  |    100     |   3.584 |   1.129 |  3.181 | 13.747
   System.Drawing.Tests.Perf_Graphics_TransformPoints.Transform_Points  | Allocation Size on Benchmark Execution Thread | bytes |    100     |   0.000 |   0.000 |  0.000 |  0.000

We're riddled with this sort of overhead in our oldest UI stacks. I hope to change that. 😜

stephentoub · 2018-06-04T01:04:57Z

src/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPath.Windows.cs


-            PathData pathData = new PathData() { Types = new byte[numPts] };
+            if (count == 0)
+                return pathData;


How common is this case? If common, consider moving it earlier and doing:

if (count == 0) { return new PathData() { Types = Array.Empty<byte>(), Points = Array.Empty<PointF>() }; }

Not common.

stephentoub · 2018-06-04T01:07:45Z

src/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPath.Windows.cs

                {
-                    typesHandle.Free();
+                    throw SafeNativeMethods.Gdip.StatusException(status);


There's a helper for this:

SafeNativeMethods.Gdip.CheckStatus(status);

corefx/src/System.Drawing.Common/src/System/Drawing/Gdiplus.cs

Lines 261 to 267 in b5b75ee

internal static void CheckStatus(int status)

{

if (status != Ok)

{

throw StatusException(status);

}

}

Yes, I plan to rationalize out the variety of helpers in a separate change.

This is a start on trying to remove some of it. This change is meant to show the general pattern I intend to follow. - Fixes the managed array and uses it directly instead of copying multiple times - Adds some basic sanity tests to validate that we're getting the expected output - Splits existing negative tests into new files with sanity tests - Adds a performance project Output tests were generated and run before making the functional change.

JeremyKuhne · 2018-06-04T06:29:27Z

I've put back brackets around the if statements where I made no other change to said if statements.

safern · 2018-06-04T17:27:59Z

src/System.Drawing.Common/src/System/Drawing/Drawing2D/Matrix.cs


-            try
+            fixed(PointF* p = pts)


Nit: space before (

safern · 2018-06-04T17:29:26Z

src/System.Drawing.Common/src/System.Drawing.Common.csproj

@@ -378,5 +378,8 @@
    <Reference Include="System" />
    <Reference Include="System.Drawing" />
  </ItemGroup>
+  <ItemGroup>
+    <Compile Include="System\Drawing\Internal\GpPathData.cs" />


If this is a Windows specific could you move it up to the ItemGroup conditioned to TargetWindows L207? if we plan to use this in Unix code as well, could you please move it up the the common ItemGroup L29?

safern · 2018-06-06T02:14:49Z

src/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPath.Windows.cs

-                IntPtr buf = Marshal.AllocHGlobal(checked(count * size));
-                try
+                PointF[] points = new PointF[PointCount];
+                fixed(PointF* p = points)


Nit: space before (

D'oh, I searched and didn't click on this result. I'll get it in the next change.

safern · 2018-06-06T02:19:38Z

src/System.Drawing.Common/src/System/Drawing/Drawing2D/Matrix.cs

-            IntPtr buf = SafeNativeMethods.Gdip.ConvertPointToMemory(pts);
-
-            try
+            fixed(Point* p = pts)


Nit: space before (

safern · 2018-06-06T02:21:47Z

src/System.Drawing.Common/tests/DrawingTest.cs

+
+namespace System.Drawing.Tests
+{
+    public abstract class DrawingTest


Should we name this DrawingTestBase?

I haven't used Base in other assemblies.

safern · 2018-06-06T02:28:26Z

src/System.Drawing.Common/tests/Performance/Configurations.props

+<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <PropertyGroup>
+    <BuildConfigurations>
+      netstandard;


Should we run the perf tests on netcoreapp only? The netstandard implementation is a turd assembly. In fact the regular tests only have a netcoreapp and desktop configurations

Do we run them anywhere else? I was just copy/pasting from FileSystem. If we can/should specify netcoreapp I'm happy to change it.

I’m don’t think we yet do but if in the future we start it might cause pain to whoever brings the infra to do so, but I can live with this being NS

Tanya-Solyanik · 2018-06-06T05:35:29Z

src/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPathIterator.cs

            {
-                int status = SafeNativeMethods.Gdip.GdipPathIterEnumerate(new HandleRef(this, nativeIter), out resultCount,


Would be nice to rename nativeIter to nativeIterator, or something like this, perhaps not in the scope of this change.

I'm going to follow up this change with a style pass. I'll look at private/internal members.

safern

LGTM. Just a missing space after a fixed statement.

I would like to also use this in the Unix code if possible in the future and I think there is a lot that can be improved in the Drawing area. Thanks for starting this.

Tanya-Solyanik · 2018-06-06T05:52:20Z

src/System.Drawing.Common/tests/Graphics_DrawBezierTests.cs

+
+                graphics.DrawBeziers(pen, points);
+                ValidateImageContent(image,
+                    PlatformDetection.IsWindows


Just curious, why is IsWindows needed if this test is already conditioned on GDI+ availability? What is the other platform?

The check for GDI+ availability is also checking for the mono version of GDI+, which we use on Unix. The implementation isn't exactly the same and has some gaps.

filipnavara · 2019-02-21T22:51:39Z

src/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPath.Windows.cs

+                    Count = count,
+                    Points = p,
+                    Types = t
+                };


Doesn't this have to be fixed (...) as well? (or is it guaranteed to be on stack and hence not moving?)

or is it guaranteed to be on stack and hence not moving

Correct. The language won't even let you use fixed here (assuming you're referring to data); it'll produce an error something about not letting you use fixed with an already fixed expression.

* System.Drawing is doing a lot of unnecessary work. This is a start on trying to remove some of it. This change is meant to show the general pattern I intend to follow. - Fixes the managed array and uses it directly instead of copying multiple times - Adds some basic sanity tests to validate that we're getting the expected output - Splits existing negative tests into new files with sanity tests - Adds a performance project Output tests were generated and run before making the functional change. * Address feedback * Add space after fixed statements. Commit migrated from dotnet/corefx@ba496ae

JeremyKuhne added the area-System.Drawing label Jun 3, 2018

JeremyKuhne added this to the 3.0 milestone Jun 3, 2018

JeremyKuhne requested review from danmoseley, ericstj and safern June 3, 2018 00:03

stephentoub reviewed Jun 3, 2018

View reviewed changes

danmoseley reviewed Jun 3, 2018

View reviewed changes

JeremyKuhne self-assigned this Jun 3, 2018

stephentoub reviewed Jun 4, 2018

View reviewed changes

JeremyKuhne force-pushed the blitpoints branch from 337e656 to 066662c Compare June 4, 2018 06:27

safern reviewed Jun 4, 2018

View reviewed changes

Address feedback

43b2bd2

JeremyKuhne requested a review from Tanya-Solyanik June 5, 2018 03:27

safern reviewed Jun 6, 2018

View reviewed changes

Add space after fixed statements.

a0b4689

Tanya-Solyanik reviewed Jun 6, 2018

View reviewed changes

safern approved these changes Jun 6, 2018

View reviewed changes

Tanya-Solyanik approved these changes Jun 6, 2018

View reviewed changes

JeremyKuhne merged commit ba496ae into dotnet:master Jun 6, 2018

JeremyKuhne deleted the blitpoints branch June 6, 2018 17:26

filipnavara reviewed Feb 21, 2019

View reviewed changes

stephentoub mentioned this pull request Mar 14, 2023

DrawLines_PointFs, DrawBezier_PointFs, DrawBezier_Points and DrawLines_Points failing sporadically in CI dotnet/winforms#8814

Open

	internal static void CheckStatus(int status)
	{
	if (status != Ok)
	{
	throw StatusException(status);
	}
	}

		{
		int status = SafeNativeMethods.Gdip.GdipPathIterEnumerate(new HandleRef(this, nativeIter), out resultCount,

Fix blittable array instead of copying #30099

Fix blittable array instead of copying #30099

Conversation

JeremyKuhne commented Jun 3, 2018

JeremyKuhne commented Jun 3, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jun 3, 2018 • 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

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

JeremyKuhne commented Jun 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyKuhne commented Jun 3, 2018

JeremyKuhne commented Jun 3, 2018

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

JeremyKuhne commented Jun 4, 2018

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipnavara Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

stephentoub Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

jkotas Jun 3, 2018 •

edited

Loading

filipnavara Feb 21, 2019 •

edited

Loading

stephentoub Feb 21, 2019 •

edited

Loading