Skip to content

Optimize some SegmentedArrayHelper members for JIT #67558

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AlekseyTs merged 7 commits into from
Apr 21, 2023
Merged

Optimize some SegmentedArrayHelper members for JIT #67558

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
eecd99d
Optimize some SegmentedArrayHelper members for JIT
EgorBo Mar 29, 2023
874b732
Fix build
EgorBo Mar 29, 2023
455cecf
revert previous changes
EgorBo Mar 29, 2023
c544e7f
fix build
EgorBo Mar 29, 2023
5672c74
Faster segment size calculation logic for netcoreapp
EgorBo Mar 30, 2023
2f69c8f
Fix formatting
EgorBo Mar 30, 2023
bd2bce1
Address feedback
EgorBo Mar 30, 2023
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
50 changes: 40 additions & 10 deletions src/Dependencies/Collections/Internal/SegmentedArrayHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,12 @@ internal static class SegmentedArrayHelper
// Large value types may benefit from a smaller number.
internal const int IntrosortSizeThreshold = 16;

/// <summary>
/// A combination of <see cref="MethodImplOptions.AggressiveInlining"/> and
/// <see cref="F:System.Runtime.CompilerServices.MethodImplOptions.AggressiveOptimization"/>.
/// </summary>
[SuppressMessage("Documentation", "CA1200:Avoid using cref tags with a prefix", Justification = "The field is not supported in all compilation targets.")]
internal const MethodImplOptions FastPathMethodImplOptions = MethodImplOptions.AggressiveInlining | (MethodImplOptions)512;

[MethodImpl(FastPathMethodImplOptions)]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static int GetSegmentSize<T>()
{
#if NETCOREAPP3_0_OR_NEWER
return InlineCalculateSegmentSize(Unsafe.SizeOf<T>());
#else
if (Unsafe.SizeOf<T>() == Unsafe.SizeOf<object>())
Copy link
Copy Markdown
Member

@jkotas jkotas Mar 29, 2023

Choose a reason for hiding this comment

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

if (sizeof(T) == sizeof(object)) would be better, in particular on .NET Framework where Unsafe.SizeOf is not an intrinsic.

Copy link
Copy Markdown
Member Author

@EgorBo EgorBo Mar 29, 2023

Choose a reason for hiding this comment

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

It complains on error CS0208: Cannot take the address of, get the size of, or declare a pointer to a managed type ('T') that I can't suppress 🤔

Copy link
Copy Markdown
Member

@jkotas jkotas Mar 29, 2023

Choose a reason for hiding this comment

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

Is it building with recent Roslyn? The ability to suppress this was added not too long ago.

Copy link
Copy Markdown
Member Author

@EgorBo EgorBo Mar 29, 2023

Choose a reason for hiding this comment

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

I assume it builds with the bootstrap SDK which is 8.0.100-preview.1.23115.2

Copy link
Copy Markdown
Member

@jkotas jkotas Mar 29, 2023

Choose a reason for hiding this comment

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

It is possible to suppress this in .NET 7 SDK. For example, this compiles fine with .NET 7 SDK:

internal unsafe static int GetSegmentSize<T>()
{
#pragma warning disable CS8500 // Takes a pointer to a managed type
    return (sizeof(T) == sizeof(object)) ? 1 : -1;
#pragma warning restore CS8500
}

Copy link
Copy Markdown
Member Author

@EgorBo EgorBo Mar 29, 2023

Choose a reason for hiding this comment

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

I think I've already tried that in the Roslyn repo and but it still complained during build, I'll try again tomorrow

Copy link
Copy Markdown
Member Author

@EgorBo EgorBo Mar 30, 2023

Choose a reason for hiding this comment

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

Yes, still complains, perhaps someone from Roslyn can help? Although, I think I fixed the main perf issue in this PR already

{
return ReferenceTypeSegmentHelper.SegmentSize;
Expand All @@ -34,11 +30,15 @@ internal static int GetSegmentSize<T>()
{
return ValueTypeSegmentHelper<T>.SegmentSize;
}
#endif
}

[MethodImpl(FastPathMethodImplOptions)]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static int GetSegmentShift<T>()
{
#if NETCOREAPP3_0_OR_NEWER
return InlineCalculateSegmentShift(Unsafe.SizeOf<T>());
#else
if (Unsafe.SizeOf<T>() == Unsafe.SizeOf<object>())
{
return ReferenceTypeSegmentHelper.SegmentShift;
Comment thread
EgorBo marked this conversation as resolved.
Expand All @@ -47,11 +47,15 @@ internal static int GetSegmentShift<T>()
{
return ValueTypeSegmentHelper<T>.SegmentShift;
}
#endif
}

[MethodImpl(FastPathMethodImplOptions)]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static int GetOffsetMask<T>()
{
#if NETCOREAPP3_0_OR_NEWER
return InlineCalculateOffsetMask(Unsafe.SizeOf<T>());
#else
if (Unsafe.SizeOf<T>() == Unsafe.SizeOf<object>())
{
return ReferenceTypeSegmentHelper.OffsetMask;
Expand All @@ -60,6 +64,7 @@ internal static int GetOffsetMask<T>()
{
return ValueTypeSegmentHelper<T>.OffsetMask;
}
#endif
}

/// <summary>
Expand Down Expand Up @@ -121,6 +126,31 @@ private static int CalculateOffsetMask(int segmentSize)
return segmentSize - 1;
}

// Faster inline implementation for NETCOREAPP to avoid static constructors and non-inlineable
// generics with runtime lookups
#if NETCOREAPP3_0_OR_NEWER
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static int InlineCalculateSegmentSize(int elementSize)
{
return 1 << InlineCalculateSegmentShift(elementSize);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static int InlineCalculateSegmentShift(int elementSize)
{
// Default Large Object Heap size threshold
// https://github.com/dotnet/runtime/blob/c9d69e38d0e54bea5d188593ef6c3b30139f3ab1/src/coreclr/src/gc/gc.h#L111
const uint Threshold = 85000;
return System.Numerics.BitOperations.Log2((uint)((Threshold / elementSize) - (2 * Unsafe.SizeOf<object>())));
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static int InlineCalculateOffsetMask(int elementSize)
{
return InlineCalculateSegmentSize(elementSize) - 1;
}
#endif

internal static class TestAccessor
{
public static int CalculateSegmentSize(int elementSize)
Expand Down
5 changes: 1 addition & 4 deletions src/Dependencies/Collections/SegmentedArray`1.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ namespace Microsoft.CodeAnalysis.Collections
/// </remarks>
private static int SegmentSize
{
[MethodImpl(SegmentedArrayHelper.FastPathMethodImplOptions)]
get
{
return SegmentedArrayHelper.GetSegmentSize<T>();
Expand All @@ -44,7 +43,6 @@ private static int SegmentSize
/// </summary>
private static int SegmentShift
{
[MethodImpl(SegmentedArrayHelper.FastPathMethodImplOptions)]
get
{
return SegmentedArrayHelper.GetSegmentShift<T>();
Expand All @@ -56,7 +54,6 @@ private static int SegmentShift
/// </summary>
private static int OffsetMask
{
[MethodImpl(SegmentedArrayHelper.FastPathMethodImplOptions)]
get
{
return SegmentedArrayHelper.GetOffsetMask<T>();
Expand Down Expand Up @@ -115,7 +112,7 @@ private SegmentedArray(int length, T[][] items)

public ref T this[int index]
{
[MethodImpl(SegmentedArrayHelper.FastPathMethodImplOptions)]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get
{
return ref _items[index >> SegmentShift][index & OffsetMask];
Expand Down