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

Fix failing test on NativeAOT #109853

Merged
merged 1 commit into from
Nov 19, 2024
Merged

Fix failing test on NativeAOT #109853

merged 1 commit into from
Nov 19, 2024

Conversation

noahfalk
Copy link
Member

Fixes #109828
This test hadn't been updated to account for NativeAOT's lack of type names in the new randomized sampling allocation events.

@noahfalk
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@noahfalk
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@noahfalk
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Fixes dotnet#109828
This test hadn't been updated to account for NativeAOT's lack of type names in the new randomized sampling allocation events.
@noahfalk
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@noahfalk
Copy link
Member Author

@jkotas @MichalStrehovsky - I think this resolves the failure in the allocation sampling test though it appears the outer loop runs still have some other failures in the Numerics tests.

@MichalStrehovsky
Copy link
Member

@jkotas @MichalStrehovsky - I think this resolves the failure in the allocation sampling test though it appears the outer loop runs still have some other failures in the Numerics tests.

That one was fixed in #109842. We could rebase and recheck, but I think this is good to merge as-is! Thanks!

Btw, we are able to compute type names of everything on the GC heap (since we keep it around for object.GetType to work). If it's possible to call into managed code at the spot where this is needed (we can only compute the names in managed code), it should be fixable.

Copy link
Member

@tommcdon tommcdon left a comment

Choose a reason for hiding this comment

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

Thanks!

@noahfalk
Copy link
Member Author

Btw, we are able to compute type names of everything on the GC heap (since we keep it around for object.GetType to work). If it's possible to call into managed code at the spot where this is needed (we can only compute the names in managed code), it should be fixable.

Ah thats good to know. I had been under the impression the names were strippable like other metadata and couldn't be assured. The place where we need the name is here: https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/Runtime/GCHelpers.cpp#L485

The callstack would look like:

FireAllocationSampled
GcAllocInternal
RhAllocateXYZ
ManagedCode

I assume there isn't anything preventing a native->managed call at that point but it would be a little odd if managed code did any allocations which could lead to recursion. Not knowing what is involved just yet, do you think we could keep that call allocation-free?

@noahfalk noahfalk merged commit e33be4d into dotnet:main Nov 19, 2024
85 of 92 checks passed
@jkotas
Copy link
Member

jkotas commented Nov 19, 2024

I assume there isn't anything preventing a native->managed call at that point but it would be a little odd if managed code did any allocations which could lead to recursion.

Right, it would not be pretty to make this work.

Sending the type name as part of each sample can result in a lot of redundant information being transferred. Would it be better to send the type id to type name mapping in separate events, once for each type id? It would work better for native AOT as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

randomizedallocationsampling tests are failing on nativeaot
4 participants