[NativeAOT + Objective-C Marshal] request for advise on restricted GC callouts

[AustinWise]

Background

The Objective-C marshal feature creates reference-counted GC handles. In the Native-AOT implementation, the callbacks for these GC handles calls into C# code. This feature was implemented in #78280.

According to the rules for these sort of callbacks, the C# code cannot allocate GC memory or take locks.

Problem

The C# code currently uses ConditionalWeakTable. This class calls RuntimeHelpers.GetHashCode to get a hash code for object. This in turn can potentially allocate memory or attempt to acquire a lock, in violation of the rules for restated callouts.

Potential Solutions

Don't use the ConditionalWeakTable

Instead of using the ConditionalWeakTable and hoping it and every function it calls obeys the rules for restricted call outs, create a new facility that is specifically designed with these limitations in mind.

One approach is copy the approach CoreCLR uses. The extra data could be stored in the sync table entry for the object (analogous to the sync block entry in CoreCLR). Something like this:

AustinWise@286ced2

Change ConditionalWeakTable to not allocate

Logically, if an object has never had a hash code assigned to it, it cannot have previously been inserted into a ConditionalWeakTable. We can take advantage of this fact to avoid allocating and take locks when locking up an object. Something like this:

AustinWise@d457357

Request for advise

Do either of those approaches seem reasonable?

[ghost]

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

Issue Details

---

Background

The Objective-C marshal feature creates reference-counted GC handles. In the Native-AOT implementation, the callbacks for these GC handles calls into C# code. This feature was implemented in #78280.

According to the rules for these sort of callbacks, the C# code cannot allocate GC memory or take locks.

Problem

The C# code currently uses ConditionalWeakTable. This class calls RuntimeHelpers.GetHashCode to get a hash code for object. This in turn can potentially allocate memory or attempt to acquire a lock, in violation of the rules for restated callouts.

Potential Solutions

Don't use the ConditionalWeakTable

Instead of using the ConditionalWeakTable and hoping it and every function it calls obeys the rules for restricted call outs, create a new facility that is specifically designed with these limitations in mind.

One approach is copy the approach CoreCLR uses. The extra data could be stored in the sync table entry for the object (analogous to the sync block entry in CoreCLR). Something like this:

AustinWise@286ced2

Change ConditionalWeakTable to not allocate

Logically, if an object has never had a hash code assigned to it, it cannot have previously been inserted into a ConditionalWeakTable. We can take advantage of this fact to avoid allocating and take locks when locking up an object. Something like this:

AustinWise@d457357

Request for advise

Do either of those approaches seem reasonable?

Author:	AustinWise
Assignees:	-
Labels:	area-GC-coreclr, untriaged
Milestone:	-

[jkotas]

@VSadov Do you have an option?

I like ConditionalWeakTable change better. It is simpler and it makes ConditionalWeakTable better for other uses as well. The change can be under NATIVEAOT ifdef for now if you do not want to implement TryGetHashCode on other runtimes.

[ghost]

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background

The Objective-C marshal feature creates reference-counted GC handles. In the Native-AOT implementation, the callbacks for these GC handles calls into C# code. This feature was implemented in #78280.

According to the rules for these sort of callbacks, the C# code cannot allocate GC memory or take locks.

Problem

The C# code currently uses ConditionalWeakTable. This class calls RuntimeHelpers.GetHashCode to get a hash code for object. This in turn can potentially allocate memory or attempt to acquire a lock, in violation of the rules for restated callouts.

Potential Solutions

Don't use the ConditionalWeakTable

Instead of using the ConditionalWeakTable and hoping it and every function it calls obeys the rules for restricted call outs, create a new facility that is specifically designed with these limitations in mind.

One approach is copy the approach CoreCLR uses. The extra data could be stored in the sync table entry for the object (analogous to the sync block entry in CoreCLR). Something like this:

AustinWise@286ced2

Change ConditionalWeakTable to not allocate

Logically, if an object has never had a hash code assigned to it, it cannot have previously been inserted into a ConditionalWeakTable. We can take advantage of this fact to avoid allocating and take locks when locking up an object. Something like this:

AustinWise@d457357

Request for advise

Do either of those approaches seem reasonable?

Author:	AustinWise
Assignees:	-
Labels:	area-GC-coreclr, untriaged, area-NativeAOT-coreclr
Milestone:	-

[VSadov]

ConditionalWeakTable change is simpler, but there is a feeling that it would be a subtle hack in a way how otherwise very independent pieces of the runtime interact.

The whole part about calling managed code from GC feels fragile to me, but unavoidable. My intuition is that we should take any opportunity to constrain what code may possibly need to run this way.

From this point it would be better to put interop stuff in the syncblock. That would serve the distinct purpose and will have less impact from the layering point of view. - fewer components will need to know about it.

• Why do we have this TryGetHashcode API? someone will ask.
• Oh, that is because ConditionalWeakTable uses it in FindEntry, and it needs it because there is code related to interop that calls this method from a GC callout and in the current GC implementation the callout has the following restrictions...

Let me think a bit more about long term viability of tweaking the ConditionWeakTable.

[VSadov]

I also think the interop info referenced via the syncblock could be a bit faster to retrieve (no hashtable lookup), but I am unsure how meaningful the difference will be for a typical interop scenario.

[jkotas]

The reason that tipped me over to prefer ConditionalWeakTable fix was that it makes the runtime better for other scenarios. I expect that it is fairly common to use try pattern with ConditionalWeakTable. It is beneficial to avoid assigning the hashcode when the try pattern is about to fail: There is a local saving from skipping the hashcode assignment and lookup that is guaranteed to fail, and there is potential global saving from reducing number of syncblocks that need to be created due to needing both hashcode and lock.

it would be a subtle hack in a way how otherwise very independent pieces of the runtime interact.

There are 3 methods (1 worker method and 2 wrappers) on ConditionalWeakTable that would be impacted by this. We can add comments on them for clarity. We have number of other places like that in CoreLib where one cannot call X from Y (that is independent on X) because it would introduce cycle.

Why do we have this TryGetHashcode API? someone will ask.

We can consider making the TryGetHashcode API public so that 3rd party hash tables can do this optimization as well.

I am unsure how meaningful the difference will be for a typical interop scenario.

Right, the interop scenarios that use ConditionalWeakTable are not the most perf critical ones.

[VSadov]

There is a local saving from skipping the hashcode assignment and lookup that is guaranteed to fail

Yes, that did occur to me. Not sure if that is universally useful, as hashtable lookups are often expected not to miss, but for a conditional weak table it is definitely useful.

I guess we can go with the TryGetHashcode/CWT change for the above considerations and because it is a smaller change that does not preclude the other approach if we run into more issues.

TryGetHashcode API will likely go to RuntimeHelpers, but I think we should use it internally for a while before considering making it public.