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

Adding ValidateTypeIsBlittable SpanHelper method and rename constant #24626

Merged
merged 5 commits into from
Oct 18, 2017
Merged

Adding ValidateTypeIsBlittable SpanHelper method and rename constant #24626

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0496db7
Adding ValidateTypeIsBlittable SpanHelper method and rename constant.
ahsonkhan Oct 13, 2017
48a47b9
Reverting SpanHelper change and renaming const to netstandard
ahsonkhan Oct 17, 2017
cc902de
Removing extra spaces.
ahsonkhan Oct 17, 2017
f8aa20b
Remove unnecessary new lines
ahsonkhan Oct 17, 2017
c58dbf3
Addressing PR feedback.
ahsonkhan Oct 17, 2017
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
5 changes: 2 additions & 3 deletions src/System.Memory/src/System.Memory.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
<CLSCompliant>false</CLSCompliant>
<DocumentationFile>$(OutputPath)$(MSBuildProjectName).xml</DocumentationFile>
<IsPartialFacadeAssembly Condition="'$(TargetGroup)' == 'netcoreapp' OR '$(TargetGroup)' == 'uap'">true</IsPartialFacadeAssembly>
<DefineConstants Condition="'$(IsPartialFacadeAssembly)' == 'true'">$(DefineConstants);IsPartialFacade</DefineConstants>
<DefineConstants Condition="'$(IsPartialFacadeAssembly)' == 'true'">$(DefineConstants);FEATURE_FASTSPAN</DefineConstants>
Copy link
Member

@jkotas jkotas Oct 16, 2017

Choose a reason for hiding this comment

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

The name of the define may rather be netstandard, with the condition inverted. It would be more similar to what we do in other places.

<DefineConstants Condition="'$(TargetGroup)'=='netcoreapp'">$(DefineConstants);netcoreapp</DefineConstants>
<DefineConstants Condition="'$(TargetGroup)'=='netstandard1.1'">$(DefineConstants);netstandard11</DefineConstants>
</PropertyGroup>
Expand Down Expand Up @@ -55,9 +55,8 @@
<Reference Include="System.Runtime" />
<Reference Include="System.Runtime.Extensions" />
<Reference Include="System.Runtime.InteropServices" />
<Reference Include="System.Runtime.CompilerServices.Unsafe" />
<Reference Condition="'$(TargetGroup)' != 'netstandard1.1'" Include="System.Numerics.Vectors" />
<Reference Condition="'$(TargetGroup)' != 'netstandard1.1'" Include="System.Runtime.CompilerServices.Unsafe" />
<ProjectReference Condition="'$(TargetGroup)' == 'netstandard1.1'" Include="..\..\System.Runtime.CompilerServices.Unsafe\src\System.Runtime.CompilerServices.Unsafe.ilproj" />
</ItemGroup>
<ItemGroup Condition="'$(IsPartialFacadeAssembly)' == 'true'">
<Compile Include="System\SpanExtensions.Fast.cs" />
Expand Down
26 changes: 4 additions & 22 deletions src/System.Memory/src/System/Buffers/Binary/Reader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,17 +98,8 @@ public static ulong ReverseEndianness(ulong value)
public static T ReadMachineEndian<T>(ReadOnlySpan<byte> buffer)
where T : struct
{
#if IsPartialFacade
if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
{
throw new ArgumentException(SR.Format(SR.Argument_InvalidTypeWithPointersNotSupported, typeof(T)));
}
#else
if (SpanHelpers.IsReferenceOrContainsReferences<T>())
{
ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(T));
}
#endif
SpanHelpers.ValidateTypeIsBlittable<T>();
Copy link
Member

@jkotas jkotas Oct 13, 2017

Choose a reason for hiding this comment

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

RuntimeHelpers.IsReferenceOrContainsReferences<T> needs to be called directly, not via wrapper method. The JIT won't be able to inline the wrapper method in all cases. This is introducing performance regression.

Copy link
Member

@weshaggard weshaggard Oct 13, 2017

Choose a reason for hiding this comment

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

Wouldn't the JIT be able to inline the helper method as well? If not would it be able to handle it if it didn't wrap the exception throwing as well?

Copy link
Member

@jkotas jkotas Oct 13, 2017

Choose a reason for hiding this comment

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

All generic methods wrapping other generic methods are problematic. They do not always inline well.

Copy link
Author

@ahsonkhan ahsonkhan Oct 13, 2017

Choose a reason for hiding this comment

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

In that case, I will revert this change. Is there any way to avoid having the #ifdef in multiple places (whenever I need to call IsReferenceOrContainsReferences) without a performance regression?

Copy link
Member

@jkotas jkotas Oct 14, 2017

Choose a reason for hiding this comment

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

You can put the whole thing in separate files. (I do not think it is an improvement.)

Copy link
Member

@weshaggard weshaggard Oct 16, 2017

Choose a reason for hiding this comment

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

OK @ahsonkhan thanks for trying to apply my feedback feel free to undo this part of the change and just do the other 2 pieces.


if (Unsafe.SizeOf<T>() > buffer.Length)
{
throw new ArgumentOutOfRangeException();
Expand All @@ -124,17 +115,8 @@ public static T ReadMachineEndian<T>(ReadOnlySpan<byte> buffer)
public static bool TryReadMachineEndian<T>(ReadOnlySpan<byte> buffer, out T value)
where T : struct
{
#if IsPartialFacade
if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
{
throw new ArgumentException(SR.Format(SR.Argument_InvalidTypeWithPointersNotSupported, typeof(T)));
}
#else
if (SpanHelpers.IsReferenceOrContainsReferences<T>())
{
ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(T));
}
#endif
SpanHelpers.ValidateTypeIsBlittable<T>();

if (Unsafe.SizeOf<T>() > (uint)buffer.Length)
{
value = default;
Expand Down
26 changes: 4 additions & 22 deletions src/System.Memory/src/System/Buffers/Binary/Writer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,8 @@ public static partial class BinaryPrimitives
public static void WriteMachineEndian<T>(Span<byte> buffer, ref T value)
where T : struct
{
#if IsPartialFacade
if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
{
throw new ArgumentException(SR.Format(SR.Argument_InvalidTypeWithPointersNotSupported, typeof(T)));
}
#else
if (SpanHelpers.IsReferenceOrContainsReferences<T>())
{
ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(T));
}
#endif
SpanHelpers.ValidateTypeIsBlittable<T>();

if ((uint)Unsafe.SizeOf<T>() > (uint)buffer.Length)
{
throw new ArgumentOutOfRangeException();
Expand All @@ -42,17 +33,8 @@ public static void WriteMachineEndian<T>(Span<byte> buffer, ref T value)
public static bool TryWriteMachineEndian<T>(Span<byte> buffer, ref T value)
where T : struct
{
#if IsPartialFacade
if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
{
throw new ArgumentException(SR.Format(SR.Argument_InvalidTypeWithPointersNotSupported, typeof(T)));
}
#else
if (SpanHelpers.IsReferenceOrContainsReferences<T>())
{
ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(T));
}
#endif
SpanHelpers.ValidateTypeIsBlittable<T>();

if (Unsafe.SizeOf<T>() > (uint)buffer.Length)
{
return false;
Expand Down
4 changes: 2 additions & 2 deletions src/System.Memory/src/System/ReadOnlySpan.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ public ReadOnlySpan(T[] array, int start, int length)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public unsafe ReadOnlySpan(void* pointer, int length)
{
if (SpanHelpers.IsReferenceOrContainsReferences<T>())
ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(T));
SpanHelpers.ValidateTypeIsBlittable<T>();

if (length < 0)
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);

Expand Down
4 changes: 2 additions & 2 deletions src/System.Memory/src/System/Span.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ public Span(T[] array, int start, int length)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public unsafe Span(void* pointer, int length)
{
if (SpanHelpers.IsReferenceOrContainsReferences<T>())
ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(T));
SpanHelpers.ValidateTypeIsBlittable<T>();

if (length < 0)
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);

Expand Down
20 changes: 6 additions & 14 deletions src/System.Memory/src/System/SpanExtensions.Portable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@ public static partial class SpanExtensions
public static Span<byte> AsBytes<T>(this Span<T> source)
where T : struct
{
if (SpanHelpers.IsReferenceOrContainsReferences<T>())
ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(T));
SpanHelpers.ValidateTypeIsBlittable<T>();

int newLength = checked(source.Length * Unsafe.SizeOf<T>());
return new Span<byte>(Unsafe.As<Pinnable<byte>>(source.Pinnable), source.ByteOffset, newLength);
Expand All @@ -49,8 +48,7 @@ public static Span<byte> AsBytes<T>(this Span<T> source)
public static ReadOnlySpan<byte> AsBytes<T>(this ReadOnlySpan<T> source)
where T : struct
{
if (SpanHelpers.IsReferenceOrContainsReferences<T>())
ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(T));
SpanHelpers.ValidateTypeIsBlittable<T>();

int newLength = checked(source.Length * Unsafe.SizeOf<T>());
return new ReadOnlySpan<byte>(Unsafe.As<Pinnable<byte>>(source.Pinnable), source.ByteOffset, newLength);
Expand Down Expand Up @@ -89,11 +87,8 @@ public static Span<TTo> NonPortableCast<TFrom, TTo>(this Span<TFrom> source)
where TFrom : struct
where TTo : struct
{
if (SpanHelpers.IsReferenceOrContainsReferences<TFrom>())
ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(TFrom));

if (SpanHelpers.IsReferenceOrContainsReferences<TTo>())
ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(TTo));
SpanHelpers.ValidateTypeIsBlittable<TFrom>();
SpanHelpers.ValidateTypeIsBlittable<TTo>();

int newLength = checked((int)((long)source.Length * Unsafe.SizeOf<TFrom>() / Unsafe.SizeOf<TTo>()));
return new Span<TTo>(Unsafe.As<Pinnable<TTo>>(source.Pinnable), source.ByteOffset, newLength);
Expand All @@ -118,11 +113,8 @@ public static ReadOnlySpan<TTo> NonPortableCast<TFrom, TTo>(this ReadOnlySpan<TF
where TFrom : struct
where TTo : struct
{
if (SpanHelpers.IsReferenceOrContainsReferences<TFrom>())
ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(TFrom));

if (SpanHelpers.IsReferenceOrContainsReferences<TTo>())
ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(TTo));
SpanHelpers.ValidateTypeIsBlittable<TFrom>();
SpanHelpers.ValidateTypeIsBlittable<TTo>();

int newLength = checked((int)((long)source.Length * Unsafe.SizeOf<TFrom>() / Unsafe.SizeOf<TTo>()));
return new ReadOnlySpan<TTo>(Unsafe.As<Pinnable<TTo>>(source.Pinnable), source.ByteOffset, newLength);
Expand Down
15 changes: 15 additions & 0 deletions src/System.Memory/src/System/SpanHelpers.T.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,5 +182,20 @@ public static bool SequenceEqual<T>(ref T first, ref T second, int length)
NotEqual: // Workaround for https://github.com/dotnet/coreclr/issues/13549
return false;
}

public static void ValidateTypeIsBlittable<T>()
{
#if FEATURE_FASTSPAN
if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
{
throw new ArgumentException(SR.Format(SR.Argument_InvalidTypeWithPointersNotSupported, typeof(T)));
}
#else
if (IsReferenceOrContainsReferences<T>())
{
ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(T));
}
#endif
}
}
}