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

For transition profiler callbacks, always load the thread #105104

Merged
merged 4 commits into from
Jul 19, 2024

Conversation

jkoritzinsky
Copy link
Member

Also, add tests for profiler callbacks on a new thread to the runtime

Fixes #105070

Also, add tests for profiler callbacks on a new thread to the runtime
@jkoritzinsky jkoritzinsky changed the title For profiler callbacks, always load the thread For transition profiler callbacks, always load the thread Jul 18, 2024
Copy link
Contributor

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

Copy link
Member

@davmason davmason left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, I don't know the details of the interop space enough to comment on the fix but the profiler tests look good to me

Copy link
Member

@mdh1418 mdh1418 left a comment

Choose a reason for hiding this comment

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

I don't fully understand all the IL code emitting details, but from #69761, the change to EmitProfilerEndTransitionCallback to have _ASSERTE(SF_IsForwardStub(dwStubFlags)); + defaulting to !fReverseInterop code paths in stubhelpers.cpp seems to suggest that Profiler transitions shouldn't occur during reverse interops, but even if they did, behave the same as forward interops? Am I understanding that correctly?

Could we add asserts that pThread is not null in stubhelpers.cpp ProfilerBeginTransitionCallback/ProfilerEndTransitionCallback?

@jkoritzinsky
Copy link
Member Author

I don't fully understand all the IL code emitting details, but from #69761, the change to EmitProfilerEndTransitionCallback to have _ASSERTE(SF_IsForwardStub(dwStubFlags)); + defaulting to !fReverseInterop code paths in stubhelpers.cpp seems to suggest that Profiler transitions shouldn't occur during reverse interops, but even if they did, behave the same as forward interops? Am I understanding that correctly?

Could we add asserts that pThread is not null in stubhelpers.cpp ProfilerBeginTransitionCallback/ProfilerEndTransitionCallback?

The profiler transitions for "reverse interop" are done as part of the JIT code emit now for these scenarios, so we don't handle them in the IL stub (which is what #69761 was fixing).

I can add the assert.

Copy link
Member

@mdh1418 mdh1418 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!

@jkoritzinsky jkoritzinsky merged commit 2f0920c into dotnet:main Jul 19, 2024
93 of 96 checks passed
@jkoritzinsky jkoritzinsky deleted the profile-callback branch July 19, 2024 23:37
@gbalykov
Copy link
Member

@jkoritzinsky thanks! Does this need to be backported to .net 7 and .net 8?

@jkoritzinsky
Copy link
Member Author

I'll backport this to .NET 8. 7 is EOL so I can't backport there.

@jkoritzinsky
Copy link
Member Author

/backport to release/8.0-staging

Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/10029358160

@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 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.

Profiling problem on armv7l
5 participants