Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using Internal.Runtime.CompilerServices;

namespace System.Runtime.InteropServices
{
Expand All @@ -18,6 +19,25 @@ public static class CollectionsMarshal
public static Span<T> AsSpan<T>(List<T>? list)
=> list is null ? default : new Span<T>(list._items, 0, list._size);

/// <summary>
/// Gets a ref to a <typeparamref name="TValue"/> in the <see cref="Dictionary{TKey, TValue}"/>.
/// </summary>
/// <param name="dictionary">The dictionary to get the ref to <typeparamref name="TValue"/> from.</param>
/// <param name="key">The key used for lookup.</param>
/// <remarks>Items should not be added or removed from the <see cref="Dictionary{TKey, TValue}"/> while the ref <typeparamref name="TValue"/> is in use.</remarks>
/// <exception cref="KeyNotFoundException">Thrown when <paramref name="key"/> does not exist in the <paramref name="dictionary"/>.</exception>
public static ref TValue GetValueRef<TKey, TValue>(Dictionary<TKey, TValue> dictionary, TKey key) where TKey : notnull
{
ref TValue valueRef = ref dictionary.FindValue(key);

if (Unsafe.IsNullRef(ref valueRef))
{
ThrowHelper.ThrowKeyNotFoundException(key);
}

return ref valueRef;
}
Copy link
Member

@stephentoub stephentoub Jun 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of the approved API looks good. Thanks!

That said, I still question who's actually going to use this method. If you care about this level of performance optimization, in what situation are you going to want it to throw an exception if the key can't be found? i.e. why wouldn't you use GetValueRefOrNullRef?

Are there any places in all of dotnet/runtime where a) the performance benefits of this method will be meaningful and b) we want the exception behavior when the key isn't found?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"[...] I still question who's actually going to use this method. If you care about this level of performance optimization, in what situation are you going to want it to throw an exception if the key can't be found? i.e. why wouldn't you use GetValueRefOrNullRef?"

I can agree this is likely the least useful API of the 3 approved in this group, yeah. I'm thinking a valid use case could be the same as GetValueRefOrNullRef (as in, where you'd use this method for the same reasons why you'd use GetValueRefOrNullRef over just the standard indexer or ContainsKey/TryGetValue + indexer), except in cases where you never expect the key not to be present in the dictionary at that point, so you'd just use this one to avoid doing the check on your own? The two would be functionally equivalent, yeah, with the only difference just being that this would make the user code less verbose in this scenario 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Can you find a real piece of code somewhere where this would make a meaningful difference? It would need to be something where only part of the fetched value is needed, the overhead of copying the whole value unnecessarily is measurable, an exception on failure is desired to the point where on this hot path the extra check required for the implementation to throw on null is required, a not found exception rather than a null reference exception if not found is important, etc. I'm struggling to think of any situation where I'd want to use this method over one of the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll admit I haven't been able to find applicable code snippets myself (looked in System.Private.CoreLib, ImageSharp, and tried asking on Discord), so at this time I'm not able to provide much evidence to argue for this API other than the general use case scenario support that @benaadams already mentioned in the original proposal in #27062. As mentioned, I can definitely agree that this is the least useful API of the three as it doesn't offer new functionality per se but it's more like a convenience wrapper (personally I mostly just care about #54611 making it to .NET 6 😄).

Let me know how/if you'd like to proceed with this, I guess we could either just merge it given that it's a very minor change anyway and the API itself has already been approved by FXDC, or we could just close this for now 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference is we close this PR and "unapprove" the API; we shouldn't add it just because we can... new surface area needs good justification. @dotnet/area-system-collections can make a call, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub you are saying unapprove only GetValueRef which throws, correct?

I'd agree that this is mostly just a "convenience" API so users don't need to deal with NullRef and handle the logic themselves, it doesn't represent anything that they can't do themselves. Given the other considerations around the CollectionMarshal APIs, forcing users to understand NullRef also seems the least of the worries 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are saying unapprove only GetValueRef which throws, correct?

Correct

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd personally be fine with ditching the throwing variant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go ahead and close this then since we all agree this API in particular is not strictly necessary.
There's only #54611 left to review and merge then 😄


/// <summary>
/// Gets either a ref to a <typeparamref name="TValue"/> in the <see cref="Dictionary{TKey, TValue}"/> or a ref null if it does not exist in the <paramref name="dictionary"/>.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ public CoClassAttribute(System.Type coClass) { }
public static partial class CollectionsMarshal
{
public static System.Span<T> AsSpan<T>(System.Collections.Generic.List<T>? list) { throw null; }
public static ref TValue GetValueRef<TKey, TValue>(System.Collections.Generic.Dictionary<TKey, TValue> dictionary, TKey key) where TKey : notnull { throw null; }
public static ref TValue GetValueRefOrNullRef<TKey, TValue>(System.Collections.Generic.Dictionary<TKey, TValue> dictionary, TKey key) where TKey : notnull { throw null; }
}
[System.AttributeUsageAttribute(System.AttributeTargets.Field | System.AttributeTargets.Parameter | System.AttributeTargets.Property | System.AttributeTargets.ReturnValue, Inherited=false)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,159 @@ public void ListAsSpanLinkBreaksOnResize()
}
}

[Fact]
public void GetValueRefValueType()
{
var dict = new Dictionary<int, Struct>
{
{ 1, default },
{ 2, default }
};

Assert.Equal(2, dict.Count);

Assert.Equal(0, dict[1].Value);
Assert.Equal(0, dict[1].Property);

var itemVal = dict[1];
itemVal.Value = 1;
itemVal.Property = 2;

// Does not change values in dictionary
Assert.Equal(0, dict[1].Value);
Assert.Equal(0, dict[1].Property);

CollectionsMarshal.GetValueRef(dict, 1).Value = 3;
CollectionsMarshal.GetValueRef(dict, 1).Property = 4;

Assert.Equal(3, dict[1].Value);
Assert.Equal(4, dict[1].Property);

ref var itemRef = ref CollectionsMarshal.GetValueRef(dict, 2);

Assert.Equal(0, itemRef.Value);
Assert.Equal(0, itemRef.Property);

itemRef.Value = 5;
itemRef.Property = 6;

Assert.Equal(5, itemRef.Value);
Assert.Equal(6, itemRef.Property);
Assert.Equal(dict[2].Value, itemRef.Value);
Assert.Equal(dict[2].Property, itemRef.Property);

itemRef = new() { Value = 7, Property = 8 };

Assert.Equal(7, itemRef.Value);
Assert.Equal(8, itemRef.Property);
Assert.Equal(dict[2].Value, itemRef.Value);
Assert.Equal(dict[2].Property, itemRef.Property);

// Check for exception

Assert.Throws<KeyNotFoundException>(() => CollectionsMarshal.GetValueRef(dict, 3));

Assert.Equal(2, dict.Count);
}

[Fact]
public void GetValueRefClass()
{
var dict = new Dictionary<int, IntAsObject>
{
{ 1, new() },
{ 2, new() }
};

Assert.Equal(2, dict.Count);

Assert.Equal(0, dict[1].Value);
Assert.Equal(0, dict[1].Property);

var itemVal = dict[1];
itemVal.Value = 1;
itemVal.Property = 2;

// Does change values in dictionary
Assert.Equal(1, dict[1].Value);
Assert.Equal(2, dict[1].Property);

CollectionsMarshal.GetValueRef(dict, 1).Value = 3;
CollectionsMarshal.GetValueRef(dict, 1).Property = 4;

Assert.Equal(3, dict[1].Value);
Assert.Equal(4, dict[1].Property);

ref var itemRef = ref CollectionsMarshal.GetValueRef(dict, 2);

Assert.Equal(0, itemRef.Value);
Assert.Equal(0, itemRef.Property);

itemRef.Value = 5;
itemRef.Property = 6;

Assert.Equal(5, itemRef.Value);
Assert.Equal(6, itemRef.Property);
Assert.Equal(dict[2].Value, itemRef.Value);
Assert.Equal(dict[2].Property, itemRef.Property);

itemRef = new() { Value = 7, Property = 8 };

Assert.Equal(7, itemRef.Value);
Assert.Equal(8, itemRef.Property);
Assert.Equal(dict[2].Value, itemRef.Value);
Assert.Equal(dict[2].Property, itemRef.Property);

// Check for exception

Assert.Throws<KeyNotFoundException>(() => CollectionsMarshal.GetValueRef(dict, 3));

Assert.Equal(2, dict.Count);
}

[Fact]
public void GetValueRefLinkBreaksOnResize()
{
var dict = new Dictionary<int, Struct>
{
{ 1, new() }
};

Assert.Equal(1, dict.Count);

ref var itemRef = ref CollectionsMarshal.GetValueRef(dict, 1);

Assert.Equal(0, itemRef.Value);
Assert.Equal(0, itemRef.Property);

itemRef.Value = 1;
itemRef.Property = 2;

Assert.Equal(1, itemRef.Value);
Assert.Equal(2, itemRef.Property);
Assert.Equal(dict[1].Value, itemRef.Value);
Assert.Equal(dict[1].Property, itemRef.Property);

// Resize
dict.EnsureCapacity(100);
for (int i = 2; i <= 50; i++)
{
dict.Add(i, new());
}

itemRef.Value = 3;
itemRef.Property = 4;

Assert.Equal(3, itemRef.Value);
Assert.Equal(4, itemRef.Property);

// Check connection broken
Assert.NotEqual(dict[1].Value, itemRef.Value);
Assert.NotEqual(dict[1].Property, itemRef.Property);

Assert.Equal(50, dict.Count);
}

[Fact]
public void GetValueRefOrNullRefValueType()
{
Expand Down