Adding ValidateTypeIsBlittable SpanHelper method and rename constant #24626
Conversation
| ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(T)); | ||
| } | ||
| #endif | ||
| SpanHelpers.ValidateTypeIsBlittable<T>(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
All generic methods wrapping other generic methods are problematic. They do not always inline well.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
You can put the whole thing in separate files. (I do not think it is an improvement.)
There was a problem hiding this comment.
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.
| @@ -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> | |||
There was a problem hiding this comment.
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.
| @@ -98,7 +98,7 @@ public static ulong ReverseEndianness(ulong value) | |||
| public static T ReadMachineEndian<T>(ReadOnlySpan<byte> buffer) | |||
| where T : struct | |||
| { | |||
| #if IsPartialFacade | |||
| #if netstandard | |||
There was a problem hiding this comment.
The order of if/else blocks need to be switched too.
|
@dotnet-bot test Linux x64 Release Build |
…otnet#24626) * Adding ValidateTypeIsBlittable SpanHelper method and rename constant. * Reverting SpanHelper change and renaming const to netstandard * Removing extra spaces. * Remove unnecessary new lines * Addressing PR feedback.
…otnet/corefx#24626) * Adding ValidateTypeIsBlittable SpanHelper method and rename constant. * Reverting SpanHelper change and renaming const to netstandard * Removing extra spaces. * Remove unnecessary new lines * Addressing PR feedback. Commit migrated from dotnet/corefx@5c8af50
Addressing leftover feedback from #24400 (review)
cc @weshaggard, @KrzysztofCwalina, @jkotas