-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Optimize some SegmentedArrayHelper members for JIT #67558
Conversation
internal const MethodImplOptions FastPathMethodImplOptions = MethodImplOptions.AggressiveInlining | (MethodImplOptions)512; | ||
|
||
[MethodImpl(FastPathMethodImplOptions)] | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
internal static int GetSegmentSize<T>() | ||
{ | ||
if (Unsafe.SizeOf<T>() == Unsafe.SizeOf<object>()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Fixes dotnet/runtime#84094 |
@jkotas it seems that the generic path where it doesn't inline is fairly popular (I ran Roslyn tests). But the object sizes were mostly 24 or 40 so we can either hard-code segment sizes for these or rewrite size calculation logic to be foldable at jit-time always without static readonly (if we can rely on .NET framework) |
segmentSize = 1 << BitOperations.Log2((uint)((85000 / elementSize) - IntPtr.Size * 2)); seems to match existing logic. Although, not sure Log2 is constant folded on .NET Framework (or does it even exist there. UPD - it does not. Well, at least it is folded on .NET 8) PS: the existing formula seems to be a bit inaccurate, the layout of array is [PtrSize header][PtrSize pMt][8b bounds/sizes][data] |
@dotnet/roslyn-compiler I'll submit a follow-up pull request to manually constant-fold the results for the most common item sizes (4, 8, 24, and 40), and also fix the logic as mentioned in #67558 (comment). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 7)
These properties pop up in perf traces while it is expected that they should be folded to constants. There are two problems here:
AggressiveOptimization
((MethodImplOptions)512
) attribute often ruinsstatic readonly
related optimizations because JIT is forced to compile a method straight to Tier1 so some types might not be statically initialized yet.Example:
Codegen for
GetSize
function:Now, if I remove
AggressiveOptimization
and run it again:As a bonus, JIT won't waste time on jitting these functions during startup as R2R is expected to be picked up (it doesn't exist for methods with
AggressiveOptimization
).Then
ValueTypeSegmentHelper<T>.field
can't be inlined as is (needs dictionary runtime lookup and jit gives up on those).I movedSegmentShift
andOffsetMask
to a non generic type since those don't depend onT
, but I can't do the same forSegmentSize
.As I workaround we can record which sizes are the most popular and add fast paths probably. Or use a dictionary, cc @jkotas
Here is a minimal repro:
Codegen for
Foo
: