Revert "[NativeAOT] Add null checks into memcpy/memset helpers"

[jkotas]

Reverts #98547

[ghost]

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Reverts #98547

Author:	jkotas
Assignees:	jkotas
Labels:	area-NativeAOT-coreclr
Milestone:	-

[jkotas]

The fix is not correct as discovered in #98623 (comment) .

Also, there may be unforeseen perf issues due to #98623 (comment) . I am thinking that we should revert it and wait for the proper fix from @EgorBo (#98623).

[jkotas]

cc @filipnavara

[filipnavara]

I’m fine with reverting and waiting for a more comprehensive fix. However, the Roslyn issue is not invalidating the fix completely. The managed code paths still trigger the NRE now where it would not previously trigger it. It’s only broken in the PInvoke path of the Memmove for large sizes. The performance cliff for SpanHelpers.Fill is also not present in the NativeAOT code since the usages of Unsafe.InitBlock are still in place. They are only removed in @EgorBo’s PR.

[EgorBo]

Up to you, hopefully, I'll land my PR tomorrow/this week 🙂

[jkotas]

The performance cliff for SpanHelpers.Fill is also not present in the NativeAOT code since the usages of Unsafe.InitBlock are still in place.

The problematic call chain is user code -> Span.Clear -> SpanHelpers.ClearWithoutReferences -> Unsafe.InitBlock -> MemoryHelpers.MemSet -> SpanHelpers.Fill. I have not realized that we use Unsafe.InitBlock in the implementation of Span.Clear when approving the PR. It would make me more worried about the perf impact.

The change regressed performance of common memory clearing operations like Span.Clear or Memory.Clear by up to 2x+. For example:

using System.Diagnostics;
for (;;)
{
   var a = new byte[512];
   var sw = new Stopwatch();
   sw.Start();
   Work(a);
   Console.WriteLine(sw.ElapsedMilliseconds);
}

static void Work(byte[] a)
{
   for (int i = 0; i < 100_000_000; i++) a.AsSpan().Clear();
}

Results with PublishAot on Winx64, Intel 9:
Before #98547: ~390ms per iteration
After #98547: ~880ms per iteration

Bulk memory clearing is part of many operations. I am worried that we will start getting reports of random perf regressions and we will have to spend time chasing them down to this change if we keep it in.

[filipnavara]

I see. I thought you were referring to performance regressions for small blocks of constant size (which, presumably, still get optimized by JIT and don’t end up in the helper). I didn’t realise that SpanHelpers.Fill is so significantly slower for big blocks.