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 ee_alloc_context (NativeAOT) #104851

Merged
merged 5 commits into from
Oct 16, 2024
Merged

Conversation

noahfalk
Copy link
Member

@noahfalk noahfalk commented Jul 13, 2024

This change is some preparatory refactoring for the randomized allocation sampling feature. We need to add more state onto allocation context but we don't want to do a breaking change of the GC interface. The new state only needs to be visible to the EE but we want it physically near the existing alloc context state for good cache locality. To accomplish this we created a new ee_alloc_context struct which contains an instance of gc_alloc_context within it.

The new field ee_alloc_context::combined_limit should be used by fast allocation helpers to determine when to go down the slow path. Most of the time combined_limit has the same value as alloc_limit, but periodically we need to emit an allocation sampling event on an object that is somewhere in the middle of an AC. Using combined_limit rather than alloc_limit as the slow path trigger allows us to keep all the sampling event logic in the slow path.

There is another PR for CoreCLR making the same change: #104849

@jkotas
Copy link
Member

jkotas commented Jul 13, 2024

In a future PR we will add a field called combined_limit

I think you can add it in this PR. Adding the extra field is going to touch a lot of the same lines as this PR.

@jkotas
Copy link
Member

jkotas commented Jul 13, 2024

(Also, same comment as #104851 (comment).)

@noahfalk noahfalk force-pushed the combined_limit_nativeaot branch 2 times, most recently from 8d9bb20 to 01933fa Compare July 14, 2024 07:59
src/coreclr/nativeaot/Runtime/thread.cpp Outdated Show resolved Hide resolved
src/coreclr/nativeaot/Runtime/thread.inl Outdated Show resolved Hide resolved
@noahfalk
Copy link
Member Author

@jkotas @MichalStrehovsky - I've applied the feedback from the CoreCLR issue to this one as well. Is anything else needed before you can signoff?

@jkotas
Copy link
Member

jkotas commented Jul 29, 2024

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member

jkotas commented Jul 29, 2024

nativeaot-outerloop (Build windows-x64 Release NativeAOT_Checked timeouts look like infrastructure issues, but they do not go away with reruns. Either the Helix queues are overloaded, or there is a bug that makes the tests run a lot slower.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me otherwise

src/coreclr/nativeaot/Runtime/thread.h Show resolved Hide resolved
src/coreclr/nativeaot/Runtime/thread.h Show resolved Hide resolved
@noahfalk
Copy link
Member Author

I've updated for the feedback so far. Everything is addressed except NativeAOT SOS which has to be handled out-of-band. Looking at the time I think I may need to call it quits on this for .NET 9 and instead treat it as .NET 10 on a slower cadence. At this point I don't think I have more time available right now to do a staged checkin, revalidate testing after the ongoing changes, and continue to have time in reserve to respond to potential post-checkin issues.

noahfalk and others added 4 commits October 14, 2024 21:43
This change is some preparatory refactoring for the randomized allocation sampling feature. We need to add more state onto allocation context but we don't want to do a breaking change of the GC interface. The new state only needs to be visible to the EE but we want it physically near the existing alloc context state for good cache locality. To accomplish this we created a new ee_alloc_context struct which contains an instance of gc_alloc_context within it.

The new ee_alloc_context::combined_limit should be used by fast allocation helpers to determine when to go down the slow path. Most of the time combined_limit has the same value as alloc_limit, but periodically we need to emit an allocation sampling event on an object that is somewhere in the middle of an AC. Using combined_limit rather than alloc_limit as the slow path trigger allows us to keep all the sampling event logic in the slow path.
- removed unnecessary UpdateCombinedLimit() in thread detach
- updated comment for workaround on 96081
- swapped to updating combined_limit inside GcEnumAllocContexts() instead of in RestartEE()
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@noahfalk noahfalk merged commit 588bc45 into dotnet:main Oct 16, 2024
88 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants