Skip to content

Commit

Permalink
Remove some volatile use on objects in corelib (dotnet#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 authored and Ruihan-Yin committed May 30, 2024
1 parent 83ad500 commit cdc117e
Show file tree
Hide file tree
Showing 16 changed files with 55 additions and 86 deletions.
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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 cdc117e

Please sign in to comment.