From 731dee4a8d9ad231826106ad59d3a5841738e774 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Thu, 12 Nov 2020 09:33:57 -0500 Subject: [PATCH] Remove LazyInitializer usage from corelib (#44409) It's fine for higher layers, but in corelib we don't want to forcibly prevent LazyInitializer from being trimmed nor pay the overheads of additional cached delegates. --- .../System/Environment.GetFolderPathCore.Unix.cs | 23 ++++++++------- .../src/System/IO/BufferedStream.cs | 34 +++++++++------------- .../System.Private.CoreLib/src/System/IO/Stream.cs | 14 ++++++--- .../src/System/Resources/ResourceReader.Core.cs | 31 ++++++++++---------- .../Tasks/ConcurrentExclusiveSchedulerPair.cs | 14 +++++++-- .../src/System/Threading/Tasks/Task.cs | 24 ++++++++++----- .../System/Runtime/InteropServices/Marshal.Mono.cs | 16 +++++----- .../src/System/RuntimeType.Mono.cs | 16 ++++------ 8 files changed, 92 insertions(+), 80 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Environment.GetFolderPathCore.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Environment.GetFolderPathCore.Unix.cs index 6e5e7eb..4ce955a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Environment.GetFolderPathCore.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Environment.GetFolderPathCore.Unix.cs @@ -37,20 +37,21 @@ namespace System { return string.Empty; } - else - { - Debug.Assert(option == SpecialFolderOption.Create); - Func createDirectory = LazyInitializer.EnsureInitialized(ref s_directoryCreateDirectory, static () => - { - Type dirType = Type.GetType("System.IO.Directory, System.IO.FileSystem, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a", throwOnError: true)!; - MethodInfo mi = dirType.GetMethod("CreateDirectory", BindingFlags.Public | BindingFlags.Static)!; - return mi.CreateDelegate>(); - }); - createDirectory(path); + Debug.Assert(option == SpecialFolderOption.Create); - return path; + Func? createDirectory = Volatile.Read(ref s_directoryCreateDirectory); + if (createDirectory is null) + { + Type dirType = Type.GetType("System.IO.Directory, System.IO.FileSystem, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a", throwOnError: true)!; + MethodInfo mi = dirType.GetMethod("CreateDirectory", BindingFlags.Public | BindingFlags.Static)!; + createDirectory = mi.CreateDelegate>(); + Volatile.Write(ref s_directoryCreateDirectory, createDirectory); } + + createDirectory(path); + + return path; } private static string GetFolderPathCoreWithoutValidation(SpecialFolder folder) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs index fac7f93..bc1ea76 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs @@ -58,14 +58,6 @@ namespace System.IO // (perf optimization for successive reads of the same size) // Removing a private default constructor is a breaking change for the DataDebugSerializer. // Because this ctor was here previously we need to keep it around. - private SemaphoreSlim? _asyncActiveSemaphore; - - internal SemaphoreSlim LazyEnsureAsyncActiveSemaphoreInitialized() - { - // Lazily-initialize _asyncActiveSemaphore. As we're never accessing the SemaphoreSlim's - // WaitHandle, we don't need to worry about Disposing it. - return LazyInitializer.EnsureInitialized(ref _asyncActiveSemaphore, () => new SemaphoreSlim(1, 1)); - } public BufferedStream(Stream stream) : this(stream, DefaultBufferSize) @@ -329,8 +321,7 @@ namespace System.IO { Debug.Assert(_stream != null); - SemaphoreSlim sem = LazyEnsureAsyncActiveSemaphoreInitialized(); - await sem.WaitAsync(cancellationToken).ConfigureAwait(false); + await EnsureAsyncActiveSemaphoreInitialized().WaitAsync(cancellationToken).ConfigureAwait(false); try { if (_writePos > 0) @@ -371,7 +362,7 @@ namespace System.IO } finally { - sem.Release(); + _asyncActiveSemaphore.Release(); } } @@ -616,7 +607,7 @@ namespace System.IO // Try to satisfy the request from the buffer synchronously. But still need a sem-lock in case that another // Async IO Task accesses the buffer concurrently. If we fail to acquire the lock without waiting, make this // an Async operation. - SemaphoreSlim sem = LazyEnsureAsyncActiveSemaphoreInitialized(); + SemaphoreSlim sem = EnsureAsyncActiveSemaphoreInitialized(); Task semaphoreLockTask = sem.WaitAsync(cancellationToken); if (semaphoreLockTask.IsCompletedSuccessfully) { @@ -667,7 +658,7 @@ namespace System.IO EnsureCanRead(); int bytesFromBuffer = 0; - SemaphoreSlim sem = LazyEnsureAsyncActiveSemaphoreInitialized(); + SemaphoreSlim sem = EnsureAsyncActiveSemaphoreInitialized(); Task semaphoreLockTask = sem.WaitAsync(cancellationToken); if (semaphoreLockTask.IsCompletedSuccessfully) { @@ -706,6 +697,7 @@ namespace System.IO Debug.Assert(_stream != null); Debug.Assert(_stream.CanRead); Debug.Assert(_bufferSize > 0); + Debug.Assert(_asyncActiveSemaphore != null); Debug.Assert(semaphoreLockTask != null); // Employ async waiting based on the same synchronization used in BeginRead of the abstract Stream. @@ -750,8 +742,7 @@ namespace System.IO } finally { - SemaphoreSlim sem = LazyEnsureAsyncActiveSemaphoreInitialized(); - sem.Release(); + _asyncActiveSemaphore.Release(); } } @@ -1042,7 +1033,7 @@ namespace System.IO EnsureCanWrite(); // Try to satisfy the request from the buffer synchronously. - SemaphoreSlim sem = LazyEnsureAsyncActiveSemaphoreInitialized(); + SemaphoreSlim sem = EnsureAsyncActiveSemaphoreInitialized(); Task semaphoreLockTask = sem.WaitAsync(cancellationToken); if (semaphoreLockTask.IsCompletedSuccessfully) { @@ -1087,6 +1078,7 @@ namespace System.IO Debug.Assert(_stream != null); Debug.Assert(_stream.CanWrite); Debug.Assert(_bufferSize > 0); + Debug.Assert(_asyncActiveSemaphore != null); Debug.Assert(semaphoreLockTask != null); // See the LARGE COMMENT in Write(..) for the explanation of the write buffer algorithm. @@ -1161,8 +1153,7 @@ namespace System.IO } finally { - SemaphoreSlim sem = LazyEnsureAsyncActiveSemaphoreInitialized(); - sem.Release(); + _asyncActiveSemaphore.Release(); } } @@ -1299,7 +1290,7 @@ namespace System.IO Debug.Assert(_stream != null); // Synchronize async operations as does Read/WriteAsync. - await LazyEnsureAsyncActiveSemaphoreInitialized().WaitAsync(cancellationToken).ConfigureAwait(false); + await EnsureAsyncActiveSemaphoreInitialized().WaitAsync(cancellationToken).ConfigureAwait(false); try { int readBytes = _readLen - _readPos; @@ -1321,7 +1312,10 @@ namespace System.IO // Our buffer is now clear. Copy data directly from the source stream to the destination stream. await _stream.CopyToAsync(destination, bufferSize, cancellationToken).ConfigureAwait(false); } - finally { _asyncActiveSemaphore!.Release(); } + finally + { + _asyncActiveSemaphore.Release(); + } } } // class BufferedStream } // namespace diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs index a34ec70..cb6f006 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs @@ -3,6 +3,7 @@ using System.Buffers; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Threading; @@ -14,13 +15,18 @@ namespace System.IO { public static readonly Stream Null = new NullStream(); - /// To implement Async IO operations on streams that don't support async IO - private SemaphoreSlim? _asyncActiveSemaphore; + /// To serialize async operations on streams that don't implement their own. + private protected SemaphoreSlim? _asyncActiveSemaphore; - internal SemaphoreSlim EnsureAsyncActiveSemaphoreInitialized() => + [MemberNotNull(nameof(_asyncActiveSemaphore))] + 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. - LazyInitializer.EnsureInitialized(ref _asyncActiveSemaphore, () => new SemaphoreSlim(1, 1)); +#pragma warning disable CS8774 // We lack a NullIffNull annotation for Volatile.Read + Volatile.Read(ref _asyncActiveSemaphore) ?? +#pragma warning restore CS8774 + Interlocked.CompareExchange(ref _asyncActiveSemaphore, new SemaphoreSlim(1, 1), null) ?? + _asyncActiveSemaphore; public abstract bool CanRead { get; } public abstract bool CanWrite { get; } diff --git a/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.Core.cs b/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.Core.cs index bb2cea4..03f1be6 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.Core.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.Core.cs @@ -49,7 +49,7 @@ namespace System.Resources throw new NotSupportedException(SR.NotSupported_ResourceObjectSerialization); } - if (_binaryFormatter == null) + if (Volatile.Read(ref _binaryFormatter) is null) { if (!InitializeBinaryFormatter()) { @@ -78,24 +78,23 @@ namespace System.Resources // If BinaryFormatter support is disabled for the app, the linker will replace this entire // method body with "return false;", skipping all reflection code below. - LazyInitializer.EnsureInitialized(ref s_binaryFormatterType, static () => - Type.GetType("System.Runtime.Serialization.Formatters.Binary.BinaryFormatter, System.Runtime.Serialization.Formatters, Version=0.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a", - throwOnError: true)!); - - LazyInitializer.EnsureInitialized(ref s_deserializeMethod, static () => + if (Volatile.Read(ref s_binaryFormatterType) is null || Volatile.Read(ref s_deserializeMethod) is null) { - MethodInfo binaryFormatterDeserialize = s_binaryFormatterType!.GetMethod("Deserialize", new Type[] { typeof(Stream) })!; - - // create an unbound delegate that can accept a BinaryFormatter instance as object - return (Func)typeof(ResourceReader) - .GetMethod(nameof(CreateUntypedDelegate), BindingFlags.NonPublic | BindingFlags.Static)! - .MakeGenericMethod(s_binaryFormatterType) - .Invoke(null, new object[] { binaryFormatterDeserialize })!; - }); + Type binaryFormatterType = Type.GetType("System.Runtime.Serialization.Formatters.Binary.BinaryFormatter, System.Runtime.Serialization.Formatters", throwOnError: true)!; + MethodInfo? binaryFormatterDeserialize = binaryFormatterType.GetMethod("Deserialize", new[] { typeof(Stream) }); + Func? deserializeMethod = (Func?) + typeof(ResourceReader) + .GetMethod(nameof(CreateUntypedDelegate), BindingFlags.NonPublic | BindingFlags.Static) + ?.MakeGenericMethod(binaryFormatterType) + .Invoke(null, new[] { binaryFormatterDeserialize }); + + Interlocked.CompareExchange(ref s_binaryFormatterType, binaryFormatterType, null); + Interlocked.CompareExchange(ref s_deserializeMethod, deserializeMethod, null); + } - _binaryFormatter = Activator.CreateInstance(s_binaryFormatterType!)!; + Volatile.Write(ref _binaryFormatter, Activator.CreateInstance(s_binaryFormatterType!)); - return true; // initialization successful + return s_deserializeMethod != null; } // generic method that we specialize at runtime once we've loaded the BinaryFormatter type diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ConcurrentExclusiveSchedulerPair.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ConcurrentExclusiveSchedulerPair.cs index cec1ef4..505fd06 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ConcurrentExclusiveSchedulerPair.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ConcurrentExclusiveSchedulerPair.cs @@ -143,9 +143,17 @@ namespace System.Threading.Tasks public Task Completion => EnsureCompletionStateInitialized(); /// Gets the lazily-initialized completion state. - private CompletionState EnsureCompletionStateInitialized() => - // ValueLock not needed, but it's ok if it's held - LazyInitializer.EnsureInitialized(ref m_completionState, () => new CompletionState()); + private CompletionState EnsureCompletionStateInitialized() + { + return Volatile.Read(ref m_completionState) ?? InitializeCompletionState(); + + CompletionState InitializeCompletionState() + { + // ValueLock not needed, but it's ok if it's held + Interlocked.CompareExchange(ref m_completionState, new CompletionState(), null); + return m_completionState; + } + } /// Gets whether completion has been requested. private bool CompletionRequested => m_completionState != null && Volatile.Read(ref m_completionState.m_completionRequested); diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs index 27a785f..5ae5778 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs @@ -192,12 +192,15 @@ namespace System.Threading.Tasks { Debug.Assert(task != null, "Null Task objects can't be added to the ActiveTasks collection"); - LazyInitializer.EnsureInitialized(ref s_currentActiveTasks, () => new Dictionary()); + Dictionary activeTasks = + Volatile.Read(ref s_currentActiveTasks) ?? + Interlocked.CompareExchange(ref s_currentActiveTasks, new Dictionary(), null) ?? + s_currentActiveTasks; int taskId = task.Id; - lock (s_currentActiveTasks) + lock (activeTasks) { - s_currentActiveTasks[taskId] = task; + activeTasks[taskId] = task; } // always return true to keep signature as bool for backwards compatibility return true; @@ -205,13 +208,14 @@ namespace System.Threading.Tasks internal static void RemoveFromActiveTasks(Task task) { - if (s_currentActiveTasks == null) + Dictionary? activeTasks = s_currentActiveTasks; + if (activeTasks is null) return; int taskId = task.Id; - lock (s_currentActiveTasks) + lock (activeTasks) { - s_currentActiveTasks.Remove(taskId); + activeTasks.Remove(taskId); } } @@ -1316,7 +1320,13 @@ namespace System.Threading.Tasks /// The initialized contingent properties object. internal ContingentProperties EnsureContingentPropertiesInitialized() { - return LazyInitializer.EnsureInitialized(ref m_contingentProperties, () => new ContingentProperties()); + return Volatile.Read(ref m_contingentProperties) ?? InitializeContingentProperties(); + + ContingentProperties InitializeContingentProperties() + { + Interlocked.CompareExchange(ref m_contingentProperties, new ContingentProperties(), null); + return m_contingentProperties; + } } /// diff --git a/src/mono/netcore/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.Mono.cs b/src/mono/netcore/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.Mono.cs index 7d50fa0..7e46c37 100644 --- a/src/mono/netcore/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.Mono.cs +++ b/src/mono/netcore/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.Mono.cs @@ -336,15 +336,15 @@ namespace System.Runtime.InteropServices { var key = (type, cookie); - LazyInitializer.EnsureInitialized( - ref MarshalerInstanceCache, - () => new Dictionary<(Type, string), ICustomMarshaler>(new MarshalerInstanceKeyComparer()) - ); + Dictionary<(Type, string), ICustomMarshaler> cache = + Volatile.Read(ref MarshalerInstanceCache) ?? + Interlocked.CompareExchange(ref MarshalerInstanceCache, new Dictionary<(Type, string), ICustomMarshaler>(new MarshalerInstanceKeyComparer()), null) ?? + MarshalerInstanceCache; ICustomMarshaler? result; bool gotExistingInstance; - lock (MarshalerInstanceCache) - gotExistingInstance = MarshalerInstanceCache.TryGetValue(key, out result); + lock (cache) + gotExistingInstance = cache.TryGetValue(key, out result); if (!gotExistingInstance) { @@ -389,8 +389,8 @@ namespace System.Runtime.InteropServices if (result == null) throw new ApplicationException($"A call to GetInstance() for custom marshaler '{type.FullName}' returned null, which is not allowed."); - lock (MarshalerInstanceCache) - MarshalerInstanceCache[key] = result; + lock (cache) + cache[key] = result; } return result; diff --git a/src/mono/netcore/System.Private.CoreLib/src/System/RuntimeType.Mono.cs b/src/mono/netcore/System.Private.CoreLib/src/System/RuntimeType.Mono.cs index f3f1865..a5f0724 100644 --- a/src/mono/netcore/System.Private.CoreLib/src/System/RuntimeType.Mono.cs +++ b/src/mono/netcore/System.Private.CoreLib/src/System/RuntimeType.Mono.cs @@ -1822,18 +1822,12 @@ namespace System #endregion - private TypeCache cache; + private TypeCache? cache; - internal TypeCache Cache - { - get - { - if (cache == null) - LazyInitializer.EnsureInitialized(ref cache, () => new TypeCache()); - - return cache; - } - } + internal TypeCache Cache => + Volatile.Read(ref cache) ?? + Interlocked.CompareExchange(ref cache, new TypeCache(), null) ?? + cache; internal sealed class TypeCache { -- 2.7.4