From 57fe91c2b48bdfd2379149c123c64abeac8cb66a Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 15 Jun 2021 23:00:33 +0200 Subject: [PATCH 01/30] Move DependentHandle to System.Runtime --- .../System.Private.CoreLib/System.Private.CoreLib.csproj | 2 +- .../System/Runtime/{CompilerServices => }/DependentHandle.cs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) rename src/coreclr/System.Private.CoreLib/src/System/Runtime/{CompilerServices => }/DependentHandle.cs (98%) diff --git a/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj index 626f0eec24b22e..69720e11cb0f9c 100644 --- a/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj +++ b/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj @@ -205,13 +205,13 @@ - + diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/DependentHandle.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs similarity index 98% rename from src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/DependentHandle.cs rename to src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs index d0aff34f2bc7bf..9e9ab7eb7df9e3 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -1,7 +1,9 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -namespace System.Runtime.CompilerServices +using System.Runtime.CompilerServices; + +namespace System.Runtime { // ========================================================================================= // This struct collects all operations on native DependentHandles. The DependentHandle From b1f54b523bee43322631566e185c91a05df1bc31 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 15 Jun 2021 23:30:33 +0200 Subject: [PATCH 02/30] Update DependentHandle APIs to follow review --- .../src/System/Runtime/DependentHandle.cs | 33 +++++++++++-------- .../CompilerServices/ConditionalWeakTable.cs | 23 ++++++++----- 2 files changed, 34 insertions(+), 22 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 9e9ab7eb7df9e3..f74c8ef56df585 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -29,33 +29,40 @@ namespace System.Runtime // This struct intentionally does no self-synchronization. It's up to the caller to // to use DependentHandles in a thread-safe way. // ========================================================================================= - internal struct DependentHandle + internal struct DependentHandle : IDisposable { private IntPtr _handle; - public DependentHandle(object primary, object? secondary) => + public DependentHandle(object primary, object? secondary) + { // no need to check for null result: nInitialize expected to throw OOM. _handle = nInitialize(primary, secondary); + } public bool IsAllocated => _handle != IntPtr.Zero; - // Getting the secondary object is more expensive than getting the first so - // we provide a separate primary-only accessor for those times we only want the - // primary. - public object? GetPrimary() => nGetPrimary(_handle); + public object? Target + { + get => nGetPrimary(_handle); + set => nSetPrimary(_handle, value); + } - public object? GetPrimaryAndSecondary(out object? secondary) => - nGetPrimaryAndSecondary(_handle, out secondary); + public object? Dependent + { + get => GetTargetAndDependent().Dependent; + set => nSetSecondary(_handle, value); + } - public void SetPrimary(object? primary) => - nSetPrimary(_handle, primary); + public (object? Target, object? Dependent) GetTargetAndDependent() + { + object? target = nGetPrimaryAndSecondary(_handle, out object? secondary); - public void SetSecondary(object? secondary) => - nSetSecondary(_handle, secondary); + return (target, secondary); + } // Forces dependentHandle back to non-allocated state (if not already there) // and frees the handle if needed. - public void Free() + public void Dispose() { if (_handle != IntPtr.Zero) { 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 b3c72604977b68..2e3694f91076a6 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 @@ -523,10 +523,15 @@ internal int FindEntry(TKey key, out object? value) int bucket = hashCode & (_buckets.Length - 1); for (int entriesIndex = Volatile.Read(ref _buckets[bucket]); entriesIndex != -1; entriesIndex = _entries[entriesIndex].Next) { - if (_entries[entriesIndex].HashCode == hashCode && _entries[entriesIndex].depHnd.GetPrimaryAndSecondary(out value) == key) + if (_entries[entriesIndex].HashCode == hashCode) { - GC.KeepAlive(this); // ensure we don't get finalized while accessing DependentHandles. - return entriesIndex; + (object? primary, value) = _entries[entriesIndex].depHnd.GetTargetAndDependent(); + + if (primary == key) + { + GC.KeepAlive(this); // ensure we don't get finalized while accessing DependentHandles. + return entriesIndex; + } } } @@ -540,7 +545,7 @@ internal bool TryGetEntry(int index, [NotNullWhen(true)] out TKey? key, [MaybeNu { if (index < _entries.Length) { - object? oKey = _entries[index].depHnd.GetPrimaryAndSecondary(out object? oValue); + (object? oKey, object? oValue) = _entries[index].depHnd.GetTargetAndDependent(); GC.KeepAlive(this); // ensure we don't get finalized while accessing DependentHandles. if (oKey != null) @@ -592,7 +597,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.SetPrimary(null); + entry.depHnd.Target = null; } internal void UpdateValue(int entryIndex, TValue newValue) @@ -602,7 +607,7 @@ internal void UpdateValue(int entryIndex, TValue newValue) VerifyIntegrity(); _invalid = true; - _entries[entryIndex].depHnd.SetSecondary(newValue); + _entries[entryIndex].depHnd.Dependent = newValue; _invalid = false; } @@ -634,7 +639,7 @@ internal Container Resize() break; } - if (entry.depHnd.IsAllocated && entry.depHnd.GetPrimary() is null) + if (entry.depHnd.IsAllocated && entry.depHnd.Target is null) { // the entry has expired hasExpiredEntries = true; @@ -699,7 +704,7 @@ internal Container Resize(int newSize) DependentHandle depHnd = oldEntry.depHnd; if (hashCode != -1 && depHnd.IsAllocated) { - if (depHnd.GetPrimary() != null) + if (depHnd.Target is not null) { ref Entry newEntry = ref newEntries[newEntriesIndex]; @@ -795,7 +800,7 @@ private void VerifyIntegrity() // another container, removed entries are not, therefore this container must free them. if (_oldKeepAlive is null || entries[entriesIndex].HashCode == -1) { - entries[entriesIndex].depHnd.Free(); + entries[entriesIndex].depHnd.Dispose(); } } } From b331b8f8877478a91cd09a27dde1f56392ee3497 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 00:04:37 +0200 Subject: [PATCH 03/30] Make DependentHandle type public --- .../src/System/Runtime/DependentHandle.cs | 97 +++++++++++++------ .../System.Runtime/ref/System.Runtime.cs | 11 +++ 2 files changed, 78 insertions(+), 30 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 f74c8ef56df585..86652f9639ea3c 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -5,54 +5,89 @@ namespace System.Runtime { - // ========================================================================================= - // This struct collects all operations on native DependentHandles. The DependentHandle - // merely wraps an IntPtr so this struct serves mainly as a "managed typedef." - // - // DependentHandles exist in one of two states: - // - // IsAllocated == false - // No actual handle is allocated underneath. Illegal to call GetPrimary - // or GetPrimaryAndSecondary(). Ok to call Free(). - // - // Initializing a DependentHandle using the nullary ctor creates a DependentHandle - // that's in the !IsAllocated state. - // (! Right now, we get this guarantee for free because (IntPtr)0 == NULL unmanaged handle. - // ! If that assertion ever becomes false, we'll have to add an _isAllocated field - // ! to compensate.) - // - // - // IsAllocated == true - // There's a handle allocated underneath. You must call Free() on this eventually - // or you cause a native handle table leak. - // - // This struct intentionally does no self-synchronization. It's up to the caller to - // to use DependentHandles in a thread-safe way. - // ========================================================================================= - internal struct DependentHandle : IDisposable + /// + /// Represents a dependent GC handle, which will conditionally keep a dependent object instance alive + /// as long as a target object instance is alive as well, without representing a strong reference to the + /// target object instance. That is, a value with a given object instance as + /// target will not cause the target to be kept alive if there are no other strong references to it, but + /// it will do so for the dependent object instance as long as the target is alive. + /// + /// This type is conceptually equivalent to having a weak reference to a given target object instance A, with + /// that object having a field or property (or some other strong reference) to a dependent object instance B. + /// + /// + public struct DependentHandle : IDisposable { + // ========================================================================================= + // This struct collects all operations on native DependentHandles. The DependentHandle + // merely wraps an IntPtr so this struct serves mainly as a "managed typedef." + // + // DependentHandles exist in one of two states: + // + // IsAllocated == false + // No actual handle is allocated underneath. Illegal to get Target, Dependent + // or GetTargetAndDependent(). Ok to call Dispose(). + // + // Initializing a DependentHandle using the nullary ctor creates a DependentHandle + // that's in the !IsAllocated state. + // (! Right now, we get this guarantee for free because (IntPtr)0 == NULL unmanaged handle. + // ! If that assertion ever becomes false, we'll have to add an _isAllocated field + // ! to compensate.) + // + // + // IsAllocated == true + // There's a handle allocated underneath. You must call Dispose() on this eventually + // or you cause a native handle table leak. + // + // This struct intentionally does no self-synchronization. It's up to the caller to + // to use DependentHandles in a thread-safe way. + // ========================================================================================= + private IntPtr _handle; - public DependentHandle(object primary, object? secondary) + /// + /// Initializes a new instance of the struct with the specified arguments. + /// + /// The target object instance to track. + /// The dependent object instance to associate with . + public DependentHandle(object? target, object? dependent) { // no need to check for null result: nInitialize expected to throw OOM. - _handle = nInitialize(primary, secondary); + _handle = nInitialize(target, dependent); } + /// + /// Gets a value indicating whether this handle has been allocated or not. + /// public bool IsAllocated => _handle != IntPtr.Zero; + /// + /// Gets or sets the target object instance for the current handle. + /// public object? Target { get => nGetPrimary(_handle); set => nSetPrimary(_handle, value); } + /// + /// Gets or sets the dependent object instance for the current handle. + /// + /// + /// If it is necessary to retrieve both and , it is + /// recommended to use instead. This will result in better + /// performance and it will reduce the chance of unexpected behavior in some cases. + /// public object? Dependent { get => GetTargetAndDependent().Dependent; set => nSetSecondary(_handle, value); } + /// + /// Retrieves the values of both and , if available. + /// + /// The values of and . public (object? Target, object? Dependent) GetTargetAndDependent() { object? target = nGetPrimaryAndSecondary(_handle, out object? secondary); @@ -60,10 +95,12 @@ public object? Dependent return (target, secondary); } - // Forces dependentHandle back to non-allocated state (if not already there) - // and frees the handle if needed. + /// public void Dispose() { + // Forces dependentHandle back to non-allocated state (if not already there) + // and frees the handle if needed. + if (_handle != IntPtr.Zero) { IntPtr handle = _handle; @@ -73,7 +110,7 @@ public void Dispose() } [MethodImpl(MethodImplOptions.InternalCall)] - private static extern IntPtr nInitialize(object primary, object? secondary); + private static extern IntPtr nInitialize(object? primary, object? secondary); [MethodImpl(MethodImplOptions.InternalCall)] private static extern object? nGetPrimary(IntPtr dependentHandle); diff --git a/src/libraries/System.Runtime/ref/System.Runtime.cs b/src/libraries/System.Runtime/ref/System.Runtime.cs index 39a1cfc4b816c3..a26394fecac08c 100644 --- a/src/libraries/System.Runtime/ref/System.Runtime.cs +++ b/src/libraries/System.Runtime/ref/System.Runtime.cs @@ -9593,6 +9593,17 @@ public sealed partial class AssemblyTargetedPatchBandAttribute : System.Attribut public AssemblyTargetedPatchBandAttribute(string targetedPatchBand) { } public string TargetedPatchBand { get { throw null; } } } + public partial struct DependentHandle : System.IDisposable + { + private object _dummy; + private int _dummyPrimitive; + 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; } set { } } + public void Dispose() { } + public (object? Target, object? Dependent) GetTargetAndDependent() { throw null; } + } public enum GCLargeObjectHeapCompactionMode { Default = 1, From a96fa3fb19ae72411b6ab34bcfb1721f7ccbc6b3 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 00:33:45 +0200 Subject: [PATCH 04/30] Update DependentHandle on Mono runtime --- .../System.Private.CoreLib.csproj | 2 +- .../{CompilerServices => }/DependentHandle.cs | 67 +++++++++++++------ 2 files changed, 48 insertions(+), 21 deletions(-) rename src/mono/System.Private.CoreLib/src/System/Runtime/{CompilerServices => }/DependentHandle.cs (50%) diff --git a/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj index 61ad598697e045..82ef9aeecea9c3 100644 --- a/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj +++ b/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj @@ -240,8 +240,8 @@ + - diff --git a/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/DependentHandle.cs b/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs similarity index 50% rename from src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/DependentHandle.cs rename to src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs index 1b976a5906afad..730295d8aa5103 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/DependentHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Runtime.CompilerServices; + namespace System.Runtime.CompilerServices { internal struct Ephemeron @@ -8,66 +10,91 @@ internal struct Ephemeron public object? key; public object? value; } +} +namespace System.Runtime +{ // // Instead of dependent handles, mono uses arrays of Ephemeron objects. // - internal struct DependentHandle + public struct DependentHandle : IDisposable { private Ephemeron[] data; - public DependentHandle(object? primary, object? secondary) + public DependentHandle(object? target, object? dependent) { data = new Ephemeron[1]; - data[0].key = primary; - data[0].value = secondary; + data[0].key = target; + data[0].value = dependent; GC.register_ephemeron_array(data); } public bool IsAllocated => data != null; - // Getting the secondary object is more expensive than getting the first so - // we provide a separate primary-only accessor for those times we only want the - // primary. - public object? GetPrimary() + public object? Target + { + get => GetTarget(); + set => SetTarget(value); + } + + public object? Dependent + { + get => GetDependent(); + set => SetDependent(value); + } + + public (object? Target, object? Dependent) GetTargetAndDependent() { if (!IsAllocated) throw new NotSupportedException(); if (data[0].key == GC.EPHEMERON_TOMBSTONE) - return null; - return data[0].key; + { + return default; + } + return (data[0].key, data[0].value); } - public object? GetPrimaryAndSecondary(out object? secondary) + public void Dispose() { + data = null!; + } + + private object? GetTarget() + { + // Getting the secondary object is more expensive than getting the first so + // we provide a separate primary-only accessor for those times we only want the + // primary. + if (!IsAllocated) throw new NotSupportedException(); if (data[0].key == GC.EPHEMERON_TOMBSTONE) - { - secondary = null; return null; - } - secondary = data[0].value; return data[0].key; } - public void SetPrimary(object? primary) + private void SetTarget(object? primary) { if (!IsAllocated) throw new NotSupportedException(); data[0].key = primary; } - public void SetSecondary(object? secondary) + private object? GetDependent() { if (!IsAllocated) throw new NotSupportedException(); - data[0].value = secondary; + if (data[0].key == GC.EPHEMERON_TOMBSTONE) + { + return null; + } + return data[0].value; } - public void Free() + private void SetDependent(object? secondary) { - data = null!; + if (!IsAllocated) + throw new NotSupportedException(); + data[0].value = secondary; } } } From 16fdf0e9ae9d0bad2d05ab21554ef28adda04b5f Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 01:41:16 +0200 Subject: [PATCH 05/30] Add allocation checks to DependentHandle APIs This avoids throwing ExecutionEngineException-s if one of the public APIs is called on a non-allocated DependentHandle instance --- .../src/System/Runtime/DependentHandle.cs | 62 +++++++++++++++++-- .../src/System/Runtime/DependentHandle.cs | 52 ++++++++++++---- 2 files changed, 98 insertions(+), 16 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 86652f9639ea3c..6f0abcb32e503c 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -64,10 +64,31 @@ public DependentHandle(object? target, object? dependent) /// /// Gets or sets the target object instance for the current handle. /// + /// Thrown if is . public object? Target { - get => nGetPrimary(_handle); - set => nSetPrimary(_handle, value); + get + { + IntPtr handle = _handle; + + if (handle == IntPtr.Zero) + { + ThrowHelper.ThrowInvalidOperationException(); + } + + return nGetPrimary(handle); + } + set + { + IntPtr handle = _handle; + + if (handle == IntPtr.Zero) + { + ThrowHelper.ThrowInvalidOperationException(); + } + + nSetPrimary(handle, value); + } } /// @@ -78,19 +99,50 @@ public object? Target /// recommended to use instead. This will result in better /// performance and it will reduce the chance of unexpected behavior in some cases. /// + /// Thrown if is . public object? Dependent { - get => GetTargetAndDependent().Dependent; - set => nSetSecondary(_handle, value); + get + { + IntPtr handle = _handle; + + if (handle == IntPtr.Zero) + { + ThrowHelper.ThrowInvalidOperationException(); + } + + _ = nGetPrimaryAndSecondary(handle, out object? dependent); + + return dependent; + } + set + { + IntPtr handle = _handle; + + if (handle == IntPtr.Zero) + { + ThrowHelper.ThrowInvalidOperationException(); + } + + nSetSecondary(handle, value); + } } /// /// Retrieves the values of both and , if available. /// /// The values of and . + /// Thrown if is . public (object? Target, object? Dependent) GetTargetAndDependent() { - object? target = nGetPrimaryAndSecondary(_handle, out object? secondary); + IntPtr handle = _handle; + + if (handle == IntPtr.Zero) + { + ThrowHelper.ThrowInvalidOperationException(); + } + + object? target = nGetPrimaryAndSecondary(handle, out object? secondary); return (target, secondary); } 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 730295d8aa5103..fef01ae87035f1 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -29,7 +29,7 @@ public DependentHandle(object? target, object? dependent) GC.register_ephemeron_array(data); } - public bool IsAllocated => data != null; + public bool IsAllocated => data is not null; public object? Target { @@ -45,12 +45,18 @@ public object? Dependent public (object? Target, object? Dependent) GetTargetAndDependent() { - if (!IsAllocated) - throw new NotSupportedException(); + if (this.data is not Ephemeron[] data) + { + ThrowHelper.ThrowInvalidOperationException(); + + return default; + } + if (data[0].key == GC.EPHEMERON_TOMBSTONE) { return default; } + return (data[0].key, data[0].value); } @@ -65,35 +71,59 @@ public void Dispose() // we provide a separate primary-only accessor for those times we only want the // primary. - if (!IsAllocated) - throw new NotSupportedException(); + if (this.data is not Ephemeron[] data) + { + ThrowHelper.ThrowInvalidOperationException(); + + return default; + } + if (data[0].key == GC.EPHEMERON_TOMBSTONE) + { return null; + } + return data[0].key; } private void SetTarget(object? primary) { - if (!IsAllocated) - throw new NotSupportedException(); + if (this.data is not Ephemeron[] data) + { + ThrowHelper.ThrowInvalidOperationException(); + + return; + } + data[0].key = primary; } private object? GetDependent() { - if (!IsAllocated) - throw new NotSupportedException(); + if (this.data is not Ephemeron[] data) + { + ThrowHelper.ThrowInvalidOperationException(); + + return default; + } + if (data[0].key == GC.EPHEMERON_TOMBSTONE) { return null; } + return data[0].value; } private void SetDependent(object? secondary) { - if (!IsAllocated) - throw new NotSupportedException(); + if (this.data is not Ephemeron[] data) + { + ThrowHelper.ThrowInvalidOperationException(); + + return; + } + data[0].value = secondary; } } From 748d88e6378e6c7fab173bd6b95d9cfd471f15cc Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 02:01:07 +0200 Subject: [PATCH 06/30] Add more unit tests for new public DependentHandle APIs --- .../tests/System.Runtime.Tests.csproj | 1 + .../System/Runtime/DependentHandleTests.cs | 73 +++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs diff --git a/src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj b/src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj index 2a68cc12651e18..65976718c079b9 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj +++ b/src/libraries/System.Runtime/tests/System.Runtime.Tests.csproj @@ -215,6 +215,7 @@ + diff --git a/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs b/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs new file mode 100644 index 00000000000000..a5171d0d7221c1 --- /dev/null +++ b/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs @@ -0,0 +1,73 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Xunit; + +namespace System.Runtime.Tests +{ + // NOTE: DependentHandle is already heavily tested indirectly through ConditionalWeakTable<,>. + // This class contains some specific tests for APIs that are only relevant when used directly. + public class DependentHandleTests + { + [Fact] + public void GetTarget_ThrowsInvalidOperationException() + { + Assert.Throws(() => default(DependentHandle).Target); + } + + [Fact] + public void SetTarget_ThrowsInvalidOperationException() + { + Assert.Throws(() => + { + DependentHandle handle = default; + handle.Target = new(); + }); + } + + [Fact] + public void GetDependent_ThrowsInvalidOperationException() + { + Assert.Throws(() => default(DependentHandle).Dependent); + } + + [Fact] + public void SetDependent_ThrowsInvalidOperationException() + { + Assert.Throws(() => + { + DependentHandle handle = default; + handle.Dependent = new(); + }); + } + + [Fact] + public void GetTargetAndDependent_ThrowsInvalidOperationException() + { + Assert.Throws(() => default(DependentHandle).GetTargetAndDependent()); + } + + [Fact] + public void Dispose_RepeatedCallsAreFine() + { + object key = new(), value = new(); + DependentHandle handle = new(key, value); + + Assert.True(handle.IsAllocated); + + handle.Dispose(); + + Assert.False(handle.IsAllocated); + + handle.Dispose(); + + Assert.False(handle.IsAllocated); + + handle.Dispose(); + handle.Dispose(); + handle.Dispose(); + + Assert.False(handle.IsAllocated); + } + } +} From 247fa5a5eb80e72dd00e26d02d069b24e3514976 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 02:46:16 +0200 Subject: [PATCH 07/30] Add faster, unsafe internal APIs versions to DependentHandle --- .../src/System/Runtime/DependentHandle.cs | 42 +++++++++++++++++ .../CompilerServices/ConditionalWeakTable.cs | 21 ++++----- .../src/System/Runtime/DependentHandle.cs | 46 +++++++++++++++++-- 3 files changed, 92 insertions(+), 17 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 6f0abcb32e503c..679f270072b2ab 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -147,6 +147,48 @@ public object? Dependent return (target, secondary); } + /// + /// Gets the target object instance for the current handle. + /// + /// The target object instance, if present. + /// This method mirrors , but without the allocation check. + internal object? UnsafeGetTarget() + { + return nGetPrimary(_handle); + } + + /// + /// Sets the target object instance for the current handle. + /// + /// This method mirrors , but without the allocation check. + internal void UnsafeSetTarget(object? target) + { + nSetPrimary(_handle, target); + } + + /// + /// Sets the dependent object instance for the current handle. + /// + /// This method mirrors , but without the allocation check. + internal void UnsafeSetDependent(object? dependent) + { + nSetSecondary(_handle, dependent); + } + + /// + /// Retrieves the values of both and , if available. + /// + /// The dependent instance, if available. + /// The values of and . + /// + /// This method mirrors , but without the allocation check. + /// The signature is also kept the same as the one for the internal call, to improve the codegen. + /// + internal object? UnsafeGetTargetAndDependent(out object? dependent) + { + return nGetPrimaryAndSecondary(_handle, out dependent); + } + /// public void Dispose() { 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 2e3694f91076a6..67634bce43a8d6 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 @@ -523,15 +523,10 @@ internal int FindEntry(TKey key, out object? value) int bucket = hashCode & (_buckets.Length - 1); for (int entriesIndex = Volatile.Read(ref _buckets[bucket]); entriesIndex != -1; entriesIndex = _entries[entriesIndex].Next) { - if (_entries[entriesIndex].HashCode == hashCode) + if (_entries[entriesIndex].HashCode == hashCode && _entries[entriesIndex].depHnd.UnsafeGetTargetAndDependent(out value) == key) { - (object? primary, value) = _entries[entriesIndex].depHnd.GetTargetAndDependent(); - - if (primary == key) - { - GC.KeepAlive(this); // ensure we don't get finalized while accessing DependentHandles. - return entriesIndex; - } + GC.KeepAlive(this); // ensure we don't get finalized while accessing DependentHandles. + return entriesIndex; } } @@ -545,7 +540,7 @@ internal bool TryGetEntry(int index, [NotNullWhen(true)] out TKey? key, [MaybeNu { if (index < _entries.Length) { - (object? oKey, object? oValue) = _entries[index].depHnd.GetTargetAndDependent(); + object? oKey = _entries[index].depHnd.UnsafeGetTargetAndDependent(out object? oValue); GC.KeepAlive(this); // ensure we don't get finalized while accessing DependentHandles. if (oKey != null) @@ -597,7 +592,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.Target = null; + entry.depHnd.UnsafeSetTarget(null); } internal void UpdateValue(int entryIndex, TValue newValue) @@ -607,7 +602,7 @@ internal void UpdateValue(int entryIndex, TValue newValue) VerifyIntegrity(); _invalid = true; - _entries[entryIndex].depHnd.Dependent = newValue; + _entries[entryIndex].depHnd.UnsafeSetDependent(newValue); _invalid = false; } @@ -639,7 +634,7 @@ internal Container Resize() break; } - if (entry.depHnd.IsAllocated && entry.depHnd.Target is null) + if (entry.depHnd.IsAllocated && entry.depHnd.UnsafeGetTarget() is null) { // the entry has expired hasExpiredEntries = true; @@ -704,7 +699,7 @@ internal Container Resize(int newSize) DependentHandle depHnd = oldEntry.depHnd; if (hashCode != -1 && depHnd.IsAllocated) { - if (depHnd.Target is not null) + if (depHnd.UnsafeGetTarget() is not null) { ref Entry newEntry = ref newEntries[newEntriesIndex]; 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 fef01ae87035f1..f0aa8713da28ff 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -60,6 +60,44 @@ public object? Dependent return (data[0].key, data[0].value); } + internal object? UnsafeGetTarget() + { + return GetTarget(); + } + + internal void UnsafeSetTarget(object? target) + { + SetTarget(target); + } + + internal void UnsafeSetDependent(object? dependent) + { + SetDependent(dependent); + } + + internal object? UnsafeGetTargetAndDependent(out object? dependent) + { + if (this.data is not Ephemeron[] data) + { + ThrowHelper.ThrowInvalidOperationException(); + + dependent = null; + + return null; + } + + if (data[0].key == GC.EPHEMERON_TOMBSTONE) + { + dependent = null; + + return null; + } + + dependent = data[0].value; + + return data[0].key; + } + public void Dispose() { data = null!; @@ -86,7 +124,7 @@ public void Dispose() return data[0].key; } - private void SetTarget(object? primary) + private void SetTarget(object? target) { if (this.data is not Ephemeron[] data) { @@ -95,7 +133,7 @@ private void SetTarget(object? primary) return; } - data[0].key = primary; + data[0].key = target; } private object? GetDependent() @@ -115,7 +153,7 @@ private void SetTarget(object? primary) return data[0].value; } - private void SetDependent(object? secondary) + private void SetDependent(object? dependent) { if (this.data is not Ephemeron[] data) { @@ -124,7 +162,7 @@ private void SetDependent(object? secondary) return; } - data[0].value = secondary; + data[0].value = dependent; } } } From 66d2ac518d91db11501f0c1a1b1b540c50ce9ec4 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 17:31:07 +0200 Subject: [PATCH 08/30] Naming improvements to Ephemeron type The ephemeron type is checked in the Mono runtime in "object.c" as follows: m_class_get_image (klass) == mono_defaults.corlib && !strcmp ("Ephemeron", m_class_get_name (klass)) As such, the namespace it belongs to in the managed runtime doesn't matter: the VM will just check that the type name matches, and that the type is in fact defined in corelib. This means we can just move it to System.Runtime without worrying about it being properly managed in the VM. Additionally, the type is defined in "sgen-mono.c" as follows: typedef struct { GCObject* key; GCObject* value; } Ephemeron; So as long as the layout matches the one of the type defined in C# (which it does), we're also free to rename the fields to better follow the naming guidelines, and the VM will have no issues with it. --- .../src/System/GC.Mono.cs | 1 + .../src/System/Runtime/DependentHandle.cs | 37 ++++++++----------- 2 files changed, 17 insertions(+), 21 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/GC.Mono.cs b/src/mono/System.Private.CoreLib/src/System/GC.Mono.cs index ee874822c563c1..a92ebab49920d7 100644 --- a/src/mono/System.Private.CoreLib/src/System/GC.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/GC.Mono.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Runtime; using System.Runtime.CompilerServices; using Internal.Runtime.CompilerServices; using System.Diagnostics.Tracing; 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 f0aa8713da28ff..0290fd199b226b 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -1,19 +1,14 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Runtime.CompilerServices; - -namespace System.Runtime.CompilerServices +namespace System.Runtime { internal struct Ephemeron { - public object? key; - public object? value; + public object? Key; + public object? Value; } -} -namespace System.Runtime -{ // // Instead of dependent handles, mono uses arrays of Ephemeron objects. // @@ -24,8 +19,8 @@ public struct DependentHandle : IDisposable public DependentHandle(object? target, object? dependent) { data = new Ephemeron[1]; - data[0].key = target; - data[0].value = dependent; + data[0].Key = target; + data[0].Value = dependent; GC.register_ephemeron_array(data); } @@ -52,12 +47,12 @@ public object? Dependent return default; } - if (data[0].key == GC.EPHEMERON_TOMBSTONE) + if (data[0].Key == GC.EPHEMERON_TOMBSTONE) { return default; } - return (data[0].key, data[0].value); + return (data[0].Key, data[0].Value); } internal object? UnsafeGetTarget() @@ -86,16 +81,16 @@ internal void UnsafeSetDependent(object? dependent) return null; } - if (data[0].key == GC.EPHEMERON_TOMBSTONE) + if (data[0].Key == GC.EPHEMERON_TOMBSTONE) { dependent = null; return null; } - dependent = data[0].value; + dependent = data[0].Value; - return data[0].key; + return data[0].Key; } public void Dispose() @@ -116,12 +111,12 @@ public void Dispose() return default; } - if (data[0].key == GC.EPHEMERON_TOMBSTONE) + if (data[0].Key == GC.EPHEMERON_TOMBSTONE) { return null; } - return data[0].key; + return data[0].Key; } private void SetTarget(object? target) @@ -133,7 +128,7 @@ private void SetTarget(object? target) return; } - data[0].key = target; + data[0].Key = target; } private object? GetDependent() @@ -145,12 +140,12 @@ private void SetTarget(object? target) return default; } - if (data[0].key == GC.EPHEMERON_TOMBSTONE) + if (data[0].Key == GC.EPHEMERON_TOMBSTONE) { return null; } - return data[0].value; + return data[0].Value; } private void SetDependent(object? dependent) @@ -162,7 +157,7 @@ private void SetDependent(object? dependent) return; } - data[0].value = dependent; + data[0].Value = dependent; } } } From 4067ac3fe4668ff7c6557f03ff952f243d960feb Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 17:32:21 +0200 Subject: [PATCH 09/30] Code style tweaks, improved nullability annotations --- .../src/System/Runtime/DependentHandle.cs | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) 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 0290fd199b226b..6601cf860b40ec 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -14,17 +14,17 @@ internal struct Ephemeron // public struct DependentHandle : IDisposable { - private Ephemeron[] data; + private Ephemeron[]? _data; public DependentHandle(object? target, object? dependent) { - data = new Ephemeron[1]; - data[0].Key = target; - data[0].Value = dependent; - GC.register_ephemeron_array(data); + _data = new Ephemeron[1]; + _data[0].Key = target; + _data[0].Value = dependent; + GC.register_ephemeron_array(_data); } - public bool IsAllocated => data is not null; + public bool IsAllocated => _data is not null; public object? Target { @@ -40,7 +40,7 @@ public object? Dependent public (object? Target, object? Dependent) GetTargetAndDependent() { - if (this.data is not Ephemeron[] data) + if (_data is not Ephemeron[] data) { ThrowHelper.ThrowInvalidOperationException(); @@ -72,7 +72,7 @@ internal void UnsafeSetDependent(object? dependent) internal object? UnsafeGetTargetAndDependent(out object? dependent) { - if (this.data is not Ephemeron[] data) + if (_data is not Ephemeron[] data) { ThrowHelper.ThrowInvalidOperationException(); @@ -95,7 +95,7 @@ internal void UnsafeSetDependent(object? dependent) public void Dispose() { - data = null!; + _data = null; } private object? GetTarget() @@ -104,7 +104,7 @@ public void Dispose() // we provide a separate primary-only accessor for those times we only want the // primary. - if (this.data is not Ephemeron[] data) + if (_data is not Ephemeron[] data) { ThrowHelper.ThrowInvalidOperationException(); @@ -121,7 +121,7 @@ public void Dispose() private void SetTarget(object? target) { - if (this.data is not Ephemeron[] data) + if (_data is not Ephemeron[] data) { ThrowHelper.ThrowInvalidOperationException(); @@ -133,7 +133,7 @@ private void SetTarget(object? target) private object? GetDependent() { - if (this.data is not Ephemeron[] data) + if (_data is not Ephemeron[] data) { ThrowHelper.ThrowInvalidOperationException(); @@ -150,7 +150,7 @@ private void SetTarget(object? target) private void SetDependent(object? dependent) { - if (this.data is not Ephemeron[] data) + if (_data is not Ephemeron[] data) { ThrowHelper.ThrowInvalidOperationException(); From 167033970d14f36197f28166f0c88e91494dd7b4 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 17:50:27 +0200 Subject: [PATCH 10/30] Remove incorrect DependentHandle comment on Mono --- .../src/System/Runtime/DependentHandle.cs | 4 ---- 1 file changed, 4 deletions(-) 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 6601cf860b40ec..e887c9f7b58043 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -100,10 +100,6 @@ public void Dispose() private object? GetTarget() { - // Getting the secondary object is more expensive than getting the first so - // we provide a separate primary-only accessor for those times we only want the - // primary. - if (_data is not Ephemeron[] data) { ThrowHelper.ThrowInvalidOperationException(); From 96cfc911ebb13680ab5473be4915f664ff0e4af6 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 18:00:07 +0200 Subject: [PATCH 11/30] Add default Dispose test for DependentHandle Co-authored-by: Stephen Toub --- .../tests/System/Runtime/DependentHandleTests.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs b/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs index a5171d0d7221c1..dbfe64093518e6 100644 --- a/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs +++ b/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs @@ -69,5 +69,13 @@ public void Dispose_RepeatedCallsAreFine() Assert.False(handle.IsAllocated); } + + [Fact] + public void Dispose_ValidOnDefault() + { + DependentHandle handle = default; + Assert.False(handle.IsAllocated); + handle.Dispose(); + } } } From 6a8db56e93333b288b3512909458621f295bd04b Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 18:40:25 +0200 Subject: [PATCH 12/30] Fix race condition in DependentHandle on Mono --- .../src/System/Runtime/DependentHandle.cs | 35 ++++++++----------- 1 file changed, 15 insertions(+), 20 deletions(-) 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 e887c9f7b58043..7f7d3fa664cd36 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -47,12 +47,9 @@ public object? Dependent return default; } - if (data[0].Key == GC.EPHEMERON_TOMBSTONE) - { - return default; - } + Ephemeron e = data[0]; - return (data[0].Key, data[0].Value); + return e.Key != GC.EPHEMERON_TOMBSTONE ? (e.Key, e.Value) : default; } internal object? UnsafeGetTarget() @@ -81,16 +78,20 @@ internal void UnsafeSetDependent(object? dependent) return null; } - if (data[0].Key == GC.EPHEMERON_TOMBSTONE) + Ephemeron e = data[0]; + + if (e.Key != GC.EPHEMERON_TOMBSTONE) + { + dependent = e.Value; + + return e.Key; + } + else { dependent = null; return null; } - - dependent = data[0].Value; - - return data[0].Key; } public void Dispose() @@ -107,12 +108,9 @@ public void Dispose() return default; } - if (data[0].Key == GC.EPHEMERON_TOMBSTONE) - { - return null; - } + object? key = data[0].Key; - return data[0].Key; + return key != GC.EPHEMERON_TOMBSTONE ? key : null; } private void SetTarget(object? target) @@ -136,12 +134,9 @@ private void SetTarget(object? target) return default; } - if (data[0].Key == GC.EPHEMERON_TOMBSTONE) - { - return null; - } + Ephemeron e = data[0]; - return data[0].Value; + return e.Key != GC.EPHEMERON_TOMBSTONE ? e.Value : null; } private void SetDependent(object? dependent) From 1601d889b1dd9e8d13e7440b2f1360f1a9aa1dbc Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 20:16:59 +0200 Subject: [PATCH 13/30] Optimize DependentHandle.nGetPrimary on CoreCLR Removed internal call, same optimization as GCHandle --- .../src/System/Runtime/DependentHandle.cs | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) 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 679f270072b2ab..dc5229638692ca 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Runtime.CompilerServices; +using Internal.Runtime.CompilerServices; namespace System.Runtime { @@ -206,8 +207,40 @@ public void Dispose() [MethodImpl(MethodImplOptions.InternalCall)] private static extern IntPtr nInitialize(object? primary, object? secondary); +#if DEBUG [MethodImpl(MethodImplOptions.InternalCall)] private static extern object? nGetPrimary(IntPtr dependentHandle); +#else + private static unsafe object? nGetPrimary(IntPtr dependentHandle) + { + // This optimization is the same that is used in GCHandle in RELEASE mode. + // This is not used in DEBUG builds as the runtime performs additional checks. + // This conversion works because on CoreCLR, DependentHandle is really just a + // OBJECTHANDLE value, the same that GCHandle uses. They are defined as follows: + // + // struct OBJECTHANDLE__ + // { + // void* unused; + // }; + // + // typedef struct OBJECTHANDLE__* OBJECTHANDLE; + // + // The way this works is that the GC will allocate a special region of memory where + // the object reference will be stored (that is, an object* pointer), and the GC will + // take care of updating it whenever a compacting collection occurs and the object is + // moved. The code below is just doing an indirect memory access to read the pointer + // value stored in this memory region, and then converting that to an object reference. + // Note that Unsafe.As returns a ref object and does an implicit read. + // The code below just compiles down to the following asm on x64: + // + // mov rax, [rcx] + // ret + // + // That is, it's reading the object* pointer the handle points to, and returning it. + // The object* pointer is just expressed object in C#, as it's a reference type. + return Unsafe.As(ref *(IntPtr*)dependentHandle); + } +#endif [MethodImpl(MethodImplOptions.InternalCall)] private static extern object? nGetPrimaryAndSecondary(IntPtr dependentHandle, out object? secondary); From 359938be6183b241c5c21af5ad10f53ee1836e2c Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 20:30:51 +0200 Subject: [PATCH 14/30] Small IL codegen improvement in DependentHandle.nGetPrimary --- .../src/System/Runtime/DependentHandle.cs | 4 +++- .../src/System/Runtime/InteropServices/GCHandle.CoreCLR.cs | 2 +- 2 files changed, 4 insertions(+), 2 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 dc5229638692ca..a6108d736833b6 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -231,6 +231,8 @@ public void Dispose() // moved. The code below is just doing an indirect memory access to read the pointer // value stored in this memory region, and then converting that to an object reference. // Note that Unsafe.As returns a ref object and does an implicit read. + // The (nint) cast is needed to avoid emitting a call to IntPtr::op_Explicit(nint), which + // results in the same codegen but has a larger IL and creates more IR nodes at JIT time. // The code below just compiles down to the following asm on x64: // // mov rax, [rcx] @@ -238,7 +240,7 @@ public void Dispose() // // That is, it's reading the object* pointer the handle points to, and returning it. // The object* pointer is just expressed object in C#, as it's a reference type. - return Unsafe.As(ref *(IntPtr*)dependentHandle); + return Unsafe.As(ref *(IntPtr*)(nint)dependentHandle); } #endif diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.CoreCLR.cs index 8cfaff78e938be..fa342d4db706a6 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.CoreCLR.cs @@ -22,7 +22,7 @@ public partial struct GCHandle internal static extern object? InternalGet(IntPtr handle); #else internal static unsafe object? InternalGet(IntPtr handle) => - Unsafe.As(ref *(IntPtr*)handle); + Unsafe.As(ref *(IntPtr*)(nint)handle); #endif [MethodImpl(MethodImplOptions.InternalCall)] From 312851a6fc11ac57357131ab4da4b6ff7733adbe Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 20:49:54 +0200 Subject: [PATCH 15/30] Simplify comments, add #ifdef for using directive --- .../src/System/Runtime/DependentHandle.cs | 28 ++----------------- 1 file changed, 3 insertions(+), 25 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 a6108d736833b6..843be29a1dfe5b 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -2,7 +2,9 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Runtime.CompilerServices; +#if !DEBUG using Internal.Runtime.CompilerServices; +#endif namespace System.Runtime { @@ -215,31 +217,7 @@ public void Dispose() { // This optimization is the same that is used in GCHandle in RELEASE mode. // This is not used in DEBUG builds as the runtime performs additional checks. - // This conversion works because on CoreCLR, DependentHandle is really just a - // OBJECTHANDLE value, the same that GCHandle uses. They are defined as follows: - // - // struct OBJECTHANDLE__ - // { - // void* unused; - // }; - // - // typedef struct OBJECTHANDLE__* OBJECTHANDLE; - // - // The way this works is that the GC will allocate a special region of memory where - // the object reference will be stored (that is, an object* pointer), and the GC will - // take care of updating it whenever a compacting collection occurs and the object is - // moved. The code below is just doing an indirect memory access to read the pointer - // value stored in this memory region, and then converting that to an object reference. - // Note that Unsafe.As returns a ref object and does an implicit read. - // The (nint) cast is needed to avoid emitting a call to IntPtr::op_Explicit(nint), which - // results in the same codegen but has a larger IL and creates more IR nodes at JIT time. - // The code below just compiles down to the following asm on x64: - // - // mov rax, [rcx] - // ret - // - // That is, it's reading the object* pointer the handle points to, and returning it. - // The object* pointer is just expressed object in C#, as it's a reference type. + // The logic below is the inlined copy of ObjectFromHandle in the unmanaged runtime. return Unsafe.As(ref *(IntPtr*)(nint)dependentHandle); } #endif From 0145a769f463b599c0bb21501d2f53b9311e44b2 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 20:53:57 +0200 Subject: [PATCH 16/30] Minor code style tweaks --- .../src/System/Runtime/DependentHandle.cs | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) 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 7f7d3fa664cd36..7be4b35e2b3965 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -40,7 +40,9 @@ public object? Dependent public (object? Target, object? Dependent) GetTargetAndDependent() { - if (_data is not Ephemeron[] data) + Ephemeron[]? data = _data; + + if (data is null) { ThrowHelper.ThrowInvalidOperationException(); @@ -69,7 +71,9 @@ internal void UnsafeSetDependent(object? dependent) internal object? UnsafeGetTargetAndDependent(out object? dependent) { - if (_data is not Ephemeron[] data) + Ephemeron[]? data = _data; + + if (data is null) { ThrowHelper.ThrowInvalidOperationException(); @@ -101,7 +105,9 @@ public void Dispose() private object? GetTarget() { - if (_data is not Ephemeron[] data) + Ephemeron[]? data = _data; + + if (data is null) { ThrowHelper.ThrowInvalidOperationException(); @@ -115,7 +121,9 @@ public void Dispose() private void SetTarget(object? target) { - if (_data is not Ephemeron[] data) + Ephemeron[]? data = _data; + + if (data is null) { ThrowHelper.ThrowInvalidOperationException(); @@ -127,7 +135,9 @@ private void SetTarget(object? target) private object? GetDependent() { - if (_data is not Ephemeron[] data) + Ephemeron[]? data = _data; + + if (data is null) { ThrowHelper.ThrowInvalidOperationException(); @@ -141,7 +151,9 @@ private void SetTarget(object? target) private void SetDependent(object? dependent) { - if (_data is not Ephemeron[] data) + Ephemeron[]? data = _data; + + if (data is null) { ThrowHelper.ThrowInvalidOperationException(); From 4925877a16ac629ad74c33005d2461c7bec91ec3 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 21:04:14 +0200 Subject: [PATCH 17/30] Change nGetPrimaryAndSecondary to nGetSecondary --- .../src/System/Runtime/DependentHandle.cs | 17 +++++++++++------ src/coreclr/vm/comdependenthandle.cpp | 8 +++----- src/coreclr/vm/comdependenthandle.h | 2 +- src/coreclr/vm/ecalllist.h | 12 ++++++------ 4 files changed, 21 insertions(+), 18 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 843be29a1dfe5b..1667692de5e9b5 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -114,9 +114,7 @@ public object? Dependent ThrowHelper.ThrowInvalidOperationException(); } - _ = nGetPrimaryAndSecondary(handle, out object? dependent); - - return dependent; + return nGetSecondary(handle); } set { @@ -145,7 +143,8 @@ public object? Dependent ThrowHelper.ThrowInvalidOperationException(); } - object? target = nGetPrimaryAndSecondary(handle, out object? secondary); + object? target = nGetPrimary(handle); + object? secondary = nGetSecondary(handle); return (target, secondary); } @@ -189,7 +188,13 @@ internal void UnsafeSetDependent(object? dependent) /// internal object? UnsafeGetTargetAndDependent(out object? dependent) { - return nGetPrimaryAndSecondary(_handle, out dependent); + IntPtr handle = _handle; + + object? target = nGetPrimary(handle); + + dependent = nGetSecondary(handle); + + return target; } /// @@ -223,7 +228,7 @@ public void Dispose() #endif [MethodImpl(MethodImplOptions.InternalCall)] - private static extern object? nGetPrimaryAndSecondary(IntPtr dependentHandle, out object? secondary); + private static extern object? nGetSecondary(IntPtr dependentHandle); [MethodImpl(MethodImplOptions.InternalCall)] private static extern void nSetPrimary(IntPtr dependentHandle, object? primary); diff --git a/src/coreclr/vm/comdependenthandle.cpp b/src/coreclr/vm/comdependenthandle.cpp index e261f16064a6c5..412c2551200777 100644 --- a/src/coreclr/vm/comdependenthandle.cpp +++ b/src/coreclr/vm/comdependenthandle.cpp @@ -65,18 +65,16 @@ FCIMPLEND -FCIMPL2(Object*, DependentHandle::nGetPrimaryAndSecondary, OBJECTHANDLE handle, Object **outSecondary) +FCIMPL1(Object*, DependentHandle::nGetSecondary, OBJECTHANDLE handle) { FCALL_CONTRACT; - _ASSERTE(handle != NULL && outSecondary != NULL); + _ASSERTE(handle != NULL); OBJECTREF primary = ObjectFromHandle(handle); IGCHandleManager *mgr = GCHandleUtilities::GetGCHandleManager(); // Secondary is tracked only if primary is non-null - *outSecondary = (primary != NULL) ? mgr->GetDependentHandleSecondary(handle) : NULL; - - return OBJECTREFToObject(primary); + return (primary != NULL) ? mgr->GetDependentHandleSecondary(handle) : NULL; } FCIMPLEND diff --git a/src/coreclr/vm/comdependenthandle.h b/src/coreclr/vm/comdependenthandle.h index 7c80d0bebe4a46..d10de3c869a6eb 100644 --- a/src/coreclr/vm/comdependenthandle.h +++ b/src/coreclr/vm/comdependenthandle.h @@ -42,7 +42,7 @@ class DependentHandle public: static FCDECL2(OBJECTHANDLE, nInitialize, Object *primary, Object *secondary); static FCDECL1(Object *, nGetPrimary, OBJECTHANDLE handle); - static FCDECL2(Object *, nGetPrimaryAndSecondary, OBJECTHANDLE handle, Object **outSecondary); + static FCDECL1(Object *, nGetSecondary, OBJECTHANDLE handle); static FCDECL1(VOID, nFree, OBJECTHANDLE handle); static FCDECL2(VOID, nSetPrimary, OBJECTHANDLE handle, Object *primary); static FCDECL2(VOID, nSetSecondary, OBJECTHANDLE handle, Object *secondary); diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 48a2d7d322e574..f2965c3ff6a1ef 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -61,12 +61,12 @@ FCFuncStart(gDependentHandleFuncs) - FCFuncElement("nInitialize", DependentHandle::nInitialize) - FCFuncElement("nGetPrimary", DependentHandle::nGetPrimary) - FCFuncElement("nGetPrimaryAndSecondary", DependentHandle::nGetPrimaryAndSecondary) - FCFuncElement("nFree", DependentHandle::nFree) - FCFuncElement("nSetPrimary", DependentHandle::nSetPrimary) - FCFuncElement("nSetSecondary", DependentHandle::nSetSecondary) + FCFuncElement("nInitialize", DependentHandle::nInitialize) + FCFuncElement("nGetPrimary", DependentHandle::nGetPrimary) + FCFuncElement("nGetSecondary", DependentHandle::nGetSecondary) + FCFuncElement("nFree", DependentHandle::nFree) + FCFuncElement("nSetPrimary", DependentHandle::nSetPrimary) + FCFuncElement("nSetSecondary", DependentHandle::nSetSecondary) FCFuncEnd() From 1664a9577a7e81d0592f8f75dcdaa6ba60a57ef4 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 21:15:26 +0200 Subject: [PATCH 18/30] Minor code refactoring to DependentHandle on Mono --- .../src/System/Runtime/DependentHandle.cs | 85 ++++++++----------- 1 file changed, 35 insertions(+), 50 deletions(-) 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 7be4b35e2b3965..94074539534fb8 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -28,14 +28,14 @@ public DependentHandle(object? target, object? dependent) public object? Target { - get => GetTarget(); - set => SetTarget(value); + get => UnsafeGetTarget(); + set => UnsafeSetTarget(value); } public object? Dependent { get => GetDependent(); - set => SetDependent(value); + set => UnsafeSetDependent(value); } public (object? Target, object? Dependent) GetTargetAndDependent() @@ -56,17 +56,46 @@ public object? Dependent internal object? UnsafeGetTarget() { - return GetTarget(); + Ephemeron[]? data = _data; + + if (data is null) + { + ThrowHelper.ThrowInvalidOperationException(); + + return default; + } + + object? key = data[0].Key; + + return key != GC.EPHEMERON_TOMBSTONE ? key : null; } internal void UnsafeSetTarget(object? target) { - SetTarget(target); + Ephemeron[]? data = _data; + + if (data is null) + { + ThrowHelper.ThrowInvalidOperationException(); + + return; + } + + data[0].Key = target; } internal void UnsafeSetDependent(object? dependent) { - SetDependent(dependent); + Ephemeron[]? data = _data; + + if (data is null) + { + ThrowHelper.ThrowInvalidOperationException(); + + return; + } + + data[0].Value = dependent; } internal object? UnsafeGetTargetAndDependent(out object? dependent) @@ -103,36 +132,6 @@ public void Dispose() _data = null; } - private object? GetTarget() - { - Ephemeron[]? data = _data; - - if (data is null) - { - ThrowHelper.ThrowInvalidOperationException(); - - return default; - } - - object? key = data[0].Key; - - return key != GC.EPHEMERON_TOMBSTONE ? key : null; - } - - private void SetTarget(object? target) - { - Ephemeron[]? data = _data; - - if (data is null) - { - ThrowHelper.ThrowInvalidOperationException(); - - return; - } - - data[0].Key = target; - } - private object? GetDependent() { Ephemeron[]? data = _data; @@ -148,19 +147,5 @@ private void SetTarget(object? target) return e.Key != GC.EPHEMERON_TOMBSTONE ? e.Value : null; } - - private void SetDependent(object? dependent) - { - Ephemeron[]? data = _data; - - if (data is null) - { - ThrowHelper.ThrowInvalidOperationException(); - - return; - } - - data[0].Value = dependent; - } } } From ca515b6c9c0711a188cd20994e5f00e393059d71 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 23:03:26 +0200 Subject: [PATCH 19/30] Rename DependentHandle FCalls --- .../src/System/Runtime/DependentHandle.cs | 40 ++++++------- src/coreclr/vm/comdependenthandle.cpp | 59 ++++++++----------- src/coreclr/vm/comdependenthandle.h | 27 ++++----- src/coreclr/vm/ecalllist.h | 12 ++-- 4 files changed, 65 insertions(+), 73 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 1667692de5e9b5..319c4d58f50f5a 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -56,7 +56,7 @@ public struct DependentHandle : IDisposable public DependentHandle(object? target, object? dependent) { // no need to check for null result: nInitialize expected to throw OOM. - _handle = nInitialize(target, dependent); + _handle = InternalInitialize(target, dependent); } /// @@ -79,7 +79,7 @@ public object? Target ThrowHelper.ThrowInvalidOperationException(); } - return nGetPrimary(handle); + return InternalGetTarget(handle); } set { @@ -90,7 +90,7 @@ public object? Target ThrowHelper.ThrowInvalidOperationException(); } - nSetPrimary(handle, value); + InternalSetTarget(handle, value); } } @@ -114,7 +114,7 @@ public object? Dependent ThrowHelper.ThrowInvalidOperationException(); } - return nGetSecondary(handle); + return InternalGetDependent(handle); } set { @@ -125,7 +125,7 @@ public object? Dependent ThrowHelper.ThrowInvalidOperationException(); } - nSetSecondary(handle, value); + InternalSetDependent(handle, value); } } @@ -143,8 +143,8 @@ public object? Dependent ThrowHelper.ThrowInvalidOperationException(); } - object? target = nGetPrimary(handle); - object? secondary = nGetSecondary(handle); + object? target = InternalGetTarget(handle); + object? secondary = InternalGetDependent(handle); return (target, secondary); } @@ -156,7 +156,7 @@ public object? Dependent /// This method mirrors , but without the allocation check. internal object? UnsafeGetTarget() { - return nGetPrimary(_handle); + return InternalGetTarget(_handle); } /// @@ -165,7 +165,7 @@ public object? Dependent /// This method mirrors , but without the allocation check. internal void UnsafeSetTarget(object? target) { - nSetPrimary(_handle, target); + InternalSetTarget(_handle, target); } /// @@ -174,7 +174,7 @@ internal void UnsafeSetTarget(object? target) /// This method mirrors , but without the allocation check. internal void UnsafeSetDependent(object? dependent) { - nSetSecondary(_handle, dependent); + InternalSetDependent(_handle, dependent); } /// @@ -190,9 +190,9 @@ internal void UnsafeSetDependent(object? dependent) { IntPtr handle = _handle; - object? target = nGetPrimary(handle); + object? target = InternalGetTarget(handle); - dependent = nGetSecondary(handle); + dependent = InternalGetDependent(handle); return target; } @@ -207,18 +207,18 @@ public void Dispose() { IntPtr handle = _handle; _handle = IntPtr.Zero; - nFree(handle); + InternalFree(handle); } } [MethodImpl(MethodImplOptions.InternalCall)] - private static extern IntPtr nInitialize(object? primary, object? secondary); + private static extern IntPtr InternalInitialize(object? target, object? dependent); #if DEBUG [MethodImpl(MethodImplOptions.InternalCall)] - private static extern object? nGetPrimary(IntPtr dependentHandle); + private static extern object? InternalGetTarget(IntPtr dependentHandle); #else - private static unsafe object? nGetPrimary(IntPtr dependentHandle) + private static unsafe object? InternalGetTarget(IntPtr dependentHandle) { // This optimization is the same that is used in GCHandle in RELEASE mode. // This is not used in DEBUG builds as the runtime performs additional checks. @@ -228,15 +228,15 @@ public void Dispose() #endif [MethodImpl(MethodImplOptions.InternalCall)] - private static extern object? nGetSecondary(IntPtr dependentHandle); + private static extern object? InternalGetDependent(IntPtr dependentHandle); [MethodImpl(MethodImplOptions.InternalCall)] - private static extern void nSetPrimary(IntPtr dependentHandle, object? primary); + private static extern void InternalSetTarget(IntPtr dependentHandle, object? target); [MethodImpl(MethodImplOptions.InternalCall)] - private static extern void nSetSecondary(IntPtr dependentHandle, object? secondary); + private static extern void InternalSetDependent(IntPtr dependentHandle, object? dependent); [MethodImpl(MethodImplOptions.InternalCall)] - private static extern void nFree(IntPtr dependentHandle); + private static extern void InternalFree(IntPtr dependentHandle); } } diff --git a/src/coreclr/vm/comdependenthandle.cpp b/src/coreclr/vm/comdependenthandle.cpp index 412c2551200777..151834cafd7b82 100644 --- a/src/coreclr/vm/comdependenthandle.cpp +++ b/src/coreclr/vm/comdependenthandle.cpp @@ -14,20 +14,18 @@ #include "common.h" #include "comdependenthandle.h" - - -FCIMPL2(OBJECTHANDLE, DependentHandle::nInitialize, Object *_primary, Object *_secondary) +FCIMPL2(OBJECTHANDLE, DependentHandle::InternalInitialize, Object *_target, Object *_dependent) { FCALL_CONTRACT; - OBJECTREF primary(_primary); - OBJECTREF secondary(_secondary); + OBJECTREF target(_target); + OBJECTREF dependent(_dependent); OBJECTHANDLE result = NULL; HELPER_METHOD_FRAME_BEGIN_RET_NOPOLL(); // Create the handle. - result = GetAppDomain()->CreateDependentHandle(primary, secondary); + result = GetAppDomain()->CreateDependentHandle(target, dependent); HELPER_METHOD_FRAME_END_POLL(); @@ -35,50 +33,45 @@ FCIMPL2(OBJECTHANDLE, DependentHandle::nInitialize, Object *_primary, Object *_s } FCIMPLEND - - -FCIMPL1(VOID, DependentHandle::nFree, OBJECTHANDLE handle) +FCIMPL1(Object*, DependentHandle::InternalGetTarget, OBJECTHANDLE handle) { FCALL_CONTRACT; - + FCUnique(0x54); _ASSERTE(handle != NULL); - - HELPER_METHOD_FRAME_BEGIN_0(); - - DestroyDependentHandle(handle); - - HELPER_METHOD_FRAME_END(); - + return OBJECTREFToObject(ObjectFromHandle(handle)); } FCIMPLEND - - -FCIMPL1(Object*, DependentHandle::nGetPrimary, OBJECTHANDLE handle) +FCIMPL1(Object*, DependentHandle::InternalGetDependent, OBJECTHANDLE handle) { FCALL_CONTRACT; - FCUnique(0x54); + _ASSERTE(handle != NULL); - return OBJECTREFToObject(ObjectFromHandle(handle)); -} -FCIMPLEND + OBJECTREF target = ObjectFromHandle(handle); + IGCHandleManager *mgr = GCHandleUtilities::GetGCHandleManager(); -FCIMPL1(Object*, DependentHandle::nGetSecondary, OBJECTHANDLE handle) + // The dependent is tracked only if target is non-null + return (target != NULL) ? mgr->GetDependentHandleSecondary(handle) : NULL; +} +FCIMPLEND + +FCIMPL1(VOID, DependentHandle::InternalFree, OBJECTHANDLE handle) { FCALL_CONTRACT; + _ASSERTE(handle != NULL); - OBJECTREF primary = ObjectFromHandle(handle); + HELPER_METHOD_FRAME_BEGIN_0(); - IGCHandleManager *mgr = GCHandleUtilities::GetGCHandleManager(); - // Secondary is tracked only if primary is non-null - return (primary != NULL) ? mgr->GetDependentHandleSecondary(handle) : NULL; + DestroyDependentHandle(handle); + + HELPER_METHOD_FRAME_END(); } FCIMPLEND -FCIMPL2(VOID, DependentHandle::nSetPrimary, OBJECTHANDLE handle, Object *_primary) +FCIMPL2(VOID, DependentHandle::InternalSetTarget, OBJECTHANDLE handle, Object *_target) { FCALL_CONTRACT; @@ -88,17 +81,17 @@ FCIMPL2(VOID, DependentHandle::nSetPrimary, OBJECTHANDLE handle, Object *_primar FCUnique(0x12); IGCHandleManager *mgr = GCHandleUtilities::GetGCHandleManager(); - mgr->StoreObjectInHandle(handle, _primary); + mgr->StoreObjectInHandle(handle, _target); } FCIMPLEND -FCIMPL2(VOID, DependentHandle::nSetSecondary, OBJECTHANDLE handle, Object *_secondary) +FCIMPL2(VOID, DependentHandle::InternalSetDependent, OBJECTHANDLE handle, Object *_dependent) { FCALL_CONTRACT; _ASSERTE(handle != NULL); IGCHandleManager *mgr = GCHandleUtilities::GetGCHandleManager(); - mgr->SetDependentHandleSecondary(handle, _secondary); + mgr->SetDependentHandleSecondary(handle, _dependent); } FCIMPLEND diff --git a/src/coreclr/vm/comdependenthandle.h b/src/coreclr/vm/comdependenthandle.h index d10de3c869a6eb..cbf8ef31fd4747 100644 --- a/src/coreclr/vm/comdependenthandle.h +++ b/src/coreclr/vm/comdependenthandle.h @@ -16,16 +16,16 @@ // A dependent handle is conceputally a tuple containing two object reference // -// * A Primary object (think key) -// * A Secondary Object (think value) +// * A Target object (think key) +// * A Dependent Object (think value) // -// The reference to both the primary object is (long) weak (will not keep the object alive). However the -// reference to the secondary object is (long) weak if the primary object is dead, and strong if the primary -// object is alive. (Hence it is a 'Dependent' handle since the strength of the secondary reference depends -// on the primary). +// The reference to both the target object is (long) weak (will not keep the object alive). However the +// reference to the dependent object is (long) weak if the target object is dead, and strong if the target +// object is alive. (Hence it is a 'Dependent' handle since the strength of the dependent reference depends +// on the target). // // The effect of this semantics is that it seems that while the DependentHandle exists, the system behaves as -// if there was a normal strong reference from the primary object to the secondary one. +// if there was a normal strong reference from the target object to the dependent one. // // The usefulness of a DependentHandle is to allow other objects to be 'attached' to a given object. By // having a hash table where the entries are dependent handles you can attach arbitrary objects to another @@ -40,13 +40,12 @@ class DependentHandle { public: - static FCDECL2(OBJECTHANDLE, nInitialize, Object *primary, Object *secondary); - static FCDECL1(Object *, nGetPrimary, OBJECTHANDLE handle); - static FCDECL1(Object *, nGetSecondary, OBJECTHANDLE handle); - static FCDECL1(VOID, nFree, OBJECTHANDLE handle); - static FCDECL2(VOID, nSetPrimary, OBJECTHANDLE handle, Object *primary); - static FCDECL2(VOID, nSetSecondary, OBJECTHANDLE handle, Object *secondary); + static FCDECL2(OBJECTHANDLE, InternalInitialize, Object *target, Object *dependent); + static FCDECL1(Object *, InternalGetTarget, OBJECTHANDLE handle); + static FCDECL1(Object *, InternalGetDependent, OBJECTHANDLE handle); + static FCDECL1(VOID, InternalFree, OBJECTHANDLE handle); + static FCDECL2(VOID, InternalSetTarget, OBJECTHANDLE handle, Object *target); + static FCDECL2(VOID, InternalSetDependent, OBJECTHANDLE handle, Object *dependent); }; #endif - diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index f2965c3ff6a1ef..87169bb074311d 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -61,12 +61,12 @@ FCFuncStart(gDependentHandleFuncs) - FCFuncElement("nInitialize", DependentHandle::nInitialize) - FCFuncElement("nGetPrimary", DependentHandle::nGetPrimary) - FCFuncElement("nGetSecondary", DependentHandle::nGetSecondary) - FCFuncElement("nFree", DependentHandle::nFree) - FCFuncElement("nSetPrimary", DependentHandle::nSetPrimary) - FCFuncElement("nSetSecondary", DependentHandle::nSetSecondary) + FCFuncElement("InternalInitialize", DependentHandle::InternalInitialize) + FCFuncElement("InternalGetTarget", DependentHandle::InternalGetTarget) + FCFuncElement("InternalGetDependent", DependentHandle::InternalGetDependent) + FCFuncElement("InternalFree", DependentHandle::InternalFree) + FCFuncElement("InternalSetTarget", DependentHandle::InternalSetTarget) + FCFuncElement("InternalSetDependent", DependentHandle::InternalSetDependent) FCFuncEnd() From 08df5980f7f424c1588f1b0f73425c660d5df401 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 23:44:35 +0200 Subject: [PATCH 20/30] Remove DependentHandle.UnsafeGetTargetAndDependent --- .../src/System/Runtime/DependentHandle.cs | 30 ++++------- .../CompilerServices/ConditionalWeakTable.cs | 30 ++++++++--- .../src/System/Runtime/DependentHandle.cs | 51 ++++--------------- 3 files changed, 43 insertions(+), 68 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 319c4d58f50f5a..6f866b421d35b0 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -159,6 +159,16 @@ public object? Dependent return InternalGetTarget(_handle); } + /// + /// Gets the dependent object instance for the current handle. + /// + /// The dependent object instance, if present. + /// This method mirrors , but without the allocation check. + internal object? UnsafeGetDependent() + { + return InternalGetDependent(_handle); + } + /// /// Sets the target object instance for the current handle. /// @@ -177,26 +187,6 @@ internal void UnsafeSetDependent(object? dependent) InternalSetDependent(_handle, dependent); } - /// - /// Retrieves the values of both and , if available. - /// - /// The dependent instance, if available. - /// The values of and . - /// - /// This method mirrors , but without the allocation check. - /// The signature is also kept the same as the one for the internal call, to improve the codegen. - /// - internal object? UnsafeGetTargetAndDependent(out object? dependent) - { - IntPtr handle = _handle; - - object? target = InternalGetTarget(handle); - - dependent = InternalGetDependent(handle); - - return target; - } - /// public void Dispose() { 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 67634bce43a8d6..e3920d454db5e6 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 @@ -523,10 +523,20 @@ internal int FindEntry(TKey key, out object? value) int bucket = hashCode & (_buckets.Length - 1); for (int entriesIndex = Volatile.Read(ref _buckets[bucket]); entriesIndex != -1; entriesIndex = _entries[entriesIndex].Next) { - if (_entries[entriesIndex].HashCode == hashCode && _entries[entriesIndex].depHnd.UnsafeGetTargetAndDependent(out value) == key) + if (_entries[entriesIndex].HashCode == hashCode) { - GC.KeepAlive(this); // ensure we don't get finalized while accessing DependentHandles. - return entriesIndex; + DependentHandle dependentHandle = _entries[entriesIndex].depHnd; + object? target = dependentHandle.UnsafeGetTarget(); + + if (target == key) + { + value = dependentHandle.UnsafeGetDependent(); + + GC.KeepAlive(target); // Ensure the dependent remains alive up until this point + GC.KeepAlive(this); // Ensure we don't get finalized while accessing DependentHandle + + return entriesIndex; + } } } @@ -540,13 +550,17 @@ internal bool TryGetEntry(int index, [NotNullWhen(true)] out TKey? key, [MaybeNu { if (index < _entries.Length) { - object? oKey = _entries[index].depHnd.UnsafeGetTargetAndDependent(out object? oValue); - GC.KeepAlive(this); // ensure we don't get finalized while accessing DependentHandles. + DependentHandle dependentHandle = _entries[index].depHnd; + object? target = dependentHandle.UnsafeGetTarget(); - if (oKey != null) + if (target is not null) { - key = Unsafe.As(oKey); - value = Unsafe.As(oValue); + object? dependent = dependentHandle.UnsafeGetDependent(); + + GC.KeepAlive(this); // Ensure we don't get finalized while accessing DependentHandle + + key = Unsafe.As(target); + value = Unsafe.As(dependent); return true; } } 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 94074539534fb8..80878fba2ddf27 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -34,7 +34,7 @@ public object? Target public object? Dependent { - get => GetDependent(); + get => UnsafeGetDependent(); set => UnsafeSetDependent(value); } @@ -70,7 +70,7 @@ public object? Dependent return key != GC.EPHEMERON_TOMBSTONE ? key : null; } - internal void UnsafeSetTarget(object? target) + internal object? UnsafeGetDependent() { Ephemeron[]? data = _data; @@ -78,13 +78,15 @@ internal void UnsafeSetTarget(object? target) { ThrowHelper.ThrowInvalidOperationException(); - return; + return default; } - data[0].Key = target; + Ephemeron e = data[0]; + + return e.Key != GC.EPHEMERON_TOMBSTONE ? e.Value : null; } - internal void UnsafeSetDependent(object? dependent) + internal void UnsafeSetTarget(object? target) { Ephemeron[]? data = _data; @@ -95,10 +97,10 @@ internal void UnsafeSetDependent(object? dependent) return; } - data[0].Value = dependent; + data[0].Key = target; } - internal object? UnsafeGetTargetAndDependent(out object? dependent) + internal void UnsafeSetDependent(object? dependent) { Ephemeron[]? data = _data; @@ -106,46 +108,15 @@ internal void UnsafeSetDependent(object? dependent) { ThrowHelper.ThrowInvalidOperationException(); - dependent = null; - - return null; - } - - Ephemeron e = data[0]; - - if (e.Key != GC.EPHEMERON_TOMBSTONE) - { - dependent = e.Value; - - return e.Key; + return; } - else - { - dependent = null; - return null; - } + data[0].Value = dependent; } public void Dispose() { _data = null; } - - private object? GetDependent() - { - Ephemeron[]? data = _data; - - if (data is null) - { - ThrowHelper.ThrowInvalidOperationException(); - - return default; - } - - Ephemeron e = data[0]; - - return e.Key != GC.EPHEMERON_TOMBSTONE ? e.Value : null; - } } } From 4e2b624c52023bf5f5e4e1a4479be37e75ad2f84 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 16 Jun 2021 23:59:10 +0200 Subject: [PATCH 21/30] Remove DependentHandle.GetTargetAndDependent --- .../src/System/Runtime/DependentHandle.cs | 35 +++++++------------ .../System.Runtime/ref/System.Runtime.cs | 1 - .../System/Runtime/DependentHandleTests.cs | 6 ---- .../src/System/Runtime/DependentHandle.cs | 16 --------- 4 files changed, 12 insertions(+), 46 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 6f866b421d35b0..89a207324c253f 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -19,6 +19,11 @@ namespace System.Runtime /// that object having a field or property (or some other strong reference) to a dependent object instance B. /// /// + /// + /// The type is not thread-safe, and consumers are responsible for ensuring that + /// is not called concurrently with other APIs. Not doing so results in undefined behavior. + /// The and properties are instead thread-safe. + /// public struct DependentHandle : IDisposable { // ========================================================================================= @@ -68,6 +73,7 @@ public DependentHandle(object? target, object? dependent) /// Gets or sets the target object instance for the current handle. /// /// Thrown if is . + /// This property is thread-safe. public object? Target { get @@ -98,11 +104,13 @@ public object? Target /// Gets or sets the dependent object instance for the current handle. /// /// - /// If it is necessary to retrieve both and , it is - /// recommended to use instead. This will result in better - /// performance and it will reduce the chance of unexpected behavior in some cases. + /// If it is needed to retrieve both and , it is necessary + /// to ensure that the returned instance from will be kept alive until + /// is retrieved as well, or it might be collected and result in unexpected behavior. This can be done by storing the + /// target in a local and calling on it after is accessed. /// /// Thrown if is . + /// This property is thread-safe. public object? Dependent { get @@ -129,26 +137,6 @@ public object? Dependent } } - /// - /// Retrieves the values of both and , if available. - /// - /// The values of and . - /// Thrown if is . - public (object? Target, object? Dependent) GetTargetAndDependent() - { - IntPtr handle = _handle; - - if (handle == IntPtr.Zero) - { - ThrowHelper.ThrowInvalidOperationException(); - } - - object? target = InternalGetTarget(handle); - object? secondary = InternalGetDependent(handle); - - return (target, secondary); - } - /// /// Gets the target object instance for the current handle. /// @@ -188,6 +176,7 @@ internal void UnsafeSetDependent(object? dependent) } /// + /// This method is not thread-safe. public void Dispose() { // Forces dependentHandle back to non-allocated state (if not already there) diff --git a/src/libraries/System.Runtime/ref/System.Runtime.cs b/src/libraries/System.Runtime/ref/System.Runtime.cs index a26394fecac08c..25bce4e1e3be20 100644 --- a/src/libraries/System.Runtime/ref/System.Runtime.cs +++ b/src/libraries/System.Runtime/ref/System.Runtime.cs @@ -9602,7 +9602,6 @@ public partial struct DependentHandle : System.IDisposable public bool IsAllocated { get { throw null; } } public object? Target { get { throw null; } set { } } public void Dispose() { } - public (object? Target, object? Dependent) GetTargetAndDependent() { 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 dbfe64093518e6..7f820692ee0f2d 100644 --- a/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs +++ b/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs @@ -41,12 +41,6 @@ public void SetDependent_ThrowsInvalidOperationException() }); } - [Fact] - public void GetTargetAndDependent_ThrowsInvalidOperationException() - { - Assert.Throws(() => default(DependentHandle).GetTargetAndDependent()); - } - [Fact] public void Dispose_RepeatedCallsAreFine() { 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 80878fba2ddf27..33e8044547ff00 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -38,22 +38,6 @@ public object? Dependent set => UnsafeSetDependent(value); } - public (object? Target, object? Dependent) GetTargetAndDependent() - { - Ephemeron[]? data = _data; - - if (data is null) - { - ThrowHelper.ThrowInvalidOperationException(); - - return default; - } - - Ephemeron e = data[0]; - - return e.Key != GC.EPHEMERON_TOMBSTONE ? (e.Key, e.Value) : default; - } - internal object? UnsafeGetTarget() { Ephemeron[]? data = _data; From 4e032976b48d0066616906b6a07332d7aa9519bf Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 17 Jun 2021 01:43:41 +0200 Subject: [PATCH 22/30] Fix FCall path for internal DependentHandle APIs --- src/coreclr/vm/ecalllist.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 87169bb074311d..e16091c78ec10d 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -1140,7 +1140,7 @@ FCClassElement("CustomAttribute", "System.Reflection", gCOMCustomAttributeFuncs) FCClassElement("CustomAttributeEncodedArgument", "System.Reflection", gCustomAttributeEncodedArgument) FCClassElement("Debugger", "System.Diagnostics", gDiagnosticsDebugger) FCClassElement("Delegate", "System", gDelegateFuncs) -FCClassElement("DependentHandle", "System.Runtime.CompilerServices", gDependentHandleFuncs) +FCClassElement("DependentHandle", "System.Runtime", gDependentHandleFuncs) FCClassElement("Enum", "System", gEnumFuncs) FCClassElement("Environment", "System", gEnvironmentFuncs) #if defined(FEATURE_PERFTRACING) From 25b34c2ca388ee67b542b8a7d70d6679322153d9 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 17 Jun 2021 11:23:39 +0200 Subject: [PATCH 23/30] Add more DependentHandle unit tests --- .../System/Runtime/DependentHandleTests.cs | 158 ++++++++++++++++++ 1 file changed, 158 insertions(+) diff --git a/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs b/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs index 7f820692ee0f2d..2160cc16df77b9 100644 --- a/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs +++ b/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Runtime.CompilerServices; using Xunit; namespace System.Runtime.Tests @@ -9,6 +10,163 @@ namespace System.Runtime.Tests // This class contains some specific tests for APIs that are only relevant when used directly. public class DependentHandleTests { + [Fact] + public void GetSetTarget() + { + object target = new(); + DependentHandle handle = new(null, null); + + Assert.True(handle.IsAllocated); + Assert.Null(handle.Target); + Assert.Null(handle.Dependent); + + handle.Target = target; + + // A handle with a set target and no dependent is valid + Assert.Same(target, handle.Target); + Assert.Null(handle.Dependent); + + handle.Dispose(); + } + + [Fact] + public void GetSetDependent() + { + object target = new(), dependent = new(); + DependentHandle handle = new(target, null); + + // The target can be retrieved correctly + Assert.True(handle.IsAllocated); + Assert.Same(target, handle.Target); + Assert.Null(handle.Dependent); + + handle.Dependent = dependent; + + // The dependent can also be retrieved correctly + Assert.Same(target, handle.Target); + Assert.Same(dependent, handle.Dependent); + + handle.Dispose(); + } + + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))] + public void TargetKeepsDependentAlive() + { + [MethodImpl(MethodImplOptions.NoInlining)] + static DependentHandle Initialize(out object target, out WeakReference weakDependent) + { + target = new object(); + + object dependent = new(); + + weakDependent = new WeakReference(dependent); + + return new DependentHandle(target, dependent); + } + + DependentHandle handle = Initialize(out object target, out WeakReference dependent); + + GC.Collect(); + + // The dependent has to still be alive as the target has a strong reference + Assert.Same(target, handle.Target); + Assert.True(dependent.IsAlive); + Assert.Same(dependent.Target, handle.Dependent); + + GC.KeepAlive(target); + + handle.Dispose(); + } + + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))] + public void DependentDoesNotKeepTargetAlive() + { + [MethodImpl(MethodImplOptions.NoInlining)] + static DependentHandle Initialize(out WeakReference weakTarget, out object dependent) + { + dependent = new object(); + + object target = new(); + + weakTarget = new WeakReference(target); + + return new DependentHandle(target, dependent); + } + + DependentHandle handle = Initialize(out WeakReference target, out object dependent); + + GC.Collect(); + + // The target has to be collected, as there were no strong references to it + Assert.Null(handle.Target); + Assert.False(target.IsAlive); + + GC.KeepAlive(target); + + handle.Dispose(); + } + + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))] + public void DependentIsCollectedOnTargetNotReachable() + { + [MethodImpl(MethodImplOptions.NoInlining)] + static DependentHandle Initialize(out WeakReference weakTarget, out WeakReference weakDependent) + { + object target = new(), dependent = new(); + + weakTarget = new WeakReference(target); + weakDependent = new WeakReference(dependent); + + return new DependentHandle(target, dependent); + } + + DependentHandle handle = Initialize(out WeakReference target, out WeakReference dependent); + + GC.Collect(); + + // Both target and dependent have to be collected, as there were no strong references to either + Assert.Null(handle.Target); + Assert.Null(handle.Dependent); + Assert.False(target.IsAlive); + Assert.False(dependent.IsAlive); + + handle.Dispose(); + } + + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))] + public void DependentIsCollectedOnTargetNotReachable_EvenWithReferenceCycles() + { + [MethodImpl(MethodImplOptions.NoInlining)] + static DependentHandle Initialize(out WeakReference weakTarget, out WeakReference weakDependent) + { + object target = new(); + ObjectWithReference dependent = new() { Reference = target }; + + weakTarget = new WeakReference(target); + weakDependent = new WeakReference(dependent); + + return new DependentHandle(target, dependent); + } + + DependentHandle handle = Initialize(out WeakReference target, out WeakReference dependent); + + GC.Collect(); + + // Both target and dependent have to be collected, as there were no strong references to either. + // The fact that the dependent has a strong reference back to the target should not affect this. + Assert.Null(handle.Target); + Assert.Null(handle.Dependent); + Assert.False(target.IsAlive); + Assert.False(dependent.IsAlive); + + handle.Dispose(); + } + + private sealed class ObjectWithReference + { + public object Reference; + } + [Fact] public void GetTarget_ThrowsInvalidOperationException() { From 01f32a35b5ce03665de8581cea976ed8cf1ce15f Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 17 Jun 2021 18:58:31 +0200 Subject: [PATCH 24/30] Reintroduce DependentHandle.GetTargetAndDependent() This fixes a bug due to a race condition in ConditionalWeakTable, which relies on this method which atomically retrieves both target and dependent with respect to target being set to null concurrently by other threads. This also exposes the same API publically to allow consumers to potentially implement custom conditional weak tables in the same manner. --- .../src/System/Runtime/DependentHandle.cs | 41 ++++++++++++++++--- src/coreclr/vm/comdependenthandle.cpp | 18 ++++++++ src/coreclr/vm/comdependenthandle.h | 1 + src/coreclr/vm/ecalllist.h | 13 +++--- .../CompilerServices/ConditionalWeakTable.cs | 33 +++++---------- .../System.Runtime/ref/System.Runtime.cs | 1 + .../src/System/Runtime/DependentHandle.cs | 34 +++++++++++++++ 7 files changed, 108 insertions(+), 33 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 89a207324c253f..8c5f014434f843 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -137,6 +137,29 @@ public object? Dependent } } + /// + /// Atomically retrieves the values of both and , if available. + /// 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 + /// could be possible to sometimes successfully retrieve the previous target, but then fail to get the dependent. + /// + /// The values of and . + /// Thrown if is . + public (object? Target, object? Dependent) GetTargetAndDependent() + { + IntPtr handle = _handle; + + if (handle == IntPtr.Zero) + { + ThrowHelper.ThrowInvalidOperationException(); + } + + object? target = InternalGetTargetAndDependent(handle, out object? dependent); + + return (target, dependent); + } + /// /// Gets the target object instance for the current handle. /// @@ -148,13 +171,18 @@ public object? Dependent } /// - /// Gets the dependent object instance for the current handle. + /// Atomically retrieves the values of both and , if available. /// - /// The dependent object instance, if present. - /// This method mirrors , but without the allocation check. - internal object? UnsafeGetDependent() + /// The dependent instance, if available. + /// The values of and . + /// + /// This method mirrors , 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). + /// + internal object? UnsafeGetTargetAndDependent(out object? dependent) { - return InternalGetDependent(_handle); + return InternalGetTargetAndDependent(_handle, out dependent); } /// @@ -209,6 +237,9 @@ public void Dispose() [MethodImpl(MethodImplOptions.InternalCall)] private static extern object? InternalGetDependent(IntPtr dependentHandle); + [MethodImpl(MethodImplOptions.InternalCall)] + private static extern object? InternalGetTargetAndDependent(IntPtr dependentHandle, out object? dependent); + [MethodImpl(MethodImplOptions.InternalCall)] private static extern void InternalSetTarget(IntPtr dependentHandle, object? target); diff --git a/src/coreclr/vm/comdependenthandle.cpp b/src/coreclr/vm/comdependenthandle.cpp index 151834cafd7b82..b26480aac90ac3 100644 --- a/src/coreclr/vm/comdependenthandle.cpp +++ b/src/coreclr/vm/comdependenthandle.cpp @@ -37,7 +37,9 @@ FCIMPL1(Object*, DependentHandle::InternalGetTarget, OBJECTHANDLE handle) { FCALL_CONTRACT; FCUnique(0x54); + _ASSERTE(handle != NULL); + return OBJECTREFToObject(ObjectFromHandle(handle)); } FCIMPLEND @@ -57,6 +59,22 @@ FCIMPL1(Object*, DependentHandle::InternalGetDependent, OBJECTHANDLE handle) } FCIMPLEND +FCIMPL2(Object*, DependentHandle::InternalGetTargetAndDependent, OBJECTHANDLE handle, Object **outDependent) +{ + FCALL_CONTRACT; + + _ASSERTE(handle != NULL && outDependent != NULL); + + OBJECTREF target = ObjectFromHandle(handle); + IGCHandleManager *mgr = GCHandleUtilities::GetGCHandleManager(); + + // The dependent is tracked only if target is non-null + *outDependent = (target != NULL) ? mgr->GetDependentHandleSecondary(handle) : NULL; + + return OBJECTREFToObject(target); +} +FCIMPLEND + FCIMPL1(VOID, DependentHandle::InternalFree, OBJECTHANDLE handle) { FCALL_CONTRACT; diff --git a/src/coreclr/vm/comdependenthandle.h b/src/coreclr/vm/comdependenthandle.h index cbf8ef31fd4747..5c7e055bd65531 100644 --- a/src/coreclr/vm/comdependenthandle.h +++ b/src/coreclr/vm/comdependenthandle.h @@ -43,6 +43,7 @@ class DependentHandle static FCDECL2(OBJECTHANDLE, InternalInitialize, Object *target, Object *dependent); static FCDECL1(Object *, InternalGetTarget, OBJECTHANDLE handle); static FCDECL1(Object *, InternalGetDependent, OBJECTHANDLE handle); + static FCDECL2(Object *, InternalGetTargetAndDependent, OBJECTHANDLE handle, Object **outDependent); static FCDECL1(VOID, InternalFree, OBJECTHANDLE handle); static FCDECL2(VOID, InternalSetTarget, OBJECTHANDLE handle, Object *target); static FCDECL2(VOID, InternalSetDependent, OBJECTHANDLE handle, Object *dependent); diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index e16091c78ec10d..04831c35b43c47 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -61,12 +61,13 @@ FCFuncStart(gDependentHandleFuncs) - FCFuncElement("InternalInitialize", DependentHandle::InternalInitialize) - FCFuncElement("InternalGetTarget", DependentHandle::InternalGetTarget) - FCFuncElement("InternalGetDependent", DependentHandle::InternalGetDependent) - FCFuncElement("InternalFree", DependentHandle::InternalFree) - FCFuncElement("InternalSetTarget", DependentHandle::InternalSetTarget) - FCFuncElement("InternalSetDependent", DependentHandle::InternalSetDependent) + FCFuncElement("InternalInitialize", DependentHandle::InternalInitialize) + FCFuncElement("InternalGetTarget", DependentHandle::InternalGetTarget) + FCFuncElement("InternalGetDependent", DependentHandle::InternalGetDependent) + FCFuncElement("InternalGetTargetAndDependent", DependentHandle::InternalGetTargetAndDependent) + FCFuncElement("InternalFree", DependentHandle::InternalFree) + FCFuncElement("InternalSetTarget", DependentHandle::InternalSetTarget) + FCFuncElement("InternalSetDependent", DependentHandle::InternalSetDependent) 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 e3920d454db5e6..cfbe6db0038e28 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 @@ -515,6 +515,7 @@ internal bool TryGetValueWorker(TKey key, [MaybeNullWhen(false)] out TValue valu /// Returns -1 if not found (if key expires during FindEntry, this can be treated as "not found."). /// Must hold _lock, or be prepared to retry the search while holding _lock. /// + /// This method requires to be on the stack to be properly tracked. internal int FindEntry(TKey key, out object? value) { Debug.Assert(key != null); // Key already validated as non-null. @@ -523,24 +524,15 @@ internal int FindEntry(TKey key, out object? value) int bucket = hashCode & (_buckets.Length - 1); for (int entriesIndex = Volatile.Read(ref _buckets[bucket]); entriesIndex != -1; entriesIndex = _entries[entriesIndex].Next) { - if (_entries[entriesIndex].HashCode == hashCode) + if (_entries[entriesIndex].HashCode == hashCode && _entries[entriesIndex].depHnd.UnsafeGetTargetAndDependent(out value) == key) { - DependentHandle dependentHandle = _entries[entriesIndex].depHnd; - object? target = dependentHandle.UnsafeGetTarget(); - - if (target == key) - { - value = dependentHandle.UnsafeGetDependent(); - - GC.KeepAlive(target); // Ensure the dependent remains alive up until this point - GC.KeepAlive(this); // Ensure we don't get finalized while accessing DependentHandle + GC.KeepAlive(this); // Ensure we don't get finalized while accessing DependentHandle - return entriesIndex; - } + return entriesIndex; } } - GC.KeepAlive(this); // ensure we don't get finalized while accessing DependentHandles. + GC.KeepAlive(this); // Ensure we don't get finalized while accessing DependentHandle value = null; return -1; } @@ -550,17 +542,14 @@ internal bool TryGetEntry(int index, [NotNullWhen(true)] out TKey? key, [MaybeNu { if (index < _entries.Length) { - DependentHandle dependentHandle = _entries[index].depHnd; - object? target = dependentHandle.UnsafeGetTarget(); + object? oKey = _entries[index].depHnd.UnsafeGetTargetAndDependent(out object? oValue); - if (target is not null) - { - object? dependent = dependentHandle.UnsafeGetDependent(); + GC.KeepAlive(this); // Ensure we don't get finalized while accessing DependentHandle - GC.KeepAlive(this); // Ensure we don't get finalized while accessing DependentHandle - - key = Unsafe.As(target); - value = Unsafe.As(dependent); + if (oKey != null) + { + key = Unsafe.As(oKey); + value = Unsafe.As(oValue); return true; } } diff --git a/src/libraries/System.Runtime/ref/System.Runtime.cs b/src/libraries/System.Runtime/ref/System.Runtime.cs index 25bce4e1e3be20..a26394fecac08c 100644 --- a/src/libraries/System.Runtime/ref/System.Runtime.cs +++ b/src/libraries/System.Runtime/ref/System.Runtime.cs @@ -9602,6 +9602,7 @@ public partial struct DependentHandle : System.IDisposable public bool IsAllocated { get { throw null; } } public object? Target { get { throw null; } set { } } public void Dispose() { } + public (object? Target, object? Dependent) GetTargetAndDependent() { throw null; } } public enum GCLargeObjectHeapCompactionMode { 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 33e8044547ff00..ef6ea9ef1523cc 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -38,6 +38,13 @@ public object? Dependent set => UnsafeSetDependent(value); } + public (object? Target, object? Dependent) GetTargetAndDependent() + { + object? target = UnsafeGetTargetAndDependent(out object? dependent); + + return (target, dependent); + } + internal object? UnsafeGetTarget() { Ephemeron[]? data = _data; @@ -70,6 +77,33 @@ public object? Dependent return e.Key != GC.EPHEMERON_TOMBSTONE ? e.Value : null; } + internal object? UnsafeGetTargetAndDependent(out object? dependent) + { + Ephemeron[]? data = _data; + + if (data is null) + { + ThrowHelper.ThrowInvalidOperationException(); + + dependent = null; + + return null; + } + + Ephemeron e = data[0]; + + if (e.Key != GC.EPHEMERON_TOMBSTONE) + { + dependent = e.Value; + + return e.Key; + } + + dependent = null; + + return null; + } + internal void UnsafeSetTarget(object? target) { Ephemeron[]? data = _data; From b3963f23bb5f4b9b935ad2d627c2cd5ed93d60a8 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Fri, 18 Jun 2021 13:43:36 +0200 Subject: [PATCH 25/30] Minor IL tweaks to produce smaller IR in the JIT --- .../src/System/Runtime/DependentHandle.cs | 21 ++++++++++--------- .../Runtime/InteropServices/GCHandle.cs | 8 +++---- 2 files changed, 15 insertions(+), 14 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 8c5f014434f843..4980b552785816 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -67,7 +67,7 @@ public DependentHandle(object? target, object? dependent) /// /// Gets a value indicating whether this handle has been allocated or not. /// - public bool IsAllocated => _handle != IntPtr.Zero; + public bool IsAllocated => (nint)_handle != 0; /// /// Gets or sets the target object instance for the current handle. @@ -80,7 +80,7 @@ public object? Target { IntPtr handle = _handle; - if (handle == IntPtr.Zero) + if ((nint)handle == 0) { ThrowHelper.ThrowInvalidOperationException(); } @@ -91,7 +91,7 @@ public object? Target { IntPtr handle = _handle; - if (handle == IntPtr.Zero) + if ((nint)handle == 0) { ThrowHelper.ThrowInvalidOperationException(); } @@ -117,7 +117,7 @@ public object? Dependent { IntPtr handle = _handle; - if (handle == IntPtr.Zero) + if ((nint)handle == 0) { ThrowHelper.ThrowInvalidOperationException(); } @@ -128,7 +128,7 @@ public object? Dependent { IntPtr handle = _handle; - if (handle == IntPtr.Zero) + if ((nint)handle == 0) { ThrowHelper.ThrowInvalidOperationException(); } @@ -150,7 +150,7 @@ public object? Dependent { IntPtr handle = _handle; - if (handle == IntPtr.Zero) + if ((nint)handle == 0) { ThrowHelper.ThrowInvalidOperationException(); } @@ -207,13 +207,14 @@ internal void UnsafeSetDependent(object? dependent) /// This method is not thread-safe. public void Dispose() { - // Forces dependentHandle back to non-allocated state (if not already there) - // and frees the handle if needed. + // Forces the DependentHandle back to non-allocated state + // (if not already there) and frees the handle if needed. + IntPtr handle = _handle; - if (_handle != IntPtr.Zero) + if ((nint)handle != 0) { - IntPtr handle = _handle; _handle = IntPtr.Zero; + InternalFree(handle); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs index 78aa52682e417a..579633db55cafc 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs @@ -142,7 +142,7 @@ public IntPtr AddrOfPinnedObject() } /// Determine whether this handle has been allocated or not. - public bool IsAllocated => _handle != IntPtr.Zero; + public bool IsAllocated => (nint)_handle != 0; /// /// Used to create a GCHandle from an int. This is intended to @@ -165,9 +165,9 @@ public static GCHandle FromIntPtr(IntPtr value) public override bool Equals([NotNullWhen(true)] object? o) => o is GCHandle && _handle == ((GCHandle)o)._handle; - public static bool operator ==(GCHandle a, GCHandle b) => a._handle == b._handle; + public static bool operator ==(GCHandle a, GCHandle b) => (nint)a._handle == (nint)b._handle; - public static bool operator !=(GCHandle a, GCHandle b) => a._handle != b._handle; + public static bool operator !=(GCHandle a, GCHandle b) => (nint)a._handle != (nint)b._handle; [MethodImpl(MethodImplOptions.AggressiveInlining)] private static IntPtr GetHandleValue(IntPtr handle) => new IntPtr((nint)handle & ~(nint)1); // Remove Pin flag @@ -179,7 +179,7 @@ public static GCHandle FromIntPtr(IntPtr value) private static void ThrowIfInvalid(IntPtr handle) { // Check if the handle was never initialized or was freed. - if (handle == IntPtr.Zero) + if ((nint)handle == 0) { ThrowHelper.ThrowInvalidOperationException_HandleIsNotInitialized(); } From 34e1bcbceac86e8edb3ccf548a44844b5b40ac89 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Mon, 21 Jun 2021 15:48:55 +0200 Subject: [PATCH 26/30] Add DependentHandle.StopTracking() API This also fixes two potential GC holes when setting DependentHandle.Target (see conversation from https://github.com/dotnet/runtime/pull/54246#issuecomment-863285327 onwards) --- .../src/System/Runtime/DependentHandle.cs | 53 +++++---- .../CompilerServices/ConditionalWeakTable.cs | 2 +- .../System.Runtime/ref/System.Runtime.cs | 3 +- .../System/Runtime/DependentHandleTests.cs | 101 ++++++++++++++++-- .../src/System/Runtime/DependentHandle.cs | 23 ++-- 5 files changed, 138 insertions(+), 44 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 4980b552785816..ed94e59e4cad8d 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -70,7 +70,7 @@ public DependentHandle(object? target, object? dependent) public bool IsAllocated => (nint)_handle != 0; /// - /// Gets or sets the target object instance for the current handle. + /// Gets the target object instance for the current handle. /// /// Thrown if is . /// This property is thread-safe. @@ -87,17 +87,6 @@ public object? Target return InternalGetTarget(handle); } - set - { - IntPtr handle = _handle; - - if ((nint)handle == 0) - { - ThrowHelper.ThrowInvalidOperationException(); - } - - InternalSetTarget(handle, value); - } } /// @@ -160,6 +149,29 @@ public object? Dependent return (target, dependent); } + /// + /// 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; + + if ((nint)handle == 0) + { + ThrowHelper.ThrowInvalidOperationException(); + } + + InternalSetTarget(handle, null); + } + /// /// Gets the target object instance for the current handle. /// @@ -186,21 +198,22 @@ public object? Dependent } /// - /// Sets the target object instance for the current handle. + /// Sets the dependent object instance for the current handle. /// - /// This method mirrors , but without the allocation check. - internal void UnsafeSetTarget(object? target) + /// This method mirrors , but without the allocation check. + internal void UnsafeSetDependent(object? dependent) { - InternalSetTarget(_handle, target); + InternalSetDependent(_handle, dependent); } /// - /// Sets the dependent object instance for the current handle. + /// Stops tracking the target and dependent objects in the current instance. /// - /// This method mirrors , but without the allocation check. - internal void UnsafeSetDependent(object? dependent) + /// Thrown if is . + /// This method mirrors , but without the allocation check. + internal void UnsafeStopTracking() { - InternalSetDependent(_handle, dependent); + InternalSetTarget(_handle, null); } /// 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 cfbe6db0038e28..35acec95469881 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.UnsafeSetTarget(null); + entry.depHnd.UnsafeStopTracking(); } 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 a26394fecac08c..14e91b725e4ada 100644 --- a/src/libraries/System.Runtime/ref/System.Runtime.cs +++ b/src/libraries/System.Runtime/ref/System.Runtime.cs @@ -9600,9 +9600,10 @@ 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; } set { } } + public object? Target { 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 2160cc16df77b9..94c2afed33bf9e 100644 --- a/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs +++ b/src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs @@ -11,7 +11,7 @@ namespace System.Runtime.Tests public class DependentHandleTests { [Fact] - public void GetSetTarget() + public void GetNullTarget() { object target = new(); DependentHandle handle = new(null, null); @@ -20,9 +20,17 @@ public void GetSetTarget() Assert.Null(handle.Target); Assert.Null(handle.Dependent); - handle.Target = target; + handle.Dispose(); + } + + [Fact] + public void GetNotNullTarget() + { + object target = new(); + DependentHandle handle = new(target, null); // A handle with a set target and no dependent is valid + Assert.True(handle.IsAllocated); Assert.Same(target, handle.Target); Assert.Null(handle.Dependent); @@ -168,19 +176,84 @@ private sealed class ObjectWithReference } [Fact] - public void GetTarget_ThrowsInvalidOperationException() + public void StopTracking_RepeatedCallsAreFine() { - Assert.Throws(() => default(DependentHandle).Target); + 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 SetTarget_ThrowsInvalidOperationException() + public void StopTracking_StateIsConsistent() { - Assert.Throws(() => + 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() + { + [MethodImpl(MethodImplOptions.NoInlining)] + static DependentHandle Initialize(out object target, out WeakReference weakDependent) { - DependentHandle handle = default; - handle.Target = new(); - }); + target = new(); + + object dependent = new(); + + weakDependent = new WeakReference(dependent); + + return new DependentHandle(target, dependent); + } + + DependentHandle handle = Initialize(out object target, out WeakReference dependent); + + handle.StopTracking(); + + GC.Collect(); + + // After calling StopTracking, the dependent is collected even if + // target is still alive and the handle itself has not been disposed + Assert.True(handle.IsAllocated); + Assert.Null(handle.Target); + Assert.Null(handle.Dependent); + Assert.False(dependent.IsAlive); + + GC.KeepAlive(target); + + handle.Dispose(); + } + + [Fact] + public void GetTarget_ThrowsInvalidOperationException() + { + Assert.Throws(() => default(DependentHandle).Target); } [Fact] @@ -199,11 +272,17 @@ public void SetDependent_ThrowsInvalidOperationException() }); } + [Fact] + public void StopTracking_ThrowsInvalidOperationException() + { + Assert.Throws(() => default(DependentHandle).StopTracking()); + } + [Fact] public void Dispose_RepeatedCallsAreFine() { - object key = new(), value = new(); - DependentHandle handle = new(key, value); + object target = new(), dependent = new(); + DependentHandle handle = new(target, dependent); Assert.True(handle.IsAllocated); 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 ef6ea9ef1523cc..a0a7e26c72b3dc 100644 --- a/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -26,11 +26,7 @@ public DependentHandle(object? target, object? dependent) public bool IsAllocated => _data is not null; - public object? Target - { - get => UnsafeGetTarget(); - set => UnsafeSetTarget(value); - } + public object? Target => UnsafeGetTarget(); public object? Dependent { @@ -45,6 +41,11 @@ public object? Dependent return (target, dependent); } + public void StopTracking() + { + UnsafeStopTracking(); + } + internal object? UnsafeGetTarget() { Ephemeron[]? data = _data; @@ -74,7 +75,7 @@ public object? Dependent Ephemeron e = data[0]; - return e.Key != GC.EPHEMERON_TOMBSTONE ? e.Value : null; + return e.Key != GC.EPHEMERON_TOMBSTONE && e.Key is not null ? e.Value : null; } internal object? UnsafeGetTargetAndDependent(out object? dependent) @@ -92,7 +93,7 @@ public object? Dependent Ephemeron e = data[0]; - if (e.Key != GC.EPHEMERON_TOMBSTONE) + if (e.Key != GC.EPHEMERON_TOMBSTONE && e.Key is not null) { dependent = e.Value; @@ -104,7 +105,7 @@ public object? Dependent return null; } - internal void UnsafeSetTarget(object? target) + internal void UnsafeSetDependent(object? dependent) { Ephemeron[]? data = _data; @@ -115,10 +116,10 @@ internal void UnsafeSetTarget(object? target) return; } - data[0].Key = target; + data[0].Value = dependent; } - internal void UnsafeSetDependent(object? dependent) + internal void UnsafeStopTracking() { Ephemeron[]? data = _data; @@ -129,7 +130,7 @@ internal void UnsafeSetDependent(object? dependent) return; } - data[0].Value = dependent; + data[0].Key = null; } public void Dispose() From 9fd1da4a759b3eb4f6ba2c62e895a048875151cb Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Mon, 21 Jun 2021 15:58:44 +0200 Subject: [PATCH 27/30] Rename InternalSetTarget to StopTracking, remove redundant param --- .../src/System/Runtime/DependentHandle.cs | 8 +++---- src/coreclr/vm/comdependenthandle.cpp | 22 +++++++++---------- src/coreclr/vm/comdependenthandle.h | 4 ++-- src/coreclr/vm/ecalllist.h | 4 ++-- 4 files changed, 19 insertions(+), 19 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 ed94e59e4cad8d..8ae58eba738202 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -169,7 +169,7 @@ public void StopTracking() ThrowHelper.ThrowInvalidOperationException(); } - InternalSetTarget(handle, null); + InternalStopTracking(handle); } /// @@ -213,7 +213,7 @@ internal void UnsafeSetDependent(object? dependent) /// This method mirrors , but without the allocation check. internal void UnsafeStopTracking() { - InternalSetTarget(_handle, null); + InternalStopTracking(_handle); } /// @@ -255,10 +255,10 @@ public void Dispose() private static extern object? InternalGetTargetAndDependent(IntPtr dependentHandle, out object? dependent); [MethodImpl(MethodImplOptions.InternalCall)] - private static extern void InternalSetTarget(IntPtr dependentHandle, object? target); + private static extern void InternalSetDependent(IntPtr dependentHandle, object? dependent); [MethodImpl(MethodImplOptions.InternalCall)] - private static extern void InternalSetDependent(IntPtr dependentHandle, object? dependent); + private static extern void InternalStopTracking(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 b26480aac90ac3..c2be2258be52da 100644 --- a/src/coreclr/vm/comdependenthandle.cpp +++ b/src/coreclr/vm/comdependenthandle.cpp @@ -75,21 +75,18 @@ FCIMPL2(Object*, DependentHandle::InternalGetTargetAndDependent, OBJECTHANDLE ha } FCIMPLEND -FCIMPL1(VOID, DependentHandle::InternalFree, OBJECTHANDLE handle) +FCIMPL2(VOID, DependentHandle::InternalSetDependent, OBJECTHANDLE handle, Object *_dependent) { FCALL_CONTRACT; _ASSERTE(handle != NULL); - HELPER_METHOD_FRAME_BEGIN_0(); - - DestroyDependentHandle(handle); - - HELPER_METHOD_FRAME_END(); + IGCHandleManager *mgr = GCHandleUtilities::GetGCHandleManager(); + mgr->SetDependentHandleSecondary(handle, _dependent); } FCIMPLEND -FCIMPL2(VOID, DependentHandle::InternalSetTarget, OBJECTHANDLE handle, Object *_target) +FCIMPL1(VOID, DependentHandle::InternalStopTracking, OBJECTHANDLE handle) { FCALL_CONTRACT; @@ -99,17 +96,20 @@ FCIMPL2(VOID, DependentHandle::InternalSetTarget, OBJECTHANDLE handle, Object *_ FCUnique(0x12); IGCHandleManager *mgr = GCHandleUtilities::GetGCHandleManager(); - mgr->StoreObjectInHandle(handle, _target); + mgr->StoreObjectInHandle(handle, NULL); } FCIMPLEND -FCIMPL2(VOID, DependentHandle::InternalSetDependent, OBJECTHANDLE handle, Object *_dependent) +FCIMPL1(VOID, DependentHandle::InternalFree, OBJECTHANDLE handle) { FCALL_CONTRACT; _ASSERTE(handle != NULL); - IGCHandleManager *mgr = GCHandleUtilities::GetGCHandleManager(); - mgr->SetDependentHandleSecondary(handle, _dependent); + HELPER_METHOD_FRAME_BEGIN_0(); + + DestroyDependentHandle(handle); + + HELPER_METHOD_FRAME_END(); } FCIMPLEND diff --git a/src/coreclr/vm/comdependenthandle.h b/src/coreclr/vm/comdependenthandle.h index 5c7e055bd65531..827e2ff7b69e29 100644 --- a/src/coreclr/vm/comdependenthandle.h +++ b/src/coreclr/vm/comdependenthandle.h @@ -44,9 +44,9 @@ 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, InternalFree, OBJECTHANDLE handle); - static FCDECL2(VOID, InternalSetTarget, OBJECTHANDLE handle, Object *target); static FCDECL2(VOID, InternalSetDependent, OBJECTHANDLE handle, Object *dependent); + static FCDECL1(VOID, InternalStopTracking, OBJECTHANDLE handle); + static FCDECL1(VOID, InternalFree, OBJECTHANDLE handle); }; #endif diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 04831c35b43c47..488836247f7cdf 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -65,9 +65,9 @@ FCFuncStart(gDependentHandleFuncs) FCFuncElement("InternalGetTarget", DependentHandle::InternalGetTarget) FCFuncElement("InternalGetDependent", DependentHandle::InternalGetDependent) FCFuncElement("InternalGetTargetAndDependent", DependentHandle::InternalGetTargetAndDependent) - FCFuncElement("InternalFree", DependentHandle::InternalFree) - FCFuncElement("InternalSetTarget", DependentHandle::InternalSetTarget) FCFuncElement("InternalSetDependent", DependentHandle::InternalSetDependent) + FCFuncElement("InternalStopTracking", DependentHandle::InternalStopTracking) + FCFuncElement("InternalFree", DependentHandle::InternalFree) FCFuncEnd() From d7146e0a0b85cfb0bbbc5496d7ce25df9ceaab59 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Mon, 21 Jun 2021 16:51:28 +0200 Subject: [PATCH 28/30] Remove FCUnique from InternalStopTracking This was added in https://github.com/dotnet/runtime/pull/39810 to avoid a collision with MarshalNative::GCHandleInternalSet, as the two FCalls had identical implementations and their entry points were not unique. This should no longer be needed after 099fc478551f46cc54e7a18a32d9a9ac73727c73, as that changed both the signature and the implementation of this FCall. --- src/coreclr/vm/comdependenthandle.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/coreclr/vm/comdependenthandle.cpp b/src/coreclr/vm/comdependenthandle.cpp index c2be2258be52da..1f59149920ddc8 100644 --- a/src/coreclr/vm/comdependenthandle.cpp +++ b/src/coreclr/vm/comdependenthandle.cpp @@ -92,9 +92,6 @@ FCIMPL1(VOID, DependentHandle::InternalStopTracking, OBJECTHANDLE handle) _ASSERTE(handle != NULL); - // Avoid collision with MarshalNative::GCHandleInternalSet - FCUnique(0x12); - IGCHandleManager *mgr = GCHandleUtilities::GetGCHandleManager(); mgr->StoreObjectInHandle(handle, NULL); } From c9c632552ccfb9c9322435735bbe8a029321d27e Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 22 Jun 2021 19:03:43 +0200 Subject: [PATCH 29/30] 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 8ae58eba738202..8c78252af2c23d 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 1f59149920ddc8..b2221908102338 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 827e2ff7b69e29..06786e62577466 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 488836247f7cdf..8ff8a4a5cc3d0d 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 35acec95469881..88b8187f3dfd22 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 14e91b725e4ada..580f9955b65db4 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 94c2afed33bf9e..2ee4eca51d48f2 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 a0a7e26c72b3dc..8cfd9b013e52c5 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() From c463d54a640ad20e5e23859c2a04ca59757b8ffe Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 23 Jun 2021 10:35:59 +0200 Subject: [PATCH 30/30] Update DependentHandle XML docs --- .../src/System/Runtime/DependentHandle.cs | 36 +++++++++++-------- 1 file changed, 22 insertions(+), 14 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 8c78252af2c23d..f080ba28dda621 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs @@ -9,20 +9,25 @@ namespace System.Runtime { /// - /// Represents a dependent GC handle, which will conditionally keep a dependent object instance alive - /// as long as a target object instance is alive as well, without representing a strong reference to the - /// target object instance. That is, a value with a given object instance as - /// target will not cause the target to be kept alive if there are no other strong references to it, but - /// it will do so for the dependent object instance as long as the target is alive. - /// - /// This type is conceptually equivalent to having a weak reference to a given target object instance A, with - /// that object having a field or property (or some other strong reference) to a dependent object instance B. - /// + /// Represents a dependent GC handle, which will conditionally keep a dependent object instance alive as long as + /// a target object instance is alive as well, without representing a strong reference to the target instance. /// /// + /// A value with a given object instance as target will not cause the target + /// to be kept alive if there are no other strong references to it, but it will do so for the dependent + /// object instance as long as the target is alive. + /// + /// Using this type is conceptually equivalent to having a weak reference to a given target object instance A, + /// with that object having a field or property (or some other strong reference) to a dependent object instance B. + /// + /// /// The type is not thread-safe, and consumers are responsible for ensuring that /// is not called concurrently with other APIs. Not doing so results in undefined behavior. - /// The and properties are instead thread-safe. + /// + /// + /// The , , and + /// properties are instead thread-safe, and safe to use if is not concurrently invoked as well. + /// /// public struct DependentHandle : IDisposable { @@ -60,13 +65,15 @@ public struct DependentHandle : IDisposable /// The dependent object instance to associate with . public DependentHandle(object? target, object? dependent) { - // no need to check for null result: nInitialize expected to throw OOM. + // no need to check for null result: InternalInitialize expected to throw OOM. _handle = InternalInitialize(target, dependent); } /// - /// Gets a value indicating whether this handle has been allocated or not. + /// Gets a value indicating whether this instance was constructed with + /// and has not yet been disposed. /// + /// This property is thread-safe. public bool IsAllocated => (nint)_handle != 0; /// @@ -144,11 +151,12 @@ public object? Dependent /// 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 - /// could be possible to sometimes successfully retrieve the previous target, but then fail to get the dependent. + /// If and were used sequentially in this scenario instead, it + /// would be possible to sometimes successfully retrieve the previous target, but then fail to get the dependent. /// /// The values of and . /// Thrown if is . + /// This property is thread-safe. public (object? Target, object? Dependent) TargetAndDependent { get