From c9c632552ccfb9c9322435735bbe8a029321d27e Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 22 Jun 2021 19:03:43 +0200 Subject: [PATCH] Update API surface to match approved specs from API review --- .../src/System/Runtime/DependentHandle.cs | 79 ++++++------ src/coreclr/vm/comdependenthandle.cpp | 8 +- src/coreclr/vm/comdependenthandle.h | 2 +- src/coreclr/vm/ecalllist.h | 2 +- .../CompilerServices/ConditionalWeakTable.cs | 2 +- .../System.Runtime/ref/System.Runtime.cs | 5 +- .../System/Runtime/DependentHandleTests.cs | 118 +++++++++++------- .../src/System/Runtime/DependentHandle.cs | 40 ++++-- 8 files changed, 142 insertions(+), 114 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs index 8ae58eba73820..8c78252af2c23 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -70,9 +70,12 @@ public DependentHandle(object? target, object? dependent) public bool IsAllocated => (nint)_handle != 0; /// - /// Gets the target object instance for the current handle. + /// Gets or sets the target object instance for the current handle. The target can only be set to a value + /// once the instance has been created. Doing so will cause to start + /// returning as well, and to become eligible for collection even if the previous target is still alive. /// - /// Thrown if is . + /// + /// Thrown if is or if the input value is not . /// This property is thread-safe. public object? Target { @@ -87,6 +90,17 @@ public object? Target return InternalGetTarget(handle); } + set + { + IntPtr handle = _handle; + + if ((nint)handle == 0 || value is not null) + { + ThrowHelper.ThrowInvalidOperationException(); + } + + InternalSetTargetToNull(handle); + } } /// @@ -127,7 +141,7 @@ public object? Dependent } /// - /// Atomically retrieves the values of both and , if available. + /// Gets the values of both and (if available) as an atomic operation. /// That is, even if is concurrently set to , calling this method /// will either return for both target and dependent, or return both previous values. /// If and were used sequentially in this scenario instead, it's @@ -135,41 +149,21 @@ public object? Dependent /// /// The values of and . /// Thrown if is . - public (object? Target, object? Dependent) GetTargetAndDependent() + public (object? Target, object? Dependent) TargetAndDependent { - IntPtr handle = _handle; - - if ((nint)handle == 0) + get { - ThrowHelper.ThrowInvalidOperationException(); - } - - object? target = InternalGetTargetAndDependent(handle, out object? dependent); + IntPtr handle = _handle; - return (target, dependent); - } + if ((nint)handle == 0) + { + ThrowHelper.ThrowInvalidOperationException(); + } - /// - /// Stops tracking the target and dependent objects in the current instance. Once this method - /// is invoked, calling other APIs is still allowed, but and will always just - /// return . Additionally, since the dependent instance will no longer be tracked, it will also - /// immediately become eligible for collection if there are no other roots for it, even if the original target is still alive. - /// - /// Thrown if is . - /// - /// This method does not dispose the current instance, which means that after calling it will not - /// affect the value of , and will still need to be called to release resources. - /// - public void StopTracking() - { - IntPtr handle = _handle; + object? target = InternalGetTargetAndDependent(handle, out object? dependent); - if ((nint)handle == 0) - { - ThrowHelper.ThrowInvalidOperationException(); + return (target, dependent); } - - InternalStopTracking(handle); } /// @@ -188,7 +182,7 @@ public void StopTracking() /// The dependent instance, if available. /// The values of and . /// - /// This method mirrors , but without the allocation check. + /// This method mirrors the property, but without the allocation check. /// The signature is also kept the same as the one for the internal call, to improve the codegen. /// Note that is required to be on the stack (or it might not be tracked). /// @@ -198,22 +192,21 @@ public void StopTracking() } /// - /// Sets the dependent object instance for the current handle. + /// Sets the dependent object instance for the current handle to . /// - /// This method mirrors , but without the allocation check. - internal void UnsafeSetDependent(object? dependent) + /// This method mirrors the setter, but without allocation and input checks. + internal void UnsafeSetTargetToNull() { - InternalSetDependent(_handle, dependent); + InternalSetTargetToNull(_handle); } /// - /// Stops tracking the target and dependent objects in the current instance. + /// Sets the dependent object instance for the current handle. /// - /// Thrown if is . - /// This method mirrors , but without the allocation check. - internal void UnsafeStopTracking() + /// This method mirrors , but without the allocation check. + internal void UnsafeSetDependent(object? dependent) { - InternalStopTracking(_handle); + InternalSetDependent(_handle, dependent); } /// @@ -258,7 +251,7 @@ public void Dispose() private static extern void InternalSetDependent(IntPtr dependentHandle, object? dependent); [MethodImpl(MethodImplOptions.InternalCall)] - private static extern void InternalStopTracking(IntPtr dependentHandle); + private static extern void InternalSetTargetToNull(IntPtr dependentHandle); [MethodImpl(MethodImplOptions.InternalCall)] private static extern void InternalFree(IntPtr dependentHandle); diff --git a/src/coreclr/vm/comdependenthandle.cpp b/src/coreclr/vm/comdependenthandle.cpp index 1f59149920ddc..b222190810233 100644 --- a/src/coreclr/vm/comdependenthandle.cpp +++ b/src/coreclr/vm/comdependenthandle.cpp @@ -75,25 +75,25 @@ FCIMPL2(Object*, DependentHandle::InternalGetTargetAndDependent, OBJECTHANDLE ha } FCIMPLEND -FCIMPL2(VOID, DependentHandle::InternalSetDependent, OBJECTHANDLE handle, Object *_dependent) +FCIMPL1(VOID, DependentHandle::InternalSetTargetToNull, OBJECTHANDLE handle) { FCALL_CONTRACT; _ASSERTE(handle != NULL); IGCHandleManager *mgr = GCHandleUtilities::GetGCHandleManager(); - mgr->SetDependentHandleSecondary(handle, _dependent); + mgr->StoreObjectInHandle(handle, NULL); } FCIMPLEND -FCIMPL1(VOID, DependentHandle::InternalStopTracking, OBJECTHANDLE handle) +FCIMPL2(VOID, DependentHandle::InternalSetDependent, OBJECTHANDLE handle, Object *_dependent) { FCALL_CONTRACT; _ASSERTE(handle != NULL); IGCHandleManager *mgr = GCHandleUtilities::GetGCHandleManager(); - mgr->StoreObjectInHandle(handle, NULL); + mgr->SetDependentHandleSecondary(handle, _dependent); } FCIMPLEND diff --git a/src/coreclr/vm/comdependenthandle.h b/src/coreclr/vm/comdependenthandle.h index 827e2ff7b69e2..06786e6257746 100644 --- a/src/coreclr/vm/comdependenthandle.h +++ b/src/coreclr/vm/comdependenthandle.h @@ -44,8 +44,8 @@ class DependentHandle static FCDECL1(Object *, InternalGetTarget, OBJECTHANDLE handle); static FCDECL1(Object *, InternalGetDependent, OBJECTHANDLE handle); static FCDECL2(Object *, InternalGetTargetAndDependent, OBJECTHANDLE handle, Object **outDependent); + static FCDECL1(VOID, InternalSetTargetToNull, OBJECTHANDLE handle); static FCDECL2(VOID, InternalSetDependent, OBJECTHANDLE handle, Object *dependent); - static FCDECL1(VOID, InternalStopTracking, OBJECTHANDLE handle); static FCDECL1(VOID, InternalFree, OBJECTHANDLE handle); }; diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 488836247f7cd..8ff8a4a5cc3d0 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -65,8 +65,8 @@ FCFuncStart(gDependentHandleFuncs) FCFuncElement("InternalGetTarget", DependentHandle::InternalGetTarget) FCFuncElement("InternalGetDependent", DependentHandle::InternalGetDependent) FCFuncElement("InternalGetTargetAndDependent", DependentHandle::InternalGetTargetAndDependent) + FCFuncElement("InternalSetTargetToNull", DependentHandle::InternalSetTargetToNull) FCFuncElement("InternalSetDependent", DependentHandle::InternalSetDependent) - FCFuncElement("InternalStopTracking", DependentHandle::InternalStopTracking) FCFuncElement("InternalFree", DependentHandle::InternalFree) FCFuncEnd() diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConditionalWeakTable.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConditionalWeakTable.cs index 35acec9546988..88b8187f3dfd2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConditionalWeakTable.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConditionalWeakTable.cs @@ -595,7 +595,7 @@ private void RemoveIndex(int entryIndex) Volatile.Write(ref entry.HashCode, -1); // Also, clear the key to allow GC to collect objects pointed to by the entry - entry.depHnd.UnsafeStopTracking(); + entry.depHnd.UnsafeSetTargetToNull(); } internal void UpdateValue(int entryIndex, TValue newValue) diff --git a/src/libraries/System.Runtime/ref/System.Runtime.cs b/src/libraries/System.Runtime/ref/System.Runtime.cs index 14e91b725e4ad..580f9955b65db 100644 --- a/src/libraries/System.Runtime/ref/System.Runtime.cs +++ b/src/libraries/System.Runtime/ref/System.Runtime.cs @@ -9600,10 +9600,9 @@ public partial struct DependentHandle : System.IDisposable public DependentHandle(object? target, object? dependent) { throw null; } public object? Dependent { get { throw null; } set { } } public bool IsAllocated { get { throw null; } } - public object? Target { get { throw null; } } + public object? Target { get { throw null; } set { } } + public (object? Target, object? Dependent) TargetAndDependent { get { throw null; } } public void Dispose() { } - public (object? Target, object? Dependent) GetTargetAndDependent() { throw null; } - public void StopTracking() { throw null; } } public enum GCLargeObjectHeapCompactionMode { diff --git a/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs b/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs index 94c2afed33bf9..2ee4eca51d48f 100644 --- a/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs +++ b/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs @@ -37,6 +37,48 @@ public void GetNotNullTarget() handle.Dispose(); } + [Fact] + public void SetTargetToNull_StateIsConsistent() + { + object target = new(), dependent = new(); + DependentHandle handle = new(target, dependent); + + Assert.True(handle.IsAllocated); + Assert.Same(handle.Target, target); + Assert.Same(handle.Dependent, dependent); + + handle.Target = null; + + Assert.True(handle.IsAllocated); + Assert.Null(handle.Target); + Assert.Null(handle.Dependent); + + handle.Dispose(); + } + + [Fact] + public void SetTargetToNull_RepeatedCallsAreFine() + { + object target = new(), dependent = new(); + DependentHandle handle = new(target, dependent); + + handle.Target = null; + + Assert.True(handle.IsAllocated); + Assert.Null(handle.Target); + Assert.Null(handle.Dependent); + + handle.Target = null; + handle.Target = null; + handle.Target = null; + + Assert.True(handle.IsAllocated); + Assert.Null(handle.Target); + Assert.Null(handle.Dependent); + + handle.Dispose(); + } + [Fact] public void GetSetDependent() { @@ -175,50 +217,8 @@ private sealed class ObjectWithReference public object Reference; } - [Fact] - public void StopTracking_RepeatedCallsAreFine() - { - object target = new(), dependent = new(); - DependentHandle handle = new(target, dependent); - - handle.StopTracking(); - - Assert.True(handle.IsAllocated); - Assert.Null(handle.Target); - Assert.Null(handle.Dependent); - - handle.StopTracking(); - handle.StopTracking(); - handle.StopTracking(); - - Assert.True(handle.IsAllocated); - Assert.Null(handle.Target); - Assert.Null(handle.Dependent); - - handle.Dispose(); - } - - [Fact] - public void StopTracking_StateIsConsistent() - { - object target = new(), dependent = new(); - DependentHandle handle = new(target, dependent); - - Assert.True(handle.IsAllocated); - Assert.Same(handle.Target, target); - Assert.Same(handle.Dependent, dependent); - - handle.StopTracking(); - - Assert.True(handle.IsAllocated); - Assert.Null(handle.Target); - Assert.Null(handle.Dependent); - - handle.Dispose(); - } - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))] - public void DependentIsCollectedAfterStopTracking() + public void DependentIsCollectedAfterTargetIsSetToNull() { [MethodImpl(MethodImplOptions.NoInlining)] static DependentHandle Initialize(out object target, out WeakReference weakDependent) @@ -234,7 +234,7 @@ static DependentHandle Initialize(out object target, out WeakReference weakDepen DependentHandle handle = Initialize(out object target, out WeakReference dependent); - handle.StopTracking(); + handle.Target = null; GC.Collect(); @@ -263,19 +263,41 @@ public void GetDependent_ThrowsInvalidOperationException() } [Fact] - public void SetDependent_ThrowsInvalidOperationException() + public void SetTarget_NotAllocated_ThrowsInvalidOperationException() { Assert.Throws(() => { DependentHandle handle = default; - handle.Dependent = new(); + handle.Target = new(); }); } [Fact] - public void StopTracking_ThrowsInvalidOperationException() + public void SetTarget_NotNullObject_ThrowsInvalidOperationException() { - Assert.Throws(() => default(DependentHandle).StopTracking()); + Assert.Throws(() => + { + DependentHandle handle = default; + + try + { + handle.Target = new(); + } + finally + { + handle.Dispose(); + } + }); + } + + [Fact] + public void SetDependent_ThrowsInvalidOperationException() + { + Assert.Throws(() => + { + DependentHandle handle = default; + handle.Dependent = new(); + }); } [Fact] diff --git a/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs b/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs index a0a7e26c72b3d..8cfd9b013e52c 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -26,7 +26,23 @@ public DependentHandle(object? target, object? dependent) public bool IsAllocated => _data is not null; - public object? Target => UnsafeGetTarget(); + public object? Target + { + get => UnsafeGetTarget(); + set + { + Ephemeron[]? data = _data; + + if (data is null || value is not null) + { + ThrowHelper.ThrowInvalidOperationException(); + + return; + } + + data[0].Key = null; + } + } public object? Dependent { @@ -34,16 +50,14 @@ public object? Dependent set => UnsafeSetDependent(value); } - public (object? Target, object? Dependent) GetTargetAndDependent() + public (object? Target, object? Dependent) TargetAndDependent { - object? target = UnsafeGetTargetAndDependent(out object? dependent); - - return (target, dependent); - } + get + { + object? target = UnsafeGetTargetAndDependent(out object? dependent); - public void StopTracking() - { - UnsafeStopTracking(); + return (target, dependent); + } } internal object? UnsafeGetTarget() @@ -105,7 +119,7 @@ public void StopTracking() return null; } - internal void UnsafeSetDependent(object? dependent) + internal void UnsafeSetTargetToNull() { Ephemeron[]? data = _data; @@ -116,10 +130,10 @@ internal void UnsafeSetDependent(object? dependent) return; } - data[0].Value = dependent; + data[0].Key = null; } - internal void UnsafeStopTracking() + internal void UnsafeSetDependent(object? dependent) { Ephemeron[]? data = _data; @@ -130,7 +144,7 @@ internal void UnsafeStopTracking() return; } - data[0].Key = null; + data[0].Value = dependent; } public void Dispose()