Remove some volatile use on objects in corelib (#100969)

stephentoub · web-flow · commit bc0380f03993 · 2024-04-20T10:05:45.000-04:00
From a review of the use, all of this use for lazy initialization now appears to be superfluous, given our documented memory model.
diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.cs
@@ -111,14 +111,13 @@ internal sealed partial class CultureData
         private string? _sAM1159; // (user can override) AM designator
         private string? _sPM2359; // (user can override) PM designator
         private string? _sTimeSeparator;
-        private volatile string[]? _saLongTimes; // (user can override) time format
-        private volatile string[]? _saShortTimes; // (user can override) short time format
-        private volatile string[]? _saDurationFormats; // time duration format
+        private string[]? _saLongTimes; // (user can override) time format
+        private string[]? _saShortTimes; // (user can override) short time format
 
         // Calendar specific data
         private int _iFirstDayOfWeek = undef; // (user can override) first day of week (gregorian really)
         private int _iFirstWeekOfYear = undef; // (user can override) first week of year (gregorian really)
-        private volatile CalendarId[]? _waCalendars; // all available calendar type(s).  The first one is the default calendar
+        private CalendarId[]? _waCalendars; // all available calendar type(s).  The first one is the default calendar
 
         // Store for specific data about each calendar
         private CalendarData?[]? _calendars; // Store for specific calendar data
@@ -150,7 +149,7 @@ internal sealed partial class CultureData
         /// </remarks>
         private static Dictionary<string, string> RegionNames =>
             s_regionNames ??=
-            new Dictionary<string, string>(257 /* prime */, StringComparer.OrdinalIgnoreCase)
+            new Dictionary<string, string>(255, StringComparer.OrdinalIgnoreCase)
             {
                 { "001", "en-001" },
                 { "029", "en-029" },
@@ -411,7 +410,7 @@ internal sealed partial class CultureData
 
         // Cache of regions we've already looked up
         private static volatile Dictionary<string, CultureData>? s_cachedRegions;
-        private static volatile Dictionary<string, string>? s_regionNames;
+        private static Dictionary<string, string>? s_regionNames;
 
         /// <summary>
         /// The culture name to use to interop with the underlying native globalization libraries like ICU or Windows NLS APIs.
@@ -621,7 +620,6 @@ private static CultureData CreateCultureWithInvariantData()
             invariant._sPM2359 = "PM";                   // PM designator
             invariant._saLongTimes = new string[] { "HH:mm:ss" };                             // time format
             invariant._saShortTimes = new string[] { "HH:mm", "hh:mm tt", "H:mm", "h:mm tt" }; // short time format
-            invariant._saDurationFormats = new string[] { "HH:mm:ss" };                             // time duration format
 
             // Calendar specific data
             invariant._iFirstDayOfWeek = 0;                      // first day of week
@@ -661,7 +659,7 @@ private static CultureData CreateCultureWithInvariantData()
         /// We need an invariant instance, which we build hard-coded
         /// </summary>
         internal static CultureData Invariant => s_Invariant ??= CreateCultureWithInvariantData();
-        private static volatile CultureData? s_Invariant;
+        private static CultureData? s_Invariant;
 
         // Cache of cultures we've already looked up
         private static volatile Dictionary<string, CultureData>? s_cachedCultures;
@@ -1381,18 +1379,10 @@ internal string[] LongTimes
             {
                 if (_saLongTimes == null && !GlobalizationMode.Invariant)
                 {
-                    Debug.Assert(!GlobalizationMode.Invariant);
-
                     string[]? longTimes = GetTimeFormatsCore(shortFormat: false);
-                    if (longTimes == null || longTimes.Length == 0)
-                    {
-                        _saLongTimes = Invariant._saLongTimes!;
-                    }
-                    else
-                    {
-                        _saLongTimes = longTimes;
-                    }
+                    _saLongTimes = longTimes != null && longTimes.Length != 0 ? longTimes : Invariant._saLongTimes!;
                 }
+
                 return _saLongTimes!;
             }
         }
@@ -1407,23 +1397,13 @@ internal string[] ShortTimes
             {
                 if (_saShortTimes == null && !GlobalizationMode.Invariant)
                 {
-                    Debug.Assert(!GlobalizationMode.Invariant);
-
                     // Try to get the short times from the OS/culture.dll
+                    // If we couldn't find short times, then compute them from long times
+                    // (eg: CORECLR on < Win7 OS & fallback for missing culture.dll)
                     string[]? shortTimes = GetTimeFormatsCore(shortFormat: true);
-
-                    if (shortTimes == null || shortTimes.Length == 0)
-                    {
-                        //
-                        // If we couldn't find short times, then compute them from long times
-                        // (eg: CORECLR on < Win7 OS & fallback for missing culture.dll)
-                        //
-                        shortTimes = DeriveShortTimesFromLong();
-                    }
-
-                    // Found short times, use them
-                    _saShortTimes = shortTimes;
+                    _saShortTimes = shortTimes != null && shortTimes.Length != 0 ? shortTimes : DeriveShortTimesFromLong();
                 }
+
                 return _saShortTimes!;
             }
         }
diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.cs
@@ -126,8 +126,8 @@ private static void AsyncLocalSetCurrentUICulture(AsyncLocalValueChangedArgs<Cul
             s_currentThreadUICulture = args.CurrentValue;
         }
 
-        private static volatile Dictionary<string, CultureInfo>? s_cachedCulturesByName;
-        private static volatile Dictionary<int, CultureInfo>? s_cachedCulturesByLcid;
+        private static Dictionary<string, CultureInfo>? s_cachedCulturesByName;
+        private static Dictionary<int, CultureInfo>? s_cachedCulturesByLcid;
 
         // The parent culture.
         private CultureInfo? _parent;
diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeFormatInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeFormatInfo.cs
@@ -1914,8 +1914,8 @@ internal bool YearMonthAdjustment(ref int year, ref int month, bool parsedMonthN
         internal const string JapaneseLangName = "ja";
         internal const string EnglishLangName = "en";
 
-        private static volatile DateTimeFormatInfo? s_jajpDTFI;
-        private static volatile DateTimeFormatInfo? s_zhtwDTFI;
+        private static DateTimeFormatInfo? s_jajpDTFI;
+        private static DateTimeFormatInfo? s_zhtwDTFI;
 
         /// <summary>
         /// Create a Japanese DTFI which uses JapaneseCalendar.  This is used to parse
diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/GregorianCalendar.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/GregorianCalendar.cs
@@ -26,7 +26,7 @@ public class GregorianCalendar : Calendar
 
         internal static ReadOnlySpan<int> DaysToMonth366 => [0, 31, 60, 91, 121, 152, 182, 213, 244, 274, 305, 335, 366];
 
-        private static volatile Calendar? s_defaultInstance;
+        private static Calendar? s_defaultInstance;
 
         public override DateTime MinSupportedDateTime => DateTime.MinValue;
 
diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/JapaneseCalendar.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/JapaneseCalendar.cs
@@ -42,7 +42,7 @@ public partial class JapaneseCalendar : Calendar
 
         // Using a field initializer rather than a static constructor so that the whole class can be lazy
         // init.
-        private static volatile EraInfo[]? s_japaneseEraInfo;
+        private static EraInfo[]? s_japaneseEraInfo;
 
         // m_EraInfo must be listed in reverse chronological order.  The most recent era
         // should be the first element.
@@ -67,20 +67,19 @@ public partial class JapaneseCalendar : Calendar
         internal static EraInfo[] GetEraInfo()
         {
             // See if we need to build it
-            return s_japaneseEraInfo ??
-                (s_japaneseEraInfo = GlobalizationMode.UseNls ? NlsGetJapaneseEras() : IcuGetJapaneseEras()) ??
+            return s_japaneseEraInfo ??=
+                (GlobalizationMode.UseNls ? NlsGetJapaneseEras() : IcuGetJapaneseEras()) ??
                 // See if we have to use the built-in eras
-                (s_japaneseEraInfo = new EraInfo[]
-                {
+                [
                     new EraInfo(5, 2019, 5, 1, 2018, 1, GregorianCalendar.MaxYear - 2018, "\x4ee4\x548c", "\x4ee4", "R"),
                     new EraInfo(4, 1989, 1, 8, 1988, 1, 2019 - 1988, "\x5e73\x6210", "\x5e73", "H"),
                     new EraInfo(3, 1926, 12, 25, 1925, 1, 1989 - 1925, "\x662d\x548c", "\x662d", "S"),
                     new EraInfo(2, 1912, 7, 30, 1911, 1, 1926 - 1911, "\x5927\x6b63", "\x5927", "T"),
                     new EraInfo(1, 1868, 1, 1, 1867, 1, 1912 - 1867, "\x660e\x6cbb", "\x660e", "M")
-                });
+                ];
         }
 
-        internal static volatile Calendar? s_defaultInstance;
+        internal static Calendar? s_defaultInstance;
         internal GregorianCalendarHelper _helper;
 
         internal static Calendar GetDefaultInstance() => s_defaultInstance ??= new JapaneseCalendar();
diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/NumberFormatInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/NumberFormatInfo.cs
@@ -39,7 +39,7 @@ namespace System.Globalization
     /// </remarks>
     public sealed class NumberFormatInfo : IFormatProvider, ICloneable
     {
-        private static volatile NumberFormatInfo? s_invariantInfo;
+        private static NumberFormatInfo? s_invariantInfo;
         internal static readonly string[] s_asciiDigits = ["0", "1", "2", "3", "4", "5", "6", "7", "8", "9"];
         internal static readonly int[] s_intArrayWithElement3 = [3];
 
diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/RegionInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/RegionInfo.cs
@@ -21,7 +21,7 @@ public class RegionInfo
         private readonly CultureData _cultureData;
 
         // The RegionInfo for our current region
-        internal static volatile RegionInfo? s_currentRegionInfo;
+        internal static RegionInfo? s_currentRegionInfo;
 
         public RegionInfo(string name)
         {
@@ -84,7 +84,6 @@ internal RegionInfo(CultureData cultureData)
 
         /// <summary>
         /// This instance provides methods based on the current user settings.
-        /// These settings are volatile and may change over the lifetime of the
         /// </summary>
         public static RegionInfo CurrentRegion
         {
diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/TaiwanCalendar.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/TaiwanCalendar.cs
@@ -27,7 +27,7 @@ public class TaiwanCalendar : Calendar
             new EraInfo(1, 1912, 1, 1, 1911, 1, GregorianCalendar.MaxYear - 1911)    // era #, start year/month/day, yearOffset, minEraYear
         };
 
-        private static volatile Calendar? s_defaultInstance;
+        private static Calendar? s_defaultInstance;
 
         private readonly GregorianCalendarHelper _helper;
 
diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs
@@ -22,9 +22,7 @@ public abstract class Stream : MarshalByRefObject, IDisposable, IAsyncDisposable
         private protected SemaphoreSlim EnsureAsyncActiveSemaphoreInitialized() =>
             // Lazily-initialize _asyncActiveSemaphore.  As we're never accessing the SemaphoreSlim's
             // WaitHandle, we don't need to worry about Disposing it in the case of a race condition.
-#pragma warning disable CS8774 // We lack a NullIffNull annotation for Volatile.Read
-            Volatile.Read(ref _asyncActiveSemaphore) ??
-#pragma warning restore CS8774
+            _asyncActiveSemaphore ??
             Interlocked.CompareExchange(ref _asyncActiveSemaphore, new SemaphoreSlim(1, 1), null) ??
             _asyncActiveSemaphore;
 
diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/StringWriter.cs b/src/libraries/System.Private.CoreLib/src/System/IO/StringWriter.cs
@@ -12,7 +12,7 @@ namespace System.IO
     // the resulting sequence of characters to be presented as a string.
     public class StringWriter : TextWriter
     {
-        private static volatile UnicodeEncoding? s_encoding;
+        private static UnicodeEncoding? s_encoding;
 
         private readonly StringBuilder _sb;
         private bool _isOpen;
diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs
@@ -30,7 +30,7 @@ private enum InternalState
             Unloading
         }
 
-        private static volatile Dictionary<long, WeakReference<AssemblyLoadContext>>? s_allContexts;
+        private static Dictionary<long, WeakReference<AssemblyLoadContext>>? s_allContexts;
         private static long s_nextId;
 
         [MemberNotNull(nameof(s_allContexts))]
diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/EncodingProvider.cs b/src/libraries/System.Private.CoreLib/src/System/Text/EncodingProvider.cs
@@ -118,10 +118,10 @@ internal static void AddProvider(EncodingProvider provider)
 
         internal static Encoding? GetEncodingFromProvider(string encodingName)
         {
-            if (s_providers == null)
+            EncodingProvider[]? providers = s_providers;
+            if (providers == null)
                 return null;
 
-            EncodingProvider[] providers = s_providers;
             foreach (EncodingProvider provider in providers)
             {
                 Encoding? enc = provider.GetEncoding(encodingName);
@@ -134,10 +134,10 @@ internal static void AddProvider(EncodingProvider provider)
 
         internal static Encoding? GetEncodingFromProvider(int codepage, EncoderFallback enc, DecoderFallback dec)
         {
-            if (s_providers == null)
+            EncodingProvider[]? providers = s_providers;
+            if (providers == null)
                 return null;
 
-            EncodingProvider[] providers = s_providers;
             foreach (EncodingProvider provider in providers)
             {
                 Encoding? encoding = provider.GetEncoding(codepage, enc, dec);
@@ -150,10 +150,10 @@ internal static void AddProvider(EncodingProvider provider)
 
         internal static Encoding? GetEncodingFromProvider(string encodingName, EncoderFallback enc, DecoderFallback dec)
         {
-            if (s_providers == null)
+            EncodingProvider[]? providers = s_providers;
+            if (providers == null)
                 return null;
 
-            EncodingProvider[] providers = s_providers;
             foreach (EncodingProvider provider in providers)
             {
                 Encoding? encoding = provider.GetEncoding(encodingName, enc, dec);
diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ExecutionContext.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ExecutionContext.cs
@@ -20,7 +20,7 @@ namespace System.Threading
     public sealed class ExecutionContext : IDisposable, ISerializable
     {
         internal static readonly ExecutionContext Default = new ExecutionContext();
-        private static volatile ExecutionContext? s_defaultFlowSuppressed;
+        private static ExecutionContext? s_defaultFlowSuppressed;
 
         private readonly IAsyncLocalValueMap? m_localValues;
         private readonly IAsyncLocal[]? m_localChangeNotifications;
diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ManualResetEventSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ManualResetEventSlim.cs
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Diagnostics;
+using System.Diagnostics.CodeAnalysis;
 using System.Runtime.Versioning;
 
 namespace System.Threading
@@ -35,7 +36,7 @@ public class ManualResetEventSlim : IDisposable
         // These are the default spin counts we use on single-proc and MP machines.
         private const int DEFAULT_SPIN_SP = 1;
 
-        private volatile object? m_lock;
+        private object? m_lock;
         // A lock used for waiting and pulsing. Lazily initialized via EnsureLockObjectCreated()
 
         private volatile ManualResetEvent? m_eventObj; // A true Win32 event used for waiting.
@@ -199,13 +200,13 @@ private void Initialize(bool initialState, int spinCount)
         /// <summary>
         /// Helper to ensure the lock object is created before first use.
         /// </summary>
+        [MemberNotNull(nameof(m_lock))]
         private void EnsureLockObjectCreated()
         {
-            if (m_lock != null)
-                return;
-
-            object newObj = new object();
-            Interlocked.CompareExchange(ref m_lock, newObj, null); // failure is benign. Someone else set the value.
+            if (m_lock is null)
+            {
+                Interlocked.CompareExchange(ref m_lock, new object(), null); // failure is benign. Someone else set the value.
+            }
         }
 
         /// <summary>
@@ -538,7 +539,7 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken)
                 // We must register and unregister the token outside of the lock, to avoid deadlocks.
                 using (cancellationToken.UnsafeRegister(s_cancellationTokenCallback, this))
                 {
-                    lock (m_lock!)
+                    lock (m_lock)
                     {
                         // Loop to cope with spurious wakeups from other waits being canceled
                         while (!IsSet)
diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs
@@ -21,23 +21,23 @@ internal sealed partial class ThreadPoolWorkQueue
         internal static class WorkStealingQueueList
         {
 #pragma warning disable CA1825 // avoid the extra generic instantiation for Array.Empty<T>(); this is the only place we'll ever create this array
-            private static volatile WorkStealingQueue[] _queues = new WorkStealingQueue[0];
+            private static WorkStealingQueue[] s_queues = new WorkStealingQueue[0];
 #pragma warning restore CA1825
 
-            public static WorkStealingQueue[] Queues => _queues;
+            public static WorkStealingQueue[] Queues => s_queues;
 
             public static void Add(WorkStealingQueue queue)
             {
                 Debug.Assert(queue != null);
                 while (true)
                 {
-                    WorkStealingQueue[] oldQueues = _queues;
+                    WorkStealingQueue[] oldQueues = s_queues;
                     Debug.Assert(Array.IndexOf(oldQueues, queue) < 0);
 
                     var newQueues = new WorkStealingQueue[oldQueues.Length + 1];
                     Array.Copy(oldQueues, newQueues, oldQueues.Length);
                     newQueues[^1] = queue;
-                    if (Interlocked.CompareExchange(ref _queues, newQueues, oldQueues) == oldQueues)
+                    if (Interlocked.CompareExchange(ref s_queues, newQueues, oldQueues) == oldQueues)
                     {
                         break;
                     }
@@ -49,7 +49,7 @@ public static void Remove(WorkStealingQueue queue)
                 Debug.Assert(queue != null);
                 while (true)
                 {
-                    WorkStealingQueue[] oldQueues = _queues;
+                    WorkStealingQueue[] oldQueues = s_queues;
                     if (oldQueues.Length == 0)
                     {
                         return;
@@ -77,7 +77,7 @@ public static void Remove(WorkStealingQueue queue)
                         Array.Copy(oldQueues, pos + 1, newQueues, pos, newQueues.Length - pos);
                     }
 
-                    if (Interlocked.CompareExchange(ref _queues, newQueues, oldQueues) == oldQueues)
+                    if (Interlocked.CompareExchange(ref s_queues, newQueues, oldQueues) == oldQueues)
                     {
                         break;
                     }
diff --git a/src/libraries/System.Private.CoreLib/src/System/Type.cs b/src/libraries/System.Private.CoreLib/src/System/Type.cs
@@ -705,20 +705,12 @@ public override int GetHashCode()
         [Obsolete(Obsoletions.ReflectionOnlyLoadingMessage, DiagnosticId = Obsoletions.ReflectionOnlyLoadingDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
         public static Type? ReflectionOnlyGetType(string typeName, bool throwIfNotFound, bool ignoreCase) => throw new PlatformNotSupportedException(SR.PlatformNotSupported_ReflectionOnly);
 
-        public static Binder DefaultBinder
-        {
-            get
-            {
-                if (s_defaultBinder == null)
-                {
-                    DefaultBinder binder = new DefaultBinder();
-                    Interlocked.CompareExchange<Binder?>(ref s_defaultBinder, binder, null);
-                }
-                return s_defaultBinder!;
-            }
-        }
+        public static Binder DefaultBinder =>
+            s_defaultBinder ??
+            Interlocked.CompareExchange(ref s_defaultBinder, new DefaultBinder(), null) ??
+            s_defaultBinder;
 
-        private static volatile Binder? s_defaultBinder;
+        private static Binder? s_defaultBinder;
 
         public static readonly char Delimiter = '.';
         public static readonly Type[] EmptyTypes = Array.Empty<Type>();
