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

Make AsSpan(this string) ForceInline to be on par with AsSpan(this T[]) #17368

Merged
merged 2 commits into from
Mar 31, 2018
Merged

Make AsSpan(this string) ForceInline to be on par with AsSpan(this T[]) #17368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f13d62
Make AsSpan(this string) ForceInline to be on par with AsSpan(this T[])
jkotas Mar 31, 2018
a970996
CR feedback
jkotas Mar 31, 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
4 changes: 4 additions & 0 deletions src/mscorlib/shared/System/MemoryExtensions.Fast.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,7 @@ public static bool StartsWith(this ReadOnlySpan<char> span, ReadOnlySpan<char> v
/// <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.

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

@ahsonkhan ahsonkhan Mar 31, 2018 •
edited
Loading

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 •
edited
Loading

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.

Expand All @@ -380,6 +381,7 @@ public static Span<T> AsSpan<T>(this T[] array, int start)
/// </summary>
/// <param name="text">The target string.</param>
/// <remarks>Returns default when <paramref name="text"/> is null.</remarks>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static ReadOnlySpan<char> AsSpan(this string text)
{
if (text == null)
Expand All @@ -397,6 +399,7 @@ public static ReadOnlySpan<char> AsSpan(this string text)
/// <exception cref="System.ArgumentOutOfRangeException">
/// Thrown when the specified <paramref name="start"/> index is not in range (&lt;0 or &gt;text.Length).
/// </exception>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static ReadOnlySpan<char> AsSpan(this string text, int start)
{
if (text == null)
Expand All @@ -422,6 +425,7 @@ public static ReadOnlySpan<char> AsSpan(this string text, int start)
/// <exception cref="System.ArgumentOutOfRangeException">
/// Thrown when the specified <paramref name="start"/> index or <paramref name="length"/> is not in range.
/// </exception>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static ReadOnlySpan<char> AsSpan(this string text, int start, int length)
{
if (text == null)
Expand Down
10 changes: 4 additions & 6 deletions src/mscorlib/shared/System/MemoryExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ public static ReadOnlySpan<char> TrimEnd(this ReadOnlySpan<char> span)
/// </summary>
/// <param name="span">The source span from which the character is removed.</param>
/// <param name="trimChar">The specified character to look for and remove.</param>
//[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static ReadOnlySpan<char> Trim(this ReadOnlySpan<char> span, char trimChar)
{
return span.TrimStart(trimChar).TrimEnd(trimChar);
Expand All @@ -79,7 +78,6 @@ public static ReadOnlySpan<char> Trim(this ReadOnlySpan<char> span, char trimCha
/// </summary>
/// <param name="span">The source span from which the character is removed.</param>
/// <param name="trimChar">The specified character to look for and remove.</param>
//[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static ReadOnlySpan<char> TrimStart(this ReadOnlySpan<char> span, char trimChar)
{
int start = 0;
Expand Down Expand Up @@ -1204,7 +1202,7 @@ ref MemoryMarshal.GetReference(span),
/// no larger element, the bitwise complement of <see cref="Span{T}.Length"/>.
/// </returns>
/// <exception cref="T:System.ArgumentNullException">
/// <paramref name = "comparable" /> is <see langword="null"/> .
/// <paramref name = "comparable" /> is <see langword="null"/> .
/// </exception>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int BinarySearch<T>(
Expand All @@ -1228,7 +1226,7 @@ public static int BinarySearch<T>(
/// no larger element, the bitwise complement of <see cref="Span{T}.Length"/>.
/// </returns>
/// <exception cref="T:System.ArgumentNullException">
/// <paramref name = "comparable" /> is <see langword="null"/> .
/// <paramref name = "comparable" /> is <see langword="null"/> .
/// </exception>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int BinarySearch<T, TComparable>(
Expand Down Expand Up @@ -1278,7 +1276,7 @@ public static int BinarySearch<T, TComparer>(
/// no larger element, the bitwise complement of <see cref="ReadOnlySpan{T}.Length"/>.
/// </returns>
/// <exception cref="T:System.ArgumentNullException">
/// <paramref name = "comparable" /> is <see langword="null"/> .
/// <paramref name = "comparable" /> is <see langword="null"/> .
/// </exception>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int BinarySearch<T>(
Expand All @@ -1302,7 +1300,7 @@ public static int BinarySearch<T>(
/// no larger element, the bitwise complement of <see cref="ReadOnlySpan{T}.Length"/>.
/// </returns>
/// <exception cref="T:System.ArgumentNullException">
/// <paramref name = "comparable" /> is <see langword="null"/> .
/// <paramref name = "comparable" /> is <see langword="null"/> .
/// </exception>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int BinarySearch<T, TComparable>(
Expand Down
1 change: 1 addition & 0 deletions src/mscorlib/shared/System/String.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@ public static string Create<TState>(int length, TState state, SpanAction<char, T
return result;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static implicit operator ReadOnlySpan<char>(string value) =>
value != null ? new ReadOnlySpan<char>(ref value.GetRawStringData(), value.Length) : default;

Expand Down