Add missing readonly modifiers to GCHandle and DependentHandle

[Sergio0694]

This PR sprinkles a bunch of missing readonly modifiers to APIs of GCHandle and DependentHandle that are not actually modifying instance state. This allows Roslyn to skip some unnecessary defensive copies when invoking any of these APIs over values stored in either readonly fields, or from members marked as readonly (from mutable struct types).

[jkotas]

values stored in either readonly fields

Storing GCHandles in readonly fields is a pit of failure. Consider:

using System.Runtime.InteropServices;

var t = new Test();
t.Dispose();

class Test
{
    readonly GCHandle _handle = GCHandle.Alloc(null);

    public void Dispose()
    {
         Console.WriteLine(_handle.IsAllocated);

         _handle.Free();

         // This prints `true` even though the handle was just freed!
         Console.WriteLine(_handle.IsAllocated);
    }
}

This change does not hurt, but I would avoid storing the GCHandles in readonly fields in the first place.

[ghost]

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This PR sprinkles a bunch of missing readonly modifiers to APIs of GCHandle and DependentHandle that are not actually modifying instance state. This allows Roslyn to skip some unnecessary defensive copies when invoking any of these APIs over values stored in either readonly fields, or from members marked as readonly (from mutable struct types).

Author:	Sergio0694
Assignees:	-
Labels:	area-System.Runtime.InteropServices, community-contribution, needs-area-label
Milestone:	-

[Sergio0694]

@jkotas I agree that's not a good idea, the real scenario I was thinking about (and the reason why I made this PR) was more about allowing instance methods touching GC handles stored on mutable fields to be marked as readonly without causing defensive copies (eg. if they're just accessing the target of the handle). As a side note I still wish there was some way (or an analyzer perhaps) to spot defensive copies. It's so easy to accidentally invoke a non readonly member on a value that triggers a defensive copy for one reason or another 🥲

[stephentoub]

Can you highlight any places where a) this avoids defensive copies, b) those copies involve additional cost, and c) that cost is measurable in any meaningful way?

[Sergio0694]

Replying here, I'm responding to a bunch of comments from several threads above as well as emails.

Chatted with @AaronRobinsonMSFT offline, and I've updated this PR to only add readonly to getters, not setters.

"Can you highlight any places where a) this avoids defensive copies, b) those copies involve additional cost, and c) that cost is measurable in any meaningful way?"

I can't, no. This is more of a "conceptual" change (ie. adding readonly here just seems correct on its own, and I'm not proposing the change specifically to fix some performance issue I noticed). At least in simple tests, the JIT seems to produce equivalent codegen. However, the IL is certainly more bloated without readonly (ie. it has one extra local and whatnot), so this still just seems to "make things simpler" for the JIT, like @tannergooding also mentioned.

[stephentoub]

"Can you highlight any places where a) this avoids defensive copies, b) those copies involve additional cost, and c) that cost is measurable in any meaningful way?"

I can't, no. This is more of a "conceptual" change (ie. adding readonly here just seems correct on its own, and I'm not proposing the change specifically to fix some performance issue I noticed). At least in simple tests, the JIT seems to produce equivalent codegen. However, the IL is certainly more bloated without readonly (ie. it has one extra local and whatnot), so this still just seems to "make things simpler" for the JIT, like @tannergooding also mentioned.

Thanks. It's not worth making the public API more confusing (making a setter readonly) to make the JIT's life a little bit easier, especially when it's already doing just fine here.

[AaronRobinsonMSFT]

@jkotas and @stephentoub After the most recent updates, I don't see any obvious concerns with these changes. Do either of you concerns with going forward with these changes? An alternative would be to focus on #94134 as opposed to making changes to GCHandle.

[jkotas]

Looks fine to me.