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

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Mar 31, 2018

Fixes #17366

@jkotas jkotas requested review from ahsonkhan and stephentoub March 31, 2018 04:51
/// <summary>
/// Creates a new span over the portion of the target array.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to add it to this overload? I ask because it looks relatively bulky and you didn't annotate the corresponding string/int AsSpan overload.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is trying to fix another non-nonsensical perf shard that we have today:

     char[];
     new Span<char>(a, 1 ,1); // Fastest: ForceInlined
     a.AsSpan(1,1); // Slower: ForceInlined, but the extra layer leaves collateral damage that the JIT is not able to get rid of 
     a.AsSpan(1); // Slowest: Not inlined

I agree that it is not obvious whether forceinlining this one is a good trade-off. We should be consistent at least - I will mark the string/int AsSpan overloads as well for consistency.

@ahsonkhan
Copy link

Interestingly, the portable span AsSpan overloads are all marked with this attribute:
https://github.com/dotnet/corefx/blob/master/src/System.Memory/src/System/MemoryExtensions.Portable.cs#L299

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static Span<T> AsSpan<T>(this T[] array, int start)
{
if (array == null)
Copy link

@ahsonkhan ahsonkhan Mar 31, 2018

Choose a reason for hiding this comment

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

Orthogonal to this PR: We should consider creating an internal static Create method on Span (in Span.Fast.cs) just like the portable span and move this AsSpan method into MemoryExtensions.cs with the rest of the overloads (since the implementation between the two would be the same).
https://github.com/dotnet/corefx/blob/master/src/System.Memory/src/System/Span.Portable.cs#L48
https://github.com/dotnet/corefx/blob/master/src/System.Memory/src/System/MemoryExtensions.Portable.cs#L20

Copy link
Member Author

@jkotas jkotas Mar 31, 2018

Choose a reason for hiding this comment

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

I do not think this is a good idea. These extra layers make the JIT work harder and tend to leave inefficiencies behind. We should not be de-optimizing fast Span just so we can share a few more lines with the slow Span.

@jkotas jkotas merged commit 3632a51 into dotnet:master Mar 31, 2018
@jkotas jkotas deleted the forceinline branch April 11, 2018 22:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants