Skip to content

Use zero-extension inMemory<T>.Pin #119895

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

Open
xtqqczze wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use zero-extension inMemory<T>.Pin #119895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 10 additions & 8 deletions src/libraries/System.Private.CoreLib/src/System/Memory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,10 +397,10 @@ public unsafe MemoryHandle Pin()
{
if (typeof(T) == typeof(char) && tmpObject is string s)
{
// Unsafe.AsPointer is safe since the handle pins it
GCHandle handle = GCHandle.Alloc(tmpObject, GCHandleType.Pinned);
ref char stringData = ref Unsafe.Add(ref s.GetRawStringData(), _index);
return new MemoryHandle(Unsafe.AsPointer(ref stringData), handle);
ref char reference = ref Unsafe.Add(ref s.GetRawStringData(), (nint)(uint)_index);
// Unsafe.AsPointer is safe since the handle pins it
return new MemoryHandle(Unsafe.AsPointer(ref reference), handle);
Comment on lines 400 to +403
Copy link
Member

@tannergooding tannergooding Sep 19, 2025

Choose a reason for hiding this comment

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

nit: I don't think the comment being moved down is "clearer", it just creates a larger diff.

The same with changing the name from stringData to reference.

For the actual Unsafe.Add, it can just be (uint)_index, you don't also need the cast to (nint). However, it's also worth noting that a lot of modern hardware has zero-cost sign-extension, so I'm not sure the extra churn/complexity is worthwhile.

This is unlikely to be a hot path as repeated pinning is itself expensive and the potential 1-cycle savings will get lost in the general noise due to the overhead of GCHandle.Alloc and freeing the handle.

Copy link
Contributor Author

@xtqqczze xtqqczze Sep 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We see diffs like this on X64:

-       lea      rax, bword ptr [r14+0x10]
-       mov      ecx, dword ptr [r15+0x08]
-       and      ecx, 0xD1FFAB1E
-       movsxd   rcx, ecx
-       lea      rax, [rax+8*rcx]
+       mov      eax, dword ptr [r15+0x08]
+       and      eax, 0xD1FFAB1E
+       lea      rax, bword ptr [r14+8*rax+0x10]
        xor      rcx, rcx
        mov      gword ptr [rbx], rcx
        mov      qword ptr [rbx+0x08], rax
-						;; size=30 bbWeight=0.50 PerfScore 2.88
+						;; size=23 bbWeight=0.50 PerfScore 2.75

Copy link
Contributor Author

@xtqqczze xtqqczze Sep 19, 2025

Choose a reason for hiding this comment

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

There's diffs on ARM64 as well. It seems like the primary improvement of the change is actually to addressing modes.

Copy link
Member

@tannergooding tannergooding Sep 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The latter isn't "strictly" better. It may actually be more expensive due to using a more expensive lea mode which is restricted to a single port, rather than the simpler lea mode which works on multiple ports.

The former could also become (almost) the latter if it was better and looks like just a missing JIT opt. Namely, it could be (only differing by the movsxd):

       mov      eax, dword ptr [r15+0x08]
       and      eax, 0xD1FFAB1E
       movsxd   rax, eax
       lea      rax, [r14+8*rax+0x10]

That is, there shouldn't be anything here that blocks the r14+0x10 from being contained since it's just (x+y)+(8*z) and changing to x+(8*z)+y

Copy link
Member

@tannergooding tannergooding Sep 19, 2025

Choose a reason for hiding this comment

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

It seems far more meaningful to investigate what's "blocking" that transform right now and whether it is actually more beneficial to do in typical scenarios.

It would likely have much broader impact to ecosystem and not require manually fixing a bunch of C# code to explicitly cast to uint

Copy link
Contributor Author

@xtqqczze xtqqczze Sep 19, 2025

Choose a reason for hiding this comment

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

It seems far more meaningful to investigate what's "blocking" that transform right now and whether it is actually more beneficial to do in typical scenarios.

@jakobbotsch Is this an example of the following issue?

Copy link
Contributor Author

@xtqqczze xtqqczze Sep 23, 2025

Choose a reason for hiding this comment

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

It seems far more meaningful to investigate what's "blocking" that transform right now and whether it is actually more beneficial to do in typical scenarios.

Well currently it is not possible because the logic reads _index multiple times, so it cannot be proven to be never negative. We could read var @this = this at the beginning of the method, this would certainly help.

Copy link
Member

@tannergooding tannergooding Sep 23, 2025

Choose a reason for hiding this comment

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

The memory model allows the reads to be hoisted in various scenarios, so there’s really nothing that should be blocking if we aren’t explicitly writing from the same thread

}
else if (RuntimeHelpers.ObjectHasComponentSize(tmpObject))
{
Expand All @@ -410,16 +410,18 @@ public unsafe MemoryHandle Pin()
// Array is already pre-pinned
if (_index < 0)
{
ref T arrayData = ref MemoryMarshal.GetArrayDataReference(Unsafe.As<T[]>(tmpObject));
ref T reference = ref Unsafe.Add(ref arrayData, (nint)(uint)(_index & ReadOnlyMemory<T>.RemoveFlagsBitMask));
// Unsafe.AsPointer is safe since it's pinned
void* pointer = Unsafe.Add<T>(Unsafe.AsPointer(ref MemoryMarshal.GetArrayDataReference(Unsafe.As<T[]>(tmpObject))), _index & ReadOnlyMemory<T>.RemoveFlagsBitMask);
return new MemoryHandle(pointer);
return new MemoryHandle(Unsafe.AsPointer(ref reference));
}
else
{
// Unsafe.AsPointer is safe since the handle pins it
GCHandle handle = GCHandle.Alloc(tmpObject, GCHandleType.Pinned);
void* pointer = Unsafe.Add<T>(Unsafe.AsPointer(ref MemoryMarshal.GetArrayDataReference(Unsafe.As<T[]>(tmpObject))), _index);
return new MemoryHandle(pointer, handle);
ref T arrayData = ref MemoryMarshal.GetArrayDataReference(Unsafe.As<T[]>(tmpObject));
ref T reference = ref Unsafe.Add(ref arrayData, (nint)(uint)_index);
// Unsafe.AsPointer is safe since the handle pins it
return new MemoryHandle(Unsafe.AsPointer(ref reference), handle);
}
}
else
Expand Down
18 changes: 10 additions & 8 deletions src/libraries/System.Private.CoreLib/src/System/ReadOnlyMemory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,10 +312,10 @@ public unsafe MemoryHandle Pin()
{
if (typeof(T) == typeof(char) && tmpObject is string s)
{
// Unsafe.AsPointer is safe since the handle pins it
GCHandle handle = GCHandle.Alloc(tmpObject, GCHandleType.Pinned);
ref char stringData = ref Unsafe.Add(ref s.GetRawStringData(), _index);
return new MemoryHandle(Unsafe.AsPointer(ref stringData), handle);
ref char reference = ref Unsafe.Add(ref s.GetRawStringData(), (nint)(uint)_index);
// Unsafe.AsPointer is safe since the handle pins it
return new MemoryHandle(Unsafe.AsPointer(ref reference), handle);
}
else if (RuntimeHelpers.ObjectHasComponentSize(tmpObject))
{
Expand All @@ -325,16 +325,18 @@ public unsafe MemoryHandle Pin()
// Array is already pre-pinned
if (_index < 0)
{
ref T arrayData = ref MemoryMarshal.GetArrayDataReference(Unsafe.As<T[]>(tmpObject));
ref T reference = ref Unsafe.Add(ref arrayData, (nint)(uint)(_index & RemoveFlagsBitMask));
// Unsafe.AsPointer is safe since it's pinned
void* pointer = Unsafe.Add<T>(Unsafe.AsPointer(ref MemoryMarshal.GetArrayDataReference(Unsafe.As<T[]>(tmpObject))), _index & RemoveFlagsBitMask);
return new MemoryHandle(pointer);
return new MemoryHandle(Unsafe.AsPointer(ref reference));
}
else
{
// Unsafe.AsPointer is safe since the handle pins it
GCHandle handle = GCHandle.Alloc(tmpObject, GCHandleType.Pinned);
void* pointer = Unsafe.Add<T>(Unsafe.AsPointer(ref MemoryMarshal.GetArrayDataReference(Unsafe.As<T[]>(tmpObject))), _index);
return new MemoryHandle(pointer, handle);
ref T arrayData = ref MemoryMarshal.GetArrayDataReference(Unsafe.As<T[]>(tmpObject));
ref T reference = ref Unsafe.Add(ref arrayData, (nint)(uint)_index);
// Unsafe.AsPointer is safe since the handle pins it
return new MemoryHandle(Unsafe.AsPointer(ref reference), handle);
}
}
else
Expand Down
Loading