Implement 'ConditionalWeakTable<TKey,TValue>.GetOrAdd' APIs

[Sergio0694]

Closes #89002

This PR implements the new APIs for ConditionalWeakTable<TKey, TValue> and hides some legacy ones:

namespace System.Runtime.CompilerServices;

public sealed partial class ConditionalWeakTable<TKey, TValue>
{
+   public TValue GetOrAdd(TKey key, TValue value);
+   public TValue GetOrAdd(TKey key, Func<TKey, TValue> valueFactory);
+   public TValue GetOrAdd<TArg>(TKey key, Func<TKey, TArg, TValue> valueFactory, TArg factoryArgument)
+       where TArg : allows ref struct;
    
+   [EditorBrowsable(EditorBrowsableState.Never)]
    public TValue GetValue(TKey key, CreateValueCallback createValueCallback);
  
+   [EditorBrowsable(EditorBrowsableState.Never)]
    public TValue GetOrCreateValue(TKey key);

+   [EditorBrowsable(EditorBrowsableState.Never)]
    public delegate TValue CreateValueCallback(TKey key);
}

[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, 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.

[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, 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.

[Copilot]

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• src/libraries/System.Runtime/ref/System.Runtime.cs: Evaluated as low risk

[jkotas]

Go over Corelib and maybe other core assemblies and switch them to use the new API where appropriate? There are not that many candidates.

[Sergio0694]

I've gone over uses I could find of the now hidden methods and replaced them. Particularly happy with how ComWrappers on Native AOT looks now, it could drop a couple of extra lookups entirely. @jkoritzinsky can you help take a look? 🙂

[Sergio0694]

From rt-sz, after swapping the value typle to a local type (ebbd878):

Project	Delta before	Delta after
winrt-component-full-windows	3072	1024
winrt-component-minimal-windows	5632	1024

Saved 2 KB and 4.5 KB. I'll take it 😄

[jkoritzinsky]

Okay that's a lot more savings than I was expecting. Yeah using a different type is worth it.

[Sergio0694]

Yeah that's quite a bit, and it must all just be extra value tuple cruft that's unnecessarily kept, given that using a custom type still makes you pay for the generic instantiation. Unrelated to this PR, but @MichalStrehovsky this makes me wonder whether perhaps there could be something general that ILC could do in cases like these to minimize the size impact? 🤔

[MichalStrehovsky]

Yeah that's quite a bit, and it must all just be extra value tuple cruft that's unnecessarily kept, given that using a custom type still makes you pay for the generic instantiation. Unrelated to this PR, but @MichalStrehovsky this makes me wonder whether perhaps there could be something general that ILC could do in cases like these to minimize the size impact? 🤔

I think this is the same thing as here: #111544 (comment)

We're using ValueTuple in a signature of a delegate. The compiler considers Invoke method on delegate implicitly reflected on. Reflected on methods need extra data structures for parameter types so that we can invoke them. ValueTuple is therefore a target of reflection/MakeGenericType.

[Sergio0694]

"The compiler considers Invoke method on delegate implicitly reflected on."

Wait, I thought we (you) had fixed that already a while back? I remember you made some change and we saw a whole lot of savings in CsWinRT thanks to that. IIRC you said delegate targets would only be considered reflection visible if someone were doing .Method on a delegate somewhere, which I assume should not be the case in any of those samples. Am I mixing things up? 😅

[MichalStrehovsky]

Wait, I thought we (you) had fixed that already a while back? I remember you made some change and we saw a whole lot of savings in CsWinRT thanks to that. IIRC you said delegate targets would only be considered reflection visible if someone were doing .Method on a delegate somewhere, which I assume should not be the case in any of those samples. Am I mixing things up? 😅

The thing I fixed is "do not indiscriminately mark targets of delegates as reflected on" (we were doing that because we didn't know if anyone accessed Delegate.Method property - we now track that).

The thing that is considered reflected on in the case discussed here is the Invoke method of the delegate itself, not the method the delegate points to.

[stephentoub]

Thanks