Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix blittable array instead of copying #30099

Merged
JeremyKuhne merged 3 commits into from
Jun 6, 2018
Merged

Fix blittable array instead of copying #30099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
066662c
System.Drawing is doing a lot of unnecessary work.
JeremyKuhne Jun 4, 2018
43b2bd2
Address feedback
JeremyKuhne Jun 4, 2018
a0b4689
Add space after fixed statements.
JeremyKuhne Jun 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/System.Drawing.Common/System.Drawing.Common.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "System.Drawing.Common.Tests
{191B3618-FECD-4ABD-9D6B-5AC90DC33621} = {191B3618-FECD-4ABD-9D6B-5AC90DC33621}
EndProjectSection
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "System.Drawing.Common.Performance.Tests", "tests\Performance\System.Drawing.Common.Performance.Tests.csproj", "{E66FFA55-0975-4F0D-8A18-24B2687FEDEA}"
ProjectSection(ProjectDependencies) = postProject
{191B3618-FECD-4ABD-9D6B-5AC90DC33621} = {191B3618-FECD-4ABD-9D6B-5AC90DC33621}
EndProjectSection
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "System.Drawing.Common", "src\System.Drawing.Common.csproj", "{191B3618-FECD-4ABD-9D6B-5AC90DC33621}"
ProjectSection(ProjectDependencies) = postProject
{D7AEA698-275D-441F-B7A7-8491D1F0EFF0} = {D7AEA698-275D-441F-B7A7-8491D1F0EFF0}
Expand All @@ -30,6 +35,10 @@ Global
{4B93E684-0630-45F4-8F63-6C7788C9892F}.Debug|Any CPU.Build.0 = netcoreapp-Debug|Any CPU
{4B93E684-0630-45F4-8F63-6C7788C9892F}.Release|Any CPU.ActiveCfg = netcoreapp-Release|Any CPU
{4B93E684-0630-45F4-8F63-6C7788C9892F}.Release|Any CPU.Build.0 = netcoreapp-Release|Any CPU
{E66FFA55-0975-4F0D-8A18-24B2687FEDEA}.Debug|Any CPU.ActiveCfg = netstandard-Debug|Any CPU
{E66FFA55-0975-4F0D-8A18-24B2687FEDEA}.Debug|Any CPU.Build.0 = netstandard-Debug|Any CPU
{E66FFA55-0975-4F0D-8A18-24B2687FEDEA}.Release|Any CPU.ActiveCfg = netstandard-Release|Any CPU
{E66FFA55-0975-4F0D-8A18-24B2687FEDEA}.Release|Any CPU.Build.0 = netstandard-Release|Any CPU
{191B3618-FECD-4ABD-9D6B-5AC90DC33621}.Debug|Any CPU.ActiveCfg = netcoreapp-Windows_NT-Debug|Any CPU
{191B3618-FECD-4ABD-9D6B-5AC90DC33621}.Debug|Any CPU.Build.0 = netcoreapp-Windows_NT-Debug|Any CPU
{191B3618-FECD-4ABD-9D6B-5AC90DC33621}.Release|Any CPU.ActiveCfg = netcoreapp-Windows_NT-Release|Any CPU
Expand All @@ -44,6 +53,7 @@ Global
EndGlobalSection
GlobalSection(NestedProjects) = preSolution
{4B93E684-0630-45F4-8F63-6C7788C9892F} = {1A2F9F4A-A032-433E-B914-ADD5992BB178}
{E66FFA55-0975-4F0D-8A18-24B2687FEDEA} = {1A2F9F4A-A032-433E-B914-ADD5992BB178}
{191B3618-FECD-4ABD-9D6B-5AC90DC33621} = {E107E9C1-E893-4E87-987E-04EF0DCEAEFD}
{D7AEA698-275D-441F-B7A7-8491D1F0EFF0} = {2E666815-2EDB-464B-9DF6-380BF4789AD4}
EndGlobalSection
Expand Down
3 changes: 2 additions & 1 deletion src/System.Drawing.Common/src/System.Drawing.Common.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@
<Compile Include="System\Drawing\Imaging\Metafile.Windows.cs" />
<Compile Include="System\Drawing\Imaging\MetafileHeader.Windows.cs" />
<Compile Include="System\Drawing\Imaging\MetaHeader.Windows.cs" />
<Compile Include="System\Drawing\Internal\GpPathData.cs" />
<Compile Include="System\Drawing\Internal\GPStream.cs" />
<Compile Include="System\Drawing\Internal\SystemColorTracker.cs" />
<Compile Include="System\Drawing\LocalAppContextSwitches.cs" />
Expand Down Expand Up @@ -379,4 +380,4 @@
<Reference Include="System.Drawing" />
</ItemGroup>
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
</Project>
</Project>
67 changes: 25 additions & 42 deletions src/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPath.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,46 +183,36 @@ public FillMode FillMode
}
}

private PathData _GetPathData()
private unsafe PathData _GetPathData()
{
int ptSize = Marshal.SizeOf(typeof(GPPOINTF));
int count = PointCount;

int numPts = PointCount;
PathData pathData = new PathData()
{
Types = new byte[count],
Points = new PointF[count]
};

PathData pathData = new PathData() { Types = new byte[numPts] };
if (count == 0)
return pathData;
Copy link
Copy Markdown
Member

@stephentoub stephentoub Jun 4, 2018

Choose a reason for hiding this comment

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

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>() };
}

Copy link
Copy Markdown
Member Author

@JeremyKuhne JeremyKuhne Jun 4, 2018

Choose a reason for hiding this comment

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

Not common.


IntPtr memoryPathData = Marshal.AllocHGlobal(3 * IntPtr.Size);
IntPtr memoryPoints = Marshal.AllocHGlobal(checked(ptSize * numPts));
try
fixed (byte* t = pathData.Types)
fixed (PointF* p = pathData.Points)
{
GCHandle typesHandle = GCHandle.Alloc(pathData.Types, GCHandleType.Pinned);
try
GpPathData data = new GpPathData
{
IntPtr typesPtr = typesHandle.AddrOfPinnedObject();
Count = count,
Points = p,
Types = t
};
Copy link
Copy Markdown
Member

@filipnavara filipnavara Feb 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@stephentoub stephentoub Feb 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.


Marshal.StructureToPtr(numPts, memoryPathData, false);
Marshal.StructureToPtr(memoryPoints, (IntPtr)((long)memoryPathData + IntPtr.Size), false);
Marshal.StructureToPtr(typesPtr, (IntPtr)((long)memoryPathData + 2 * IntPtr.Size), false);

int status = SafeNativeMethods.Gdip.GdipGetPathData(new HandleRef(this, nativePath), memoryPathData);

if (status != SafeNativeMethods.Gdip.Ok)
{
throw SafeNativeMethods.Gdip.StatusException(status);
}
int status = SafeNativeMethods.Gdip.GdipGetPathData(new HandleRef(this, nativePath), &data);

pathData.Points = SafeNativeMethods.Gdip.ConvertGPPOINTFArrayF(memoryPoints, numPts);
}
finally
if (status != SafeNativeMethods.Gdip.Ok)
{
typesHandle.Free();
throw SafeNativeMethods.Gdip.StatusException(status);
Copy link
Copy Markdown
Member

@stephentoub stephentoub Jun 4, 2018

Choose a reason for hiding this comment

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

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);
}
}

Copy link
Copy Markdown
Member Author

@JeremyKuhne JeremyKuhne Jun 4, 2018

Choose a reason for hiding this comment

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

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

}
}
finally
{
Marshal.FreeHGlobal(memoryPathData);
Marshal.FreeHGlobal(memoryPoints);
}

return pathData;
}
Expand Down Expand Up @@ -1094,29 +1084,22 @@ public byte[] PathTypes
return types;
}
}
public PointF[] PathPoints

public unsafe PointF[] PathPoints
{
get
{
int count = PointCount;
int size = Marshal.SizeOf(typeof(GPPOINTF));
IntPtr buf = Marshal.AllocHGlobal(checked(count * size));
try
PointF[] points = new PointF[PointCount];
fixed(PointF* p = points)
Copy link
Copy Markdown
Member

@safern safern Jun 6, 2018

Choose a reason for hiding this comment

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

Nit: space before (

Copy link
Copy Markdown
Member Author

@JeremyKuhne JeremyKuhne Jun 6, 2018

Choose a reason for hiding this comment

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

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

{
int status = SafeNativeMethods.Gdip.GdipGetPathPoints(new HandleRef(this, nativePath), new HandleRef(null, buf), count);
int status = SafeNativeMethods.Gdip.GdipGetPathPoints(new HandleRef(this, nativePath), p, points.Length);

if (status != SafeNativeMethods.Gdip.Ok)
{
throw SafeNativeMethods.Gdip.StatusException(status);
}

PointF[] points = SafeNativeMethods.Gdip.ConvertGPPOINTFArrayF(buf, count);
return points;
}
finally
{
Marshal.FreeHGlobal(buf);
}
return points;
}
}
}
Expand Down
67 changes: 22 additions & 45 deletions src/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPathIterator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@
// See the LICENSE file in the project root for more information.

using System.Diagnostics;
using System.Runtime.InteropServices;
using System.Drawing.Internal;
using System.Globalization;
using System.Runtime.InteropServices;

namespace System.Drawing.Drawing2D
{
Expand Down Expand Up @@ -169,73 +168,51 @@ public unsafe int Enumerate(ref PointF[] points, ref byte[] types)
if (points.Length != types.Length)
throw SafeNativeMethods.Gdip.StatusException(SafeNativeMethods.Gdip.InvalidParameter);

int resultCount = 0;
int size = Marshal.SizeOf(typeof(GPPOINTF));
int count = points.Length;
byte[] typesLocal = new byte[count];
if (points.Length == 0)
return 0;

IntPtr memoryPts = Marshal.AllocHGlobal(checked(count * size));
try
fixed (PointF* p = points)
fixed (byte* t = types)
{
int status = SafeNativeMethods.Gdip.GdipPathIterEnumerate(new HandleRef(this, nativeIter), out resultCount,
Copy link
Copy Markdown

@Tanya-Solyanik Tanya-Solyanik Jun 6, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@JeremyKuhne JeremyKuhne Jun 6, 2018

Choose a reason for hiding this comment

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

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

memoryPts, typesLocal, count);
int status = SafeNativeMethods.Gdip.GdipPathIterEnumerate(
new HandleRef(this, nativeIter),
out int resultCount,
p,
t,
points.Length);

if (status != SafeNativeMethods.Gdip.Ok)
{
throw SafeNativeMethods.Gdip.StatusException(status);
}

if (resultCount < count)
{
SafeNativeMethods.ZeroMemory((byte*)(checked((long)memoryPts + resultCount * size)), (ulong)((count - resultCount) * size));
}

points = SafeNativeMethods.Gdip.ConvertGPPOINTFArrayF(memoryPts, count);
typesLocal.CopyTo(types, 0);
}
finally
{
Marshal.FreeHGlobal(memoryPts);
return resultCount;
}

return resultCount;
}

public unsafe int CopyData(ref PointF[] points, ref byte[] types, int startIndex, int endIndex)
{
if ((points.Length != types.Length) || (endIndex - startIndex + 1 > points.Length))
throw SafeNativeMethods.Gdip.StatusException(SafeNativeMethods.Gdip.InvalidParameter);

int resultCount = 0;
int size = Marshal.SizeOf(typeof(GPPOINTF));
int count = points.Length;
byte[] typesLocal = new byte[count];

IntPtr memoryPts = Marshal.AllocHGlobal(checked(count * size));
try
fixed (PointF* p = points)
fixed (byte* t = types)
{
int status = SafeNativeMethods.Gdip.GdipPathIterCopyData(new HandleRef(this, nativeIter), out resultCount,
memoryPts, typesLocal, startIndex, endIndex);
int status = SafeNativeMethods.Gdip.GdipPathIterCopyData(
new HandleRef(this, nativeIter),
out int resultCount,
p,
t,
startIndex,
endIndex);

if (status != SafeNativeMethods.Gdip.Ok)
{
throw SafeNativeMethods.Gdip.StatusException(status);
}

if (resultCount < count)
{
SafeNativeMethods.ZeroMemory((byte*)(checked((long)memoryPts + resultCount * size)), (ulong)((count - resultCount) * size));
}

points = SafeNativeMethods.Gdip.ConvertGPPOINTFArrayF(memoryPts, count);
typesLocal.CopyTo(types, 0);
}
finally
{
Marshal.FreeHGlobal(memoryPts);
return resultCount;
}

return resultCount;
}

// handle to native path iterator object
Expand Down
Loading