Skip to content

Commit

Permalink
Remove LazyInitializer usage from corelib (#44409)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
stephentoub authored Nov 12, 2020
1 parent 3b5a94c commit 731dee4
Show file tree
Hide file tree
Showing 8 changed files with 92 additions and 80 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,20 +37,21 @@ private static string GetFolderPathCore(SpecialFolder folder, SpecialFolderOptio
{
return string.Empty;
}
else
{
Debug.Assert(option == SpecialFolderOption.Create);

Func<string, object> 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<Func<string, object>>();
});
createDirectory(path);
Debug.Assert(option == SpecialFolderOption.Create);

return path;
Func<string, object>? 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<Func<string, object>>();
Volatile.Write(ref s_directoryCreateDirectory, createDirectory);
}

createDirectory(path);

return path;
}

private static string GetFolderPathCoreWithoutValidation(SpecialFolder folder)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<SemaphoreSlim>(ref _asyncActiveSemaphore, () => new SemaphoreSlim(1, 1));
}

public BufferedStream(Stream stream)
: this(stream, DefaultBufferSize)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -371,7 +362,7 @@ private async Task FlushAsyncInternal(CancellationToken cancellationToken)
}
finally
{
sem.Release();
_asyncActiveSemaphore.Release();
}
}

Expand Down Expand Up @@ -616,7 +607,7 @@ public override Task<int> 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)
{
Expand Down Expand Up @@ -667,7 +658,7 @@ public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken
EnsureCanRead();

int bytesFromBuffer = 0;
SemaphoreSlim sem = LazyEnsureAsyncActiveSemaphoreInitialized();
SemaphoreSlim sem = EnsureAsyncActiveSemaphoreInitialized();
Task semaphoreLockTask = sem.WaitAsync(cancellationToken);
if (semaphoreLockTask.IsCompletedSuccessfully)
{
Expand Down Expand Up @@ -706,6 +697,7 @@ private async ValueTask<int> 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.
Expand Down Expand Up @@ -750,8 +742,7 @@ private async ValueTask<int> ReadFromUnderlyingStreamAsync(
}
finally
{
SemaphoreSlim sem = LazyEnsureAsyncActiveSemaphoreInitialized();
sem.Release();
_asyncActiveSemaphore.Release();
}
}

Expand Down Expand Up @@ -1042,7 +1033,7 @@ public override ValueTask WriteAsync(ReadOnlyMemory<byte> 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)
{
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -1161,8 +1153,7 @@ private async ValueTask WriteToUnderlyingStreamAsync(
}
finally
{
SemaphoreSlim sem = LazyEnsureAsyncActiveSemaphoreInitialized();
sem.Release();
_asyncActiveSemaphore.Release();
}
}

Expand Down Expand Up @@ -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;
Expand All @@ -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
14 changes: 10 additions & 4 deletions src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -14,13 +15,18 @@ public abstract partial class Stream : MarshalByRefObject, IDisposable, IAsyncDi
{
public static readonly Stream Null = new NullStream();

/// <summary>To implement Async IO operations on streams that don't support async IO</summary>
private SemaphoreSlim? _asyncActiveSemaphore;
/// <summary>To serialize async operations on streams that don't implement their own.</summary>
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; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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())
{
Expand Down Expand Up @@ -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<object?, Stream, object>)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<object?, Stream, object>? deserializeMethod = (Func<object?, Stream, object>?)
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,17 @@ public void Complete()
public Task Completion => EnsureCompletionStateInitialized();

/// <summary>Gets the lazily-initialized completion state.</summary>
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;
}
}

/// <summary>Gets whether completion has been requested.</summary>
private bool CompletionRequested => m_completionState != null && Volatile.Read(ref m_completionState.m_completionRequested);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,26 +192,30 @@ 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<int, Task>());
Dictionary<int, Task> activeTasks =
Volatile.Read(ref s_currentActiveTasks) ??
Interlocked.CompareExchange(ref s_currentActiveTasks, new Dictionary<int, Task>(), 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;
}

internal static void RemoveFromActiveTasks(Task task)
{
if (s_currentActiveTasks == null)
Dictionary<int, Task>? activeTasks = s_currentActiveTasks;
if (activeTasks is null)
return;

int taskId = task.Id;
lock (s_currentActiveTasks)
lock (activeTasks)
{
s_currentActiveTasks.Remove(taskId);
activeTasks.Remove(taskId);
}
}

Expand Down Expand Up @@ -1316,7 +1320,13 @@ internal bool IsCancellationRequested
/// <returns>The initialized contingent properties object.</returns>
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;
}
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down

0 comments on commit 731dee4

Please sign in to comment.