Replace memmove with Unsafe.CopyBlock in hydration code #98929
Conversation
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsThe calling convention for
|
| [MethodImplAttribute(MethodImplOptions.InternalCall)] | ||
| [RuntimeImport("*", "memmove")] | ||
| static extern unsafe void* memmove(byte* dmem, byte* smem, nuint size); | ||
| Unsafe.CopyBlock(pDest, pCurrent, (uint)payload); |
There was a problem hiding this comment.
@EgorBo would this use the managed impl of the block copying after your #98623?
Wonder whether we should just replace the whole block from line 266 onwards with that.
When I originally wrote this code in #77884 (comment) I had concerns about this running extremely early at startup. If the managed impl works here, it would probably generate equivalent code with the added benefit of being even better for --instruction-set native.
There was a problem hiding this comment.
@EgorBo would this use the managed impl of the block copying after your #98623?
Correct, it will be a direct call to https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.ByteMemOps.cs#L253 (accelerated with avx512 if presented)
There was a problem hiding this comment.
Oh, this is the thing called through RuntimeExports...
If we get a non-inlineable call then what this PR is doing is enough. We can't get rid of the small size special cases, the outer loop is very hot.
There was a problem hiding this comment.
If needed, we can consider exposing native memset/memmove/memset APIs in NativeMemory.cs API
There was a problem hiding this comment.
LGTM with a note that we're now calling into "arbitrary BCL code" at very early startup so we might need to revisit if people ever make the BCL code not low level enough (we e.g. can't touch any MethodTable at this point because it's the thing getting dehydrated).
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static void CopyBlock(void* destination, void* source, uint byteCount) | ||
| { | ||
| throw new PlatformNotSupportedException(); |
There was a problem hiding this comment.
This will need to be implemented if we ever want hydration work with minimal runtime
The calling convention for
memmoveneeds to be__cdeclon x86, so avoid hitting the issue.