Add CollectionsMarshal.GetValueRefOrAddDefault

[Sergio0694]

Contributes to #27062

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

NOTE: I've marked the returned TValue as an uncostrained nullable value, because if TValue is a reference type the returned reference will point to null. Value types are not affected by this, ie. there is no Nullable<T> involved here at all.

[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 ref TValue? GetValueRefOrAddDefault<TKey, TValue>(Dictionary<TKey, TValue> dictionary, TKey key, out bool exists)
            where TKey : notnull;
    }
}

NOTE: I've marked the returned TValue as an uncostrained nullable value, because if TValue is a reference type the returned reference will point to null. Value types are not affected by this, ie. there is no Nullable<T> involved here at all.

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

[Sergio0694]

@jkotas Could you clarify what the overhead of this new method would be, and whether it would be a blocker for this PR? This would be a very neat feature to have, and the API itself has been approved by API review too - if it's just the current implementation that's an issue would any of the proposed approaches help? As in, moving the code to a diferent spot, etc.

Thanks! 🙂

[jkotas]

It makes every Dictionary instantiation more expensive, in particular with AOT.

[Sergio0694]

I see. If the issue is specifically with the overhead of dictionary instantiations, would moving the code to a separate type (eg. a nested type within dictionary) solve that? That would only ever be loaded when the method is actually used, right? 🤔

[jkotas]

would moving the code to a separate type (eg. a nested type within dictionary) solve that

Yes, it would solve that.

[Sergio0694]

Test failures should be unrelated, and the changes in 99523ba addresses Jan's concerns on additional overhead being added to Dictionary<TKey, TValue> instantiations (moved all the new code to a separate type). Should be good to review now 😄

[GrabYourPitchforks]

@Sergio0694 Thanks for this! Can you also add a unit test for the randomized hash logic? I think the appropriate place to add this would be in the file https://github.com/dotnet/runtime/blob/main/src/libraries/System.Collections/tests/Generic/Dictionary/HashCollisionScenarios/OutOfBoundsRegression.cs.

There's no need to modify RunDictionaryTest (line 93 of that file) to run a CollectionsMarshal test for every possible combination; that's probably overkill. Instead, what you can do is immediately before line 93, insert a manual call that looks something like:

RunCollectionTestCommon(
    () => new Dictionary<string, object>(StringComparison.Ordinal),
    (dictionary, key) => CollectionsMarshal.GetRef(dictionary, key, out _) = null,
    (dictionary, key) => dictionary.ContainsKey(key),
    dictionary => dictionary.Comparer,
    expectedInternalComparerTypeBeforeCollisionThreshold: nonRandomizedOrdinalComparerType,
    expectedPublicComparerBeforeCollisionThreshold: StringComparer.Ordinal,
    expectedInternalComparerTypeAfterCollisionThreshold: randomizedOrdinalComparerType);

In a nutshell, this utilizes the existing test infrastructure to generate a whole bunch of collisions, but the delegate you provide will force it to use your new CollectionsMarshal method instead of Add. This will get regression test coverage for string collision scenarios in this method.

[eiriktsarpalis]

Other than minor feedback, LGTM. Thanks!

[GrabYourPitchforks]

A reminder for the future - try to avoid rebase & force-push after reviews have started. Makes it a little more complicated for us since we can't rely on the GitHub web UI and instead we need to generate incremental diffs manually.

[Sergio0694]

Oh, I'm sorry, I didn't realize that. Will definitely avoid doing that again in the future! 😅

[GrabYourPitchforks]

Thanks so much! :D