Skip to content

Commit

Permalink
Remove some volatile use on objects in corelib (#100969)
Browse files Browse the repository at this point in the history
From a review of the use, all of this use for lazy initialization now appears to be superfluous, given our documented memory model.
  • Loading branch information
stephentoub committed Apr 20, 2024
1 parent 907eff8 commit bc0380f
Show file tree
Hide file tree
Showing 16 changed files with 55 additions and 86 deletions.
Expand Up @@ -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
Expand Down Expand Up @@ -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" },
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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!;
}
}
Expand All @@ -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!;
}
}
Expand Down
Expand Up @@ -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;
Expand Down
Expand Up @@ -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
Expand Down
Expand Up @@ -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;

Expand Down
Expand Up @@ -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.
Expand All @@ -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();
Expand Down
Expand Up @@ -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];

Expand Down
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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
{
Expand Down
Expand Up @@ -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;

Expand Down
4 changes: 1 addition & 3 deletions src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs
Expand Up @@ -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;

Expand Down
Expand Up @@ -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;
Expand Down
Expand Up @@ -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))]
Expand Down
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down
Expand Up @@ -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;
Expand Down
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -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)
Expand Down
Expand Up @@ -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;
}
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down
18 changes: 5 additions & 13 deletions src/libraries/System.Private.CoreLib/src/System/Type.cs
Expand Up @@ -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>();
Expand Down

0 comments on commit bc0380f

Please sign in to comment.