From c423419590ae10c7c9f18d0a9da34f569cf4c1e7 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Sun, 21 May 2017 02:59:18 -0500 Subject: [PATCH] Updates based on code review feedback --- ...yStorageServiceFactory.MemoryMappedInfo.cs | 68 ++++++++----------- ...rviceFactory.ReferenceCountedDisposable.cs | 13 ++-- .../TemporaryStorageServiceFactory.cs | 6 +- 3 files changed, 41 insertions(+), 46 deletions(-) diff --git a/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.MemoryMappedInfo.cs b/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.MemoryMappedInfo.cs index 5ec5ecf468c..fbfa824f233 100644 --- a/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.MemoryMappedInfo.cs +++ b/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.MemoryMappedInfo.cs @@ -33,23 +33,6 @@ internal partial class TemporaryStorageServiceFactory /// internal sealed class MemoryMappedInfo : IDisposable { - /// - /// The name of the memory mapped file. - /// - private readonly string _name; - - /// - /// The offset into the memory mapped file of the region described by the current - /// . - /// - private readonly long _offset; - - /// - /// The size of the region of the memory mapped file described by the current - /// . - /// - private readonly long _size; - /// /// The memory mapped file. /// @@ -61,7 +44,7 @@ internal sealed class MemoryMappedInfo : IDisposable private readonly ReferenceCountedDisposable _memoryMappedFile; /// - /// actual memory accessor that owns the VM + /// A weak reference to the actual memory accessor that owns the VM /// /// /// This holds a weak counted reference to current , which @@ -71,31 +54,37 @@ internal sealed class MemoryMappedInfo : IDisposable /// unmapped, making the process address space it previously claimed available for other purposes. If/when /// it is needed again, a new view is created. /// - private ReferenceCountedDisposable.WeakReference _accessor; + private ReferenceCountedDisposable.WeakReference _weakAccessor; public MemoryMappedInfo(ReferenceCountedDisposable memoryMappedFile, string name, long offset, long size) { _memoryMappedFile = memoryMappedFile; - _name = name; - _offset = offset; - _size = size; + Name = name; + Offset = offset; + Size = size; } public MemoryMappedInfo(string name, long offset, long size) + : this(new ReferenceCountedDisposable(MemoryMappedFile.OpenExisting(name)), name, offset, size) { - _name = name; - _offset = offset; - _size = size; - - _memoryMappedFile = new ReferenceCountedDisposable(MemoryMappedFile.OpenExisting(_name)); } /// - /// Name and Size of memory map file + /// The name of the memory mapped file. + /// + public string Name { get; } + + /// + /// The offset into the memory mapped file of the region described by the current + /// . + /// + public long Offset { get; } + + /// + /// The size of the region of the memory mapped file described by the current + /// . /// - public string Name => _name; - public long Offset => _offset; - public long Size => _size; + public long Size { get; } private static void ForceCompactingGC() { @@ -116,18 +105,19 @@ public Stream CreateReadableStream() // CreateViewAccessor is not guaranteed to be thread-safe lock (_memoryMappedFile) { - // Note: TryAddReference will not return a non-null but invalid reference, even if the current - // object has been disposed (see comments on _memoryMappedFile and TryAddReference). - var streamAccessor = _accessor.TryAddReference(); + // Note: TryAddReference behaves according to its documentation even if the target object has been + // disposed. If it returns non-null, then the object will not be disposed before the returned + // reference is disposed (see comments on _memoryMappedFile and TryAddReference). + var streamAccessor = _weakAccessor.TryAddReference(); if (streamAccessor == null) { - var rawAccessor = RunWithCompactingGCFallback(info => info._memoryMappedFile.Target.CreateViewAccessor(info._offset, info._size, MemoryMappedFileAccess.Read), this); + var rawAccessor = RunWithCompactingGCFallback(info => info._memoryMappedFile.Target.CreateViewAccessor(info.Offset, info.Size, MemoryMappedFileAccess.Read), this); streamAccessor = new ReferenceCountedDisposable(rawAccessor); - _accessor = new ReferenceCountedDisposable.WeakReference(streamAccessor); + _weakAccessor = new ReferenceCountedDisposable.WeakReference(streamAccessor); } Contract.Assert(streamAccessor.Target.CanRead); - return new SharedReadableStream(this, streamAccessor, _size); + return new SharedReadableStream(this, streamAccessor, Size); } } @@ -140,7 +130,7 @@ public Stream CreateWritableStream() // CreateViewStream is not guaranteed to be thread-safe lock (_memoryMappedFile) { - return RunWithCompactingGCFallback(info => info._memoryMappedFile.Target.CreateViewStream(info._offset, info._size, MemoryMappedFileAccess.Write), this); + return RunWithCompactingGCFallback(info => info._memoryMappedFile.Target.CreateViewStream(info.Offset, info.Size, MemoryMappedFileAccess.Write), this); } } @@ -185,7 +175,7 @@ private void Dispose(bool disposing) if (disposing) { // See remarks on field for relation between _memoryMappedFile and the views/streams. There is no - // need to write _accessor here since the types involved adhere to their contracts even in + // need to write _weakAccessor here since the types involved adhere to their contracts even in // concurrent code. _memoryMappedFile.Dispose(); } diff --git a/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.ReferenceCountedDisposable.cs b/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.ReferenceCountedDisposable.cs index 58ccdfe660c..278129b06a3 100644 --- a/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.ReferenceCountedDisposable.cs +++ b/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.ReferenceCountedDisposable.cs @@ -52,6 +52,11 @@ internal sealed class ReferenceCountedDisposable : IDisposable /// The boxed reference count, which is shared by all references with the same object. /// /// + /// Only use equality operators to compare this value with 0. The actual reference count is allowed to + /// be a negative integer in order to support a reference count full 32-bit number of reference. Ideally it + /// would be represented as a , but some operations are not + /// implemented for this type. + /// /// This field is set to at the point in time when this reference is disposed. /// This occurs prior to clearing the field in order to support concurrent /// code. @@ -207,7 +212,7 @@ public struct WeakReference /// /// DO NOT DISPOSE OF THE TARGET. /// - private readonly WeakReference _instance; + private readonly WeakReference _weakInstance; private readonly StrongBox _boxedReferenceCount; public WeakReference(ReferenceCountedDisposable reference) @@ -226,7 +231,7 @@ public WeakReference(ReferenceCountedDisposable reference) return; } - _instance = new WeakReference(instance); + _weakInstance = new WeakReference(instance); _boxedReferenceCount = referenceCount; } @@ -247,8 +252,8 @@ public WeakReference(ReferenceCountedDisposable reference) /// already been disposed. public ReferenceCountedDisposable TryAddReference() { - var instance = _instance; - if (instance == null || !_instance.TryGetTarget(out var target)) + var weakInstance = _weakInstance; + if (weakInstance == null || !_weakInstance.TryGetTarget(out var target)) { return null; } diff --git a/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.cs b/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.cs index 25544f8ee85..bc7d02953c9 100644 --- a/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.cs +++ b/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.cs @@ -100,7 +100,7 @@ private MemoryMappedInfo CreateTemporaryStorage(long size) // mapped file is obtained in this section, it must either be disposed within the loop or returned // to the caller who will own it through the MemoryMappedInfo. var storage = Volatile.Read(ref _storage); - var reference = storage?.FileReference.TryAddReference(); + var reference = storage?.WeakFileReference.TryAddReference(); if (reference == null) { var oldStorage = storage; @@ -140,14 +140,14 @@ public static string CreateUniqueName(long size) /// private sealed class MemoryMappedFileStorage { - public readonly ReferenceCountedDisposable.WeakReference FileReference; + public readonly ReferenceCountedDisposable.WeakReference WeakFileReference; public readonly string Name; public readonly long Size; public long Offset; private MemoryMappedFileStorage(ReferenceCountedDisposable.WeakReference memoryMappedFile, string name, long size) { - FileReference = memoryMappedFile; + WeakFileReference = memoryMappedFile; Name = name; Size = size; } -- GitLab