From 7eb18358e9f945957195eac38746b1edec6a1bfa Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 11 Nov 2020 20:41:03 -0500 Subject: [PATCH] Remove LazyInitializer usage from corelib 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. --- .../Environment.GetFolderPathCore.Unix.cs | 23 +++++++------ .../src/System/IO/BufferedStream.cs | 34 ++++++++----------- .../src/System/IO/Stream.cs | 14 +++++--- .../System/Resources/ResourceReader.Core.cs | 31 ++++++++--------- .../Tasks/ConcurrentExclusiveSchedulerPair.cs | 14 ++++++-- .../src/System/Threading/Tasks/Task.cs | 24 +++++++++---- .../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 6e5e7ebc050b3b..4ce955a3c401a0 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 @@ private static string GetFolderPathCore(SpecialFolder folder, SpecialFolderOptio { 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 fac7f93f687b8d..bc1ea76470377d 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 @@ public sealed class BufferedStream : Stream // (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 @@ private async Task FlushAsyncInternal(CancellationToken cancellationToken) { 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 @@ private async Task FlushAsyncInternal(CancellationToken cancellationToken) } finally { - sem.Release(); + _asyncActiveSemaphore.Release(); } } @@ -616,7 +607,7 @@ public override Task ReadAsync(byte[] buffer, int offset, int count, Cancel // 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 @@ public override ValueTask ReadAsync(Memory buffer, CancellationToken EnsureCanRead(); int bytesFromBuffer = 0; - SemaphoreSlim sem = LazyEnsureAsyncActiveSemaphoreInitialized(); + SemaphoreSlim sem = EnsureAsyncActiveSemaphoreInitialized(); Task semaphoreLockTask = sem.WaitAsync(cancellationToken); if (semaphoreLockTask.IsCompletedSuccessfully) { @@ -706,6 +697,7 @@ private async ValueTask ReadFromUnderlyingStreamAsync( 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 @@ private async ValueTask ReadFromUnderlyingStreamAsync( } finally { - SemaphoreSlim sem = LazyEnsureAsyncActiveSemaphoreInitialized(); - sem.Release(); + _asyncActiveSemaphore.Release(); } } @@ -1042,7 +1033,7 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo 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 @@ private async ValueTask WriteToUnderlyingStreamAsync( 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 @@ private async ValueTask WriteToUnderlyingStreamAsync( } finally { - SemaphoreSlim sem = LazyEnsureAsyncActiveSemaphoreInitialized(); - sem.Release(); + _asyncActiveSemaphore.Release(); } } @@ -1299,7 +1290,7 @@ private async Task CopyToAsyncCore(Stream destination, int bufferSize, Cancellat 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 @@ private async Task CopyToAsyncCore(Stream destination, int bufferSize, Cancellat // 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 a34ec709b6dac9..cb6f0066681924 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 @@ public abstract partial class Stream : MarshalByRefObject, IDisposable, IAsyncDi { 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 bb2cea4df74978..03f1be63d143b9 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 @@ private object DeserializeObject(int typeIndex) throw new NotSupportedException(SR.NotSupported_ResourceObjectSerialization); } - if (_binaryFormatter == null) + if (Volatile.Read(ref _binaryFormatter) is null) { if (!InitializeBinaryFormatter()) { @@ -78,24 +78,23 @@ private bool InitializeBinaryFormatter() // 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 cec1ef48a1c090..505fd06768da84 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 @@ public void Complete() 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 27a785f7e658e3..5ae5778409bb42 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 @@ internal static bool AddToActiveTasks(Task task) { 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 @@ internal static bool AddToActiveTasks(Task task) 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 @@ internal bool IsCancellationRequested /// 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 7d50fa0d28f655..7e46c37bb8b3f4 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 @@ public int GetHashCode((Type, string) key) { 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 @@ public int GetHashCode((Type, string) key) 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 f3f18653d97e2c..a5f0724cb7431d 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 @@ private void CreateInstanceCheckThis() #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 {