Add CollectionsMarshal.GetValueRef API

[Sergio0694]

Contributes to #27062

namespace System.Runtime.InteropServices
{
    public static class CollectionsMarshal
    {
        public static ref TValue GetValueRef<TKey, TValue>(Dictionary<TKey, TValue> dictionary, TKey key)
            where TKey : notnull;
    }
}

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @eiriktsarpalis
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Contributes to #27062

namespace System.Runtime.InteropServices
{
    public static class CollectionsMarshal
    {
        public static ref TValue GetValueRef<TKey, TValue>(Dictionary<TKey, TValue> dictionary, TKey key)
            where TKey : notnull;
    }
}

Author:	Sergio0694
Assignees:	-
Labels:	area-System.Collections, area-System.Runtime.InteropServices, new-api-needs-documentation
Milestone:	-

[stephentoub]

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?

[Sergio0694]

"[...] 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 🤔

[stephentoub]

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.

[Sergio0694]

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 🙂

[stephentoub]

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.

[tannergooding]

@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 😄

[stephentoub]

you are saying unapprove only GetValueRef which throws, correct?

Correct

[eiriktsarpalis]

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

[Sergio0694]

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 😄