Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a new GC handle and use it to simplify NativeAOT corelib #99512

Closed

Conversation

AustinWise
Copy link
Contributor

Problem

In the .NET runtime, particularly in its interop systems, it is common to attached some extra state
to an object. In CoreCLR, this data is often stored in the SyncBlock of an object. In NativeAOT, this
is often accomplished using a ConditionalWeakTable.

A disadvantage of using ConditionalWeakTable compared to the SyncBlock is the behavior around
finalization. The SyncBlock system hooks into the GC to detect when an object is collected and waits until
that moment to schedule the deallocation of data associated with an object. When using a ConditionalWeakTable,
the value in a table is queued for finalization as soon as the key is queued for finalization.
The NativeAOT corelib has to ensure that the key in the table has been fully collected and call
GC.ReRegisterForFinalize to defer finialization if it has not. Failure to check for this condition
has cause a couple of bugs in the past:
#86882
#99185

This PR's solution

With a small tweak, the dependant handle and the ConditionalWeakTable can make writing these
sorts of finalizers in the corelib easier. If the dependant handle always promotes its secondary
as long as the primary was a alive, the secondary object will not be put on the finalization queue
until the primary object has been fully collected. This PR implements a new type of GC handle with
this behavior called a "defer finalize dependant handle".

Use cases

This PR uses the handle in 4 places in the NativeAOT corelib:

  • Objective-C Marshal. The new handle type has the most impact here: it simplifies the code and removes
    an extra GC handle allocation per tracked object.
  • SyncTable
  • Marshaling of delegates in P/Invoke
  • COM managed object wrapper

I've identified a couple places in CoreCLR where this handle could be used, but have not implemented
it in this PR:

Conceivably, functionality that currently is placed on the SyncBlock in CoreCLR could be moved to use
this new handle to manage the lifetime of data. This might allow for more sharing between CoreCLR
and NativeAOT. Longer term, the SyncBlock and its custom handle table could removed in favor of using
this new GC handle.

Analysis

Here are some of the pros and cons I can think of. Please let me know if there are other considerations
or if there are any best practices to quantify the performance impact of a change like this.

Pros:

  • Simplifies implementation of the CoreLib
  • Potentially improves performance by not having to rerun finalizers

Cons:

  • Introduces a new GCHandle type that diagnostic tools need to know about
  • Potentially increases memory usage by extending the lifetime of the secondary object in the dependant handle

TODO

  • Gain consensus on whether adding a new GC handle is worth it
  • Quantify performance impact if possible
  • Add more test coverage
  • Figure out if there is a better way to write tests against corelib implementation details than using UnsafeAccessor. Maybe move the test to /src/tests?

Support is for this features is added to ConditionalWeakTable and used in
NativeAOT. This replaces the pattern of re-registering for finalization
repeatedly until the primary object is collected.

# Conflicts:
#	src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp
#	src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/PInvokeMarshal.cs
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 11, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 11, 2024
@jkotas jkotas added area-GC-coreclr community-contribution Indicates that the PR has been added by a community member and removed community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 11, 2024
@jkotas
Copy link
Member

jkotas commented Mar 11, 2024

The GC handle that you are proposing provides a first-class functional equivalent of phantom references. It is possible to emulate it using combo of existing GC handles, but this emulation has overhead.

If we were to build a first-class functional equivalent of phantom references, I think we would want to design and expose them as a proper public API. I do not think that it is worth it for the few internal uses. In general, phantom references can be used in interop as a safehandles alternative with different perf characteristics and lifetime guarantees.

Regular CoreCLR CoreLib has a few places where these can be used as well. For example:

if (RuntimeMethodHandle.GetResolver(m_methodHandle) != null)
{
// Somebody might have been holding a reference on us via weak handle.
// We will keep trying. It will be hopefully released eventually.
GC.ReRegisterForFinalize(this);
return;
}

@AustinWise
Copy link
Contributor Author

I do not think that it is worth it for the few internal uses.

Thanks for taking a look, I suspected as much. I'll close this PR. Also thanks for the pointer to phantom references.

@AustinWise AustinWise closed this Mar 12, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-GC-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants