From 86011574b1726d63298ce6c34053b0d9625db7c0 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 6 Feb 2025 22:52:32 -0800 Subject: [PATCH 1/5] Add 'CWT<,>.Remove' overload --- .../CompilerServices/ConditionalWeakTable.cs | 48 ++++++++++++++++--- .../System.Runtime/ref/System.Runtime.cs | 1 + 2 files changed, 43 insertions(+), 6 deletions(-) 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 2318211cb7e9f3..8114234d920736 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 @@ -48,6 +48,7 @@ public ConditionalWeakTable() /// If the key is not found, contains default(TValue). /// /// Returns "true" if key was found, "false" otherwise. + /// is . /// /// The key may get garbage collected during the TryGetValue operation. If so, TryGetValue /// may at its discretion, return "false" and set "value" to the default (as if the key was not present.) @@ -65,6 +66,7 @@ public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) /// Adds a key to the table. /// key to add. May not be null. /// value to associate with key. + /// is . /// /// If the key is already entered into the dictionary, this method throws an exception. /// The key may get garbage collected during the Add() operation. If so, Add() @@ -94,6 +96,7 @@ public void Add(TKey key, TValue value) /// The key to add. /// The key's property value. /// true if the key/value pair was added; false if the table already contained the key. + /// is . public bool TryAdd(TKey key, TValue value) { if (key is null) @@ -117,6 +120,7 @@ public bool TryAdd(TKey key, TValue value) /// Adds the key and value if the key doesn't exist, or updates the existing key's value if it does exist. /// key to add or update. May not be null. /// value to associate with key. + /// is . public void AddOrUpdate(TKey key, TValue value) { if (key is null) @@ -143,10 +147,11 @@ public void AddOrUpdate(TKey key, TValue value) /// Removes a key and its value from the table. /// key to remove. May not be null. /// true if the key is found and removed. Returns false if the key was not in the dictionary. + /// is . /// - /// The key may get garbage collected during the Remove() operation. If so, - /// Remove() will not fail or throw, however, the return value can be either true or false - /// depending on who wins the race. + /// The key may get garbage collected during the operation. If so, + /// will not fail or throw, however, the return value can be either + /// true or false depending on who wins the race. /// public bool Remove(TKey key) { @@ -157,7 +162,38 @@ public bool Remove(TKey key) lock (_lock) { - return _container.Remove(key); + return _container.Remove(key, out _); + } + } + + /// Removes a key and its value from the table, and returns the removed value if it was present. + /// key to remove. May not be null. + /// value removed from the table, if it was present. + /// true if the key is found and removed. Returns false if the key was not in the dictionary. + /// is . + /// + /// The key may get garbage collected during the operation. If so, + /// will not fail or throw, however, the return value can be either + /// true or false depending on who wins the race. + /// + public bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) + { + if (key is null) + { + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key); + } + + lock (_lock) + { + bool found = _container.Remove(key, out object? valueObject); + + // We can safely suppress the nullability warning here, because if we did find the key, + // the retrieved value is guaranteed to not be null. The 'Remove' method on the container + // doesn't have that annotation just because it's looking for an index, not a boolean, + // and there's no way to express something like '[MaybeNullWhen(!= 1)]'. + value = Unsafe.As(valueObject!); + + return found; } } @@ -692,11 +728,11 @@ internal void RemoveAllKeys() } /// Removes the specified key from the table, if it exists. - internal bool Remove(TKey key) + internal bool Remove(TKey key, out object? value) { VerifyIntegrity(); - int entryIndex = FindEntry(key, out _); + int entryIndex = FindEntry(key, out value); if (entryIndex != -1) { RemoveIndex(entryIndex); diff --git a/src/libraries/System.Runtime/ref/System.Runtime.cs b/src/libraries/System.Runtime/ref/System.Runtime.cs index a2105c7843e8ea..90a31dda704bda 100644 --- a/src/libraries/System.Runtime/ref/System.Runtime.cs +++ b/src/libraries/System.Runtime/ref/System.Runtime.cs @@ -13284,6 +13284,7 @@ public void Clear() { } [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)] public TValue GetValue(TKey key, System.Runtime.CompilerServices.ConditionalWeakTable.CreateValueCallback createValueCallback) { throw null; } public bool Remove(TKey key) { throw null; } + public bool Remove(TKey key, [System.Diagnostics.CodeAnalysis.MaybeNullWhenAttribute(false)] out TValue value) { throw null; } System.Collections.Generic.IEnumerator> System.Collections.Generic.IEnumerable>.GetEnumerator() { throw null; } System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() { throw null; } public bool TryAdd(TKey key, TValue value) { throw null; } From 35b0c5b78e515523ebfe93aeba6fbd96ce08c7cc Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 6 Feb 2025 22:58:45 -0800 Subject: [PATCH 2/5] Add unit tests --- .../ConditionalWeakTableTests.cs | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Runtime/CompilerServices/ConditionalWeakTableTests.cs b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Runtime/CompilerServices/ConditionalWeakTableTests.cs index c4f70e4ab37687..06487e90aec3e8 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Runtime/CompilerServices/ConditionalWeakTableTests.cs +++ b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Runtime/CompilerServices/ConditionalWeakTableTests.cs @@ -22,6 +22,7 @@ public static void InvalidArgs_Throws() AssertExtensions.Throws("key", () => cwt.Add(null, new object())); // null key AssertExtensions.Throws("key", () => cwt.TryGetValue(null, out ignored)); // null key AssertExtensions.Throws("key", () => cwt.Remove(null)); // null key + AssertExtensions.Throws("key", () => cwt.Remove(null, out _)); // null key AssertExtensions.Throws("key", () => cwt.GetOrAdd(null, new object())); // null key AssertExtensions.Throws("key", () => cwt.GetOrAdd(null, k => new object())); // null key AssertExtensions.Throws("key", () => cwt.GetOrAdd(null, (k, a) => new object(), 42)); // null key @@ -171,6 +172,39 @@ public static void AddMany_ThenRemoveAll(int numObjects) } } + [Theory] + [InlineData(1)] + [InlineData(100)] + public static void AddMany_ThenRemoveAll_ValidateRemovedValue(int numObjects) + { + object[] keys = Enumerable.Range(0, numObjects).Select(_ => new object()).ToArray(); + object[] values = Enumerable.Range(0, numObjects).Select(_ => new object()).ToArray(); + var cwt = new ConditionalWeakTable(); + + for (int i = 0; i < numObjects; i++) + { + cwt.Add(keys[i], values[i]); + } + + for (int i = 0; i < numObjects; i++) + { + Assert.Same(values[i], cwt.GetValue(keys[i], _ => new object())); + } + + for (int i = 0; i < numObjects; i++) + { + Assert.True(cwt.Remove(keys[i], out var value)); + Assert.False(cwt.Remove(keys[i], out _)); + Assert.Same(values[i], value); + } + + for (int i = 0; i < numObjects; i++) + { + object ignored; + Assert.False(cwt.TryGetValue(keys[i], out ignored)); + } + } + [Theory] [InlineData(100)] public static void AddRemoveIteratively(int numObjects) @@ -188,6 +222,24 @@ public static void AddRemoveIteratively(int numObjects) } } + [Theory] + [InlineData(100)] + public static void AddRemoveIteratively_ValidateRemovedValue(int numObjects) + { + object[] keys = Enumerable.Range(0, numObjects).Select(_ => new object()).ToArray(); + object[] values = Enumerable.Range(0, numObjects).Select(_ => new object()).ToArray(); + var cwt = new ConditionalWeakTable(); + + for (int i = 0; i < numObjects; i++) + { + cwt.Add(keys[i], values[i]); + Assert.Same(values[i], cwt.GetValue(keys[i], _ => new object())); + Assert.True(cwt.Remove(keys[i], out var value)); + Assert.False(cwt.Remove(keys[i], out _)); + Assert.Same(values[i], value); + } + } + [Fact] public static void Concurrent_AddMany_DropReferences() // no asserts, just making nothing throws { @@ -218,6 +270,26 @@ public static void Concurrent_Add_Read_Remove_DifferentObjects() }); } + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] + public static void Concurrent_Add_Read_Remove_ValidateRemovedValue_DifferentObjects() + { + var cwt = new ConditionalWeakTable(); + DateTime end = DateTime.UtcNow + TimeSpan.FromSeconds(0.25); + Parallel.For(0, Environment.ProcessorCount, i => + { + while (DateTime.UtcNow < end) + { + object key = new object(); + object value = new object(); + cwt.Add(key, value); + Assert.Same(value, cwt.GetValue(key, _ => new object())); + Assert.True(cwt.Remove(key, out var removedValue)); + Assert.False(cwt.Remove(key, out _)); + Assert.Same(value, removedValue); + } + }); + } + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] public static void Concurrent_GetOrAdd_Add_Read_Remove_DifferentObjects() { From 0cf3b25a66cd520d56bf95441ef3fb40305308ea Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Fri, 7 Feb 2025 10:05:54 -0800 Subject: [PATCH 3/5] Tweak XML docs --- .../Runtime/CompilerServices/ConditionalWeakTable.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 8114234d920736..9f12f92e6ebbcb 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 @@ -145,8 +145,8 @@ public void AddOrUpdate(TKey key, TValue value) } /// Removes a key and its value from the table. - /// key to remove. May not be null. - /// true if the key is found and removed. Returns false if the key was not in the dictionary. + /// The key to remove. + /// if the key is found and removed; otherwise, . /// is . /// /// The key may get garbage collected during the operation. If so, @@ -167,9 +167,9 @@ public bool Remove(TKey key) } /// Removes a key and its value from the table, and returns the removed value if it was present. - /// key to remove. May not be null. + /// The key to remove. /// value removed from the table, if it was present. - /// true if the key is found and removed. Returns false if the key was not in the dictionary. + /// if the key is found and removed; otherwise, . /// is . /// /// The key may get garbage collected during the operation. If so, From f9cddc6e7969886578d0f9e816b785c3d12a39f6 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Fri, 7 Feb 2025 10:08:33 -0800 Subject: [PATCH 4/5] Tweak 'Remove' internal helper --- .../CompilerServices/ConditionalWeakTable.cs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) 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 9f12f92e6ebbcb..eb1cb4b079fa4e 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 @@ -185,15 +185,7 @@ public bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) lock (_lock) { - bool found = _container.Remove(key, out object? valueObject); - - // We can safely suppress the nullability warning here, because if we did find the key, - // the retrieved value is guaranteed to not be null. The 'Remove' method on the container - // doesn't have that annotation just because it's looking for an index, not a boolean, - // and there's no way to express something like '[MaybeNullWhen(!= 1)]'. - value = Unsafe.As(valueObject!); - - return found; + return _container.Remove(key, out value); } } @@ -728,17 +720,19 @@ internal void RemoveAllKeys() } /// Removes the specified key from the table, if it exists. - internal bool Remove(TKey key, out object? value) + internal bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) { VerifyIntegrity(); - int entryIndex = FindEntry(key, out value); + int entryIndex = FindEntry(key, out object? valueObject); if (entryIndex != -1) { RemoveIndex(entryIndex); + value = Unsafe.As(valueObject); return true; } + value = null; return false; } From 7cad836647bb3e02b5e7efa2b6f5c91a0f9963e6 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 12 Feb 2025 22:21:53 -0800 Subject: [PATCH 5/5] Remove unclear remarks --- .../CompilerServices/ConditionalWeakTable.cs | 21 +------------------ 1 file changed, 1 insertion(+), 20 deletions(-) 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 eb1cb4b079fa4e..eb6af90b5a5043 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 @@ -49,10 +49,6 @@ public ConditionalWeakTable() /// /// Returns "true" if key was found, "false" otherwise. /// is . - /// - /// The key may get garbage collected during the TryGetValue operation. If so, TryGetValue - /// may at its discretion, return "false" and set "value" to the default (as if the key was not present.) - /// public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) { if (key is null) @@ -67,12 +63,7 @@ public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) /// key to add. May not be null. /// value to associate with key. /// is . - /// - /// If the key is already entered into the dictionary, this method throws an exception. - /// The key may get garbage collected during the Add() operation. If so, Add() - /// has the right to consider any prior entries successfully removed and add a new entry without - /// throwing an exception. - /// + /// is already entered into the dictionary. public void Add(TKey key, TValue value) { if (key is null) @@ -148,11 +139,6 @@ public void AddOrUpdate(TKey key, TValue value) /// The key to remove. /// if the key is found and removed; otherwise, . /// is . - /// - /// The key may get garbage collected during the operation. If so, - /// will not fail or throw, however, the return value can be either - /// true or false depending on who wins the race. - /// public bool Remove(TKey key) { if (key is null) @@ -171,11 +157,6 @@ public bool Remove(TKey key) /// value removed from the table, if it was present. /// if the key is found and removed; otherwise, . /// is . - /// - /// The key may get garbage collected during the operation. If so, - /// will not fail or throw, however, the return value can be either - /// true or false depending on who wins the race. - /// public bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) { if (key is null)