Allow ArrayPool.Shared devirtualization

[benaadams]

Follow up to #15743

Pre

• No devirtualization

Commit 1 (crossgen coreclr)

• 10x Devirtualized virtual call to ArrayPool'1:Rent; now direct call to TlsOverPerCoreLockedStacksArrayPool'1:Rent [final class]
• 32x Devirtualized virtual call to ArrayPool'1:Return; now direct call to TlsOverPerCoreLockedStacksArrayPool'1:Return [final class]

Commit 2 (crossgen coreclr)

• 21x Devirtualized virtual call to ArrayPool'1:Rent; now direct call to TlsOverPerCoreLockedStacksArrayPool'1:Rent [final class]
• 32x Devirtualized virtual call to ArrayPool'1:Return; now direct call to TlsOverPerCoreLockedStacksArrayPool'1:Return [final class]

@stephentoub @jkotas @AndyAyersMS PTAL

[benaadams]

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

[jkotas]

@AndyAyersMS These changes are working around JIT optimization limitations. Do you think that these optimization limitations are not going to be fixed anytime soon, and we should merge these?

[benaadams]

Ubuntu x64 Checked CoreFX Tests https://github.com/dotnet/coreclr/issues/20507

System.IO.Tests.File_Delete.Unix_NonExistentPath_Nop [FAIL]
      System.IO.DirectoryNotFoundException : Could not find a part of the path
 '/tmp/File_Delete_0oc4eg5j.54a/Unix_NonExistentPath_Nop_118_ac051064/C'.
      Stack Trace:
            at System.IO.FileSystem.DeleteFile(String fullPath)
            at System.IO.File.Delete(String path)
         System.IO.FileSystem/tests/File/Delete.cs(15,0):
          at System.IO.Tests.File_Delete.Delete(String path)
         System.IO.FileSystem/tests/File/Delete.cs(119,0):
          at System.IO.Tests.File_Delete.Unix_NonExistentPath_Nop()

[benaadams]

Windows_NT x64 Checked Build and Test (Jit - CoreFx TieredCompilation=0) fail already reported https://github.com/dotnet/coreclr/issues/20373

[benaadams]

@dotnet-bot test Ubuntu x64 Checked CoreFX Tests
@dotnet-bot test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test

[AndyAyersMS]

I'd rather fix this in the jit. I should have time to get this in for 3.0.

[stephentoub]

@jkotas, at one point you'd expressed concern about typing this field as anything derived from ArrayPool, as you wanted to allow developers to use reflection to set it as a workaround for the pool keeping lots of state around potentially indefinitely. Are you less concerned about that now that we cull arrays from the pool over time?

[jkotas]

Correct.

[AndyAyersMS]

I have the jit side changes (based on #15783) working, but we still need to make framework changes so that the backing field for Shared has the derived type. Otherwise the backing field initialization is done over in the .cctor where the jit can't see it.

If we're willing to trust that tiering will remove the expensive dynamic static init check for frequent renters, then this can simply be something like:

@@ -33,7 +33,7 @@ namespace System.Buffers
         /// optimized for very fast access speeds, at the expense of more memory consumption.
         /// The shared pool instance is created lazily on first access.
         /// </remarks>
-        public static ArrayPool<T> Shared { get; } = new TlsOverPerCoreLockedStacksArrayPool<T>();
+        public static ArrayPool<T> Shared => TlsOverPerCoreLockedStacksArrayPool<T>.SharedPool;

         /// <summary>
         /// Creates a new <see cref="ArrayPool{T}"/> instance using default configuration options.

@@ -52,6 +52,8 @@ namespace System.Buffers

         private readonly static bool s_trimBuffers = GetTrimBuffers();

+        public static TlsOverPerCoreLockedStacksArrayPool<T> SharedPool { get; } = new TlsOverPerCoreLockedStacksArrayPool<T>();

with no other source changes.

If we think it's important to always avoid the class init check we can instead cache specific cases in a non-generic static helper class like in Ben's changes. This caching can be done over in the derived class so no details of the caching spill over into the base.

Thoughts?

[jkotas]

If we're willing to trust that tiering will remove the expensive dynamic static init check for frequent renters,

Yes, I think it is fine to make this assumption. We should be assuming that the tiering JIT is enabled when doing optimizations like this.

[jkotas]

public static TlsOverPerCoreLockedStacksArrayPool<T> SharedPool { get; } = new TlsOverPerCoreLockedStacksArrayPool<T>();

This should be a private field and I think having it on ArrayPool directly would be better. Something like:

private readonly static TlsOverPerCoreLockedStacksArrayPool<T> s_shared = new  TlsOverPerCoreLockedStacksArrayPool<T>();

public static ArrayPool<T> Shared => s_shared;

Does that work?

[benaadams]

Added PR for that #20637

Not sure if the direct use of variables in calls to Rent in this PR is still needed?

[AndyAyersMS]

No other framework changes are needed once I fix a bit of plumbing in the jit.... otherwise devirt will be blocked by the issue noted #15783 (and that seems to be the case).

[AndyAyersMS]

I need to get #20553 merged first as the new jit changes build on that.

[benaadams]

I'll close this one then