Delete custom FastRandom from ThreadPool (#47338)

* Delete custom FastRandom from ThreadPool - Consolidates 128/256-bit variants of Xoshiro** into the same class name - Uses that from ThreadPool instead of its custom xorshift-based algorithm Throughput stays the same (~2.5ns per next random value) and incurs just one additional object allocation per thread pool thread. * Consolidate TARGET_64/32BIT DefineConstants
dotnet · Jan 22, 2021 · f1ad1b9 · f1ad1b9
1 parent f89e6b1
commit f1ad1b9
Show file tree

Hide file tree

Showing 9 changed files with 33 additions and 59 deletions.
diff --git a/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj
@@ -57,19 +57,18 @@
   <PropertyGroup Condition="'$(Platform)' == 'x64'">
     <PlatformTarget>x64</PlatformTarget>
     <Prefer32Bit>false</Prefer32Bit>
-    <DefineConstants>TARGET_64BIT;TARGET_AMD64;$(DefineConstants)</DefineConstants>
+    <DefineConstants>$(DefineConstants);TARGET_AMD64</DefineConstants>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Platform)' == 'x86'">
     <PlatformTarget>x86</PlatformTarget>
-    <DefineConstants>TARGET_32BIT;$(DefineConstants)</DefineConstants>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Platform)' == 'arm'">
     <PlatformTarget>arm</PlatformTarget>
-    <DefineConstants>TARGET_32BIT;TARGET_ARM;$(DefineConstants)</DefineConstants>
+    <DefineConstants>$(DefineConstants);TARGET_ARM</DefineConstants>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Platform)' == 'arm64'">
     <PlatformTarget>AnyCPU</PlatformTarget>
-    <DefineConstants>TARGET_64BIT;TARGET_ARM64;$(DefineConstants)</DefineConstants>
+    <DefineConstants>$(DefineConstants);TARGET_ARM64</DefineConstants>
   </PropertyGroup>
 
   <!-- Configuration specific properties -->

diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
@@ -14,6 +14,11 @@
     <SupportsArmIntrinsics Condition="'$(Platform)' == 'arm64'">true</SupportsArmIntrinsics>
     <SupportsX86Intrinsics Condition="'$(Platform)' == 'x64' or ('$(Platform)' == 'x86' and '$(TargetsUnix)' != 'true')">true</SupportsX86Intrinsics>
     <ILLinkSharedDirectory>$(MSBuildThisFileDirectory)ILLink\</ILLinkSharedDirectory>
+    <Is64Bit Condition="'$(Platform)' == 'arm64' or '$(Platform)' == 'x64'">true</Is64Bit>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DefineConstants Condition="'$(Is64Bit)' != 'true'">$(DefineConstants);TARGET_32BIT</DefineConstants>
+    <DefineConstants Condition="'$(Is64Bit)' == 'true'">$(DefineConstants);TARGET_64BIT</DefineConstants>
   </PropertyGroup>
   <PropertyGroup>
     <DefineConstants Condition="'$(TargetsUnix)' == 'true'">$(DefineConstants);TARGET_UNIX</DefineConstants>
@@ -29,8 +34,8 @@
   <ItemGroup>
     <ILLinkSubstitutionsXmls Include="$(ILLinkSharedDirectory)ILLink.Substitutions.Shared.xml" />
     <ILLinkSubstitutionsXmls Include="$(ILLinkSharedDirectory)ILLink.Substitutions.LittleEndian.xml" Condition="'$(Platform)' == 'wasm' or '$(Platform)' == 'arm' or '$(Platform)' == 'arm64' or '$(Platform)' == 'x86' or '$(Platform)' == 'x64'" />
-    <ILLinkSubstitutionsXmls Include="$(ILLinkSharedDirectory)ILLink.Substitutions.32bit.xml" Condition="'$(Platform)' == 'wasm' or '$(Platform)' == 'arm' or '$(Platform)' == 'x86'" />
-    <ILLinkSubstitutionsXmls Include="$(ILLinkSharedDirectory)ILLink.Substitutions.64bit.xml" Condition="'$(Platform)' == 'arm64' or '$(Platform)' == 'x64'" />
+    <ILLinkSubstitutionsXmls Include="$(ILLinkSharedDirectory)ILLink.Substitutions.32bit.xml" Condition="'$(Is64Bit)' != 'true'" />
+    <ILLinkSubstitutionsXmls Include="$(ILLinkSharedDirectory)ILLink.Substitutions.64bit.xml" Condition="'$(Is64Bit)' == 'true'" />
     <ILLinkSubstitutionsXmls Include="$(ILLinkSharedDirectory)ILLink.Substitutions.NoArmIntrinsics.xml" Condition="'$(SupportsArmIntrinsics)' != 'true'" />
     <ILLinkSubstitutionsXmls Include="$(ILLinkSharedDirectory)ILLink.Substitutions.NoX86Intrinsics.xml" Condition="'$(SupportsX86Intrinsics)' != 'true'" />
     <ILLinkLinkAttributesXmls Include="$(ILLinkSharedDirectory)ILLink.LinkAttributes.Shared.xml" />
@@ -485,8 +490,8 @@
     <Compile Include="$(MSBuildThisFileDirectory)System\Random.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Random.ImplBase.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Random.LegacyImpl.cs" />
-    <Compile Include="$(MSBuildThisFileDirectory)System\Random.Xoshiro128StarStarImpl.cs" />
-    <Compile Include="$(MSBuildThisFileDirectory)System\Random.Xoshiro256StarStarImpl.cs" />
+    <Compile Condition="'$(Is64Bit)' != 'true'" Include="$(MSBuildThisFileDirectory)System\Random.Xoshiro128StarStarImpl.cs" />
+    <Compile Condition="'$(Is64Bit)' == 'true'" Include="$(MSBuildThisFileDirectory)System\Random.Xoshiro256StarStarImpl.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Range.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\RankException.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\ReadOnlyMemory.cs" />

diff --git a/src/libraries/System.Private.CoreLib/src/System/Random.LegacyImpl.cs b/src/libraries/System.Private.CoreLib/src/System/Random.LegacyImpl.cs
@@ -20,7 +20,7 @@ private sealed class LegacyImpl : ImplBase
         {
             /// <summary>Thread-static instance used to seed any legacy implementations created with the default ctor.</summary>
             [ThreadStatic]
-            private static ImplBase? t_seedGenerator;
+            private static XoshiroImpl? t_seedGenerator;
 
             /// <summary>Reference to the <see cref="Random"/> containing this implementation instance.</summary>
             /// <remarks>Used to ensure that any calls to other virtual members are performed using the Random-derived instance, if one exists.</remarks>
@@ -29,7 +29,7 @@ private sealed class LegacyImpl : ImplBase
             private int _inext;
             private int _inextp;
 
-            public LegacyImpl(Random parent) : this(parent, (t_seedGenerator ??= CreateDefaultImpl()).Next())
+            public LegacyImpl(Random parent) : this(parent, (t_seedGenerator ??= new()).Next())
             {
             }
 

diff --git a/src/libraries/System.Private.CoreLib/src/System/Random.Xoshiro128StarStarImpl.cs b/src/libraries/System.Private.CoreLib/src/System/Random.Xoshiro128StarStarImpl.cs
@@ -17,7 +17,7 @@ public partial class Random
         /// As such, we are free to implement however we see fit, without back compat concerns around
         /// the sequence of numbers generated or what methods call what other methods.
         /// </summary>
-        internal sealed class Xoshiro128StarStarImpl : ImplBase
+        internal sealed class XoshiroImpl : ImplBase
         {
             // NextUInt32 is based on the algorithm from http://prng.di.unimi.it/xoshiro128starstar.c:
             //
@@ -31,7 +31,7 @@ internal sealed class Xoshiro128StarStarImpl : ImplBase
 
             private uint _s0, _s1, _s2, _s3;
 
-            public unsafe Xoshiro128StarStarImpl()
+            public unsafe XoshiroImpl()
             {
                 uint* ptr = stackalloc uint[4];
                 do
@@ -64,7 +64,8 @@ internal uint NextUInt32()
             }
 
             /// <summary>Produces a value in the range [0, ulong.MaxValue].</summary>
-            private ulong NextUInt64() => (((ulong)NextUInt32()) << 32) | NextUInt32();
+            [MethodImpl(MethodImplOptions.AggressiveInlining)] // small-ish hot path used by a handful of "next" methods
+            internal ulong NextUInt64() => (((ulong)NextUInt32()) << 32) | NextUInt32();
 
             public override int Next()
             {

diff --git a/src/libraries/System.Private.CoreLib/src/System/Random.Xoshiro256StarStarImpl.cs b/src/libraries/System.Private.CoreLib/src/System/Random.Xoshiro256StarStarImpl.cs
@@ -17,7 +17,7 @@ public partial class Random
         /// As such, we are free to implement however we see fit, without back compat concerns around
         /// the sequence of numbers generated or what methods call what other methods.
         /// </summary>
-        internal sealed class Xoshiro256StarStarImpl : ImplBase
+        internal sealed class XoshiroImpl : ImplBase
         {
             // NextUInt64 is based on the algorithm from http://prng.di.unimi.it/xoshiro256starstar.c:
             //
@@ -31,7 +31,7 @@ internal sealed class Xoshiro256StarStarImpl : ImplBase
 
             private ulong _s0, _s1, _s2, _s3;
 
-            public unsafe Xoshiro256StarStarImpl()
+            public unsafe XoshiroImpl()
             {
                 ulong* ptr = stackalloc ulong[4];
                 do
@@ -45,6 +45,10 @@ public unsafe Xoshiro256StarStarImpl()
                 while ((_s0 | _s1 | _s2 | _s3) == 0); // at least one value must be non-zero
             }
 
+            /// <summary>Produces a value in the range [0, uint.MaxValue].</summary>
+            [MethodImpl(MethodImplOptions.AggressiveInlining)] // small-ish hot path used by very few call sites
+            internal uint NextUInt32() => (uint)(NextUInt64() >> 32);
+
             /// <summary>Produces a value in the range [0, ulong.MaxValue].</summary>
             [MethodImpl(MethodImplOptions.AggressiveInlining)] // small-ish hot path used by a handful of "next" methods
             internal ulong NextUInt64()

diff --git a/src/libraries/System.Private.CoreLib/src/System/Random.cs b/src/libraries/System.Private.CoreLib/src/System/Random.cs
@@ -24,12 +24,7 @@ public Random() =>
             // With no seed specified, if this is the base type, we can implement this however we like.
             // If it's a derived type, for compat we respect the previous implementation, so that overrides
             // are called as they were previously.
-            _impl = GetType() == typeof(Random) ? CreateDefaultImpl() : new LegacyImpl(this);
-
-        /// <summary>Creates the default, optimized implementation used for `new Random()`.</summary>
-        private static ImplBase CreateDefaultImpl() => IntPtr.Size == 8 ?
-            new Xoshiro256StarStarImpl() : // optimized for 64-bit
-            new Xoshiro128StarStarImpl();  // optimized for 32-bit
+            _impl = GetType() == typeof(Random) ? new XoshiroImpl() : new LegacyImpl(this);
 
         /// <summary>Initializes a new instance of the Random class, using the specified seed value.</summary>
         /// <param name="Seed">

diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.cs
@@ -549,7 +549,7 @@ internal static bool LocalFindAndPop(object callback)
                 int c = queues.Length;
                 Debug.Assert(c > 0, "There must at least be a queue for this thread.");
                 int maxIndex = c - 1;
-                int i = tl.random.Next(c);
+                uint i = tl.random.NextUInt32() % (uint)c;
                 while (c > 0)
                 {
                     i = (i < maxIndex) ? i + 1 : 0;
@@ -799,31 +799,6 @@ private static void DispatchWorkItemWithWorkerTracking(object workItem, Thread c
         }
     }
 
-    // Simple random number generator. We don't need great randomness, we just need a little and for it to be fast.
-    internal struct FastRandom // xorshift prng
-    {
-        private uint _w, _x, _y, _z;
-
-        public FastRandom(int seed)
-        {
-            _x = (uint)seed;
-            _w = 88675123;
-            _y = 362436069;
-            _z = 521288629;
-        }
-
-        public int Next(int maxValue)
-        {
-            Debug.Assert(maxValue > 0);
-
-            uint t = _x ^ (_x << 11);
-            _x = _y; _y = _z; _z = _w;
-            _w = _w ^ (_w >> 19) ^ (t ^ (t >> 8));
-
-            return (int)(_w % (uint)maxValue);
-        }
-    }
-
     // Holds a WorkStealingQueue, and removes it from the list when this object is no longer referenced.
     internal sealed class ThreadPoolWorkQueueThreadLocals
     {
@@ -834,7 +809,7 @@ internal sealed class ThreadPoolWorkQueueThreadLocals
         public readonly ThreadPoolWorkQueue.WorkStealingQueue workStealingQueue;
         public readonly Thread currentThread;
         public readonly object? threadLocalCompletionCountObject;
-        public FastRandom random = new FastRandom(Environment.CurrentManagedThreadId); // mutable struct, do not copy or make readonly
+        public readonly Random.XoshiroImpl random = new Random.XoshiroImpl();
 
         public ThreadPoolWorkQueueThreadLocals(ThreadPoolWorkQueue tpq)
         {

diff --git a/src/libraries/System.Runtime.Extensions/tests/System/Random.cs b/src/libraries/System.Runtime.Extensions/tests/System/Random.cs
@@ -398,6 +398,7 @@ public void Xoshiro_AlgorithmBehavesAsExpected()
             Type implType = typeof(Random)
                 .GetNestedTypes(BindingFlags.NonPublic)
                 .Single(t => t.Name.StartsWith("Xoshiro", StringComparison.Ordinal));
+            Assert.NotNull(implType);
 
             var randOuter = new Random();
             object randInner = randOuter.GetType().GetField("_impl", BindingFlags.Instance | BindingFlags.NonPublic).GetValue(randOuter);
@@ -415,8 +416,6 @@ public void Xoshiro_AlgorithmBehavesAsExpected()
 
             if (IntPtr.Size == 8)
             {
-                Assert.Contains("256", implType.Name);
-
                 // Example seeds from https://www.pcg-random.org/posts/a-quick-look-at-xoshiro256.html
                 s0.SetValue(randInner, 0x01d353e5f3993bb0ul);
                 s1.SetValue(randInner, 0x7b9c0df6cb193b20ul);
@@ -487,8 +486,6 @@ public void Xoshiro_AlgorithmBehavesAsExpected()
             }
             else
             {
-                Assert.Contains("128", implType.Name);
-
                 s0.SetValue(randInner, 0x01d353e5u);
                 s1.SetValue(randInner, 0x7b9c0df6u);
                 s2.SetValue(randInner, 0xfdfcaa91u);

diff --git a/src/mono/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/mono/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj
@@ -50,30 +50,28 @@
   <PropertyGroup Condition="'$(Platform)' == 'x64'">
     <PlatformTarget>x64</PlatformTarget>
     <Prefer32Bit>false</Prefer32Bit>
-    <DefineConstants>TARGET_64BIT;TARGET_AMD64;$(DefineConstants)</DefineConstants>
+    <DefineConstants>$(DefineConstants);TARGET_AMD64</DefineConstants>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Platform)' == 'x86'">
     <PlatformTarget>x86</PlatformTarget>
-    <DefineConstants>TARGET_32BIT;$(DefineConstants)</DefineConstants>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Platform)' == 'arm'">
     <PlatformTarget>arm</PlatformTarget>
-    <DefineConstants>TARGET_32BIT;TARGET_ARM;$(DefineConstants)</DefineConstants>
+    <DefineConstants>$(DefineConstants);TARGET_ARM</DefineConstants>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Platform)' == 'arm64'">
     <PlatformTarget>AnyCPU</PlatformTarget>
-    <DefineConstants>TARGET_64BIT;TARGET_ARM64;$(DefineConstants)</DefineConstants>
+    <DefineConstants>$(DefineConstants);TARGET_ARM64</DefineConstants>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Platform)' == 'wasm'">
     <PlatformTarget>AnyCPU</PlatformTarget>
-    <DefineConstants>TARGET_32BIT;$(DefineConstants)</DefineConstants>
   </PropertyGroup>
 
   <!-- Configuration specific properties -->
   <PropertyGroup Condition="'$(Configuration)' == 'Debug' or '$(Configuration)' == 'Checked'">
     <Optimize Condition="'$(Optimize)' == '' and '$(Configuration)' == 'Debug'">false</Optimize>
     <Optimize Condition="'$(Optimize)' == '' and '$(Configuration)' == 'Checked'">true</Optimize>
-    <DefineConstants>_LOGGING;DEBUG;$(DefineConstants)</DefineConstants>
+    <DefineConstants>$(DefineConstants);_LOGGING;DEBUG</DefineConstants>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)' == 'Release'">
     <Optimize Condition="'$(Optimize)' == ''">true</Optimize>
@@ -103,7 +101,7 @@
   <PropertyGroup>
     <NoWarn>$(NoWarn),618,67</NoWarn>
 
-    <DefineConstants>MONO_FEATURE_SRE;$(DefineConstants)</DefineConstants>
+    <DefineConstants>$(DefineConstants);MONO_FEATURE_SRE</DefineConstants>
 
     <FeatureManagedEtwChannels>true</FeatureManagedEtwChannels>
     <FeatureManagedEtw>true</FeatureManagedEtw>