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 for profiler test arm64 #102066

Merged
merged 6 commits into from
May 10, 2024
Merged

Fix for profiler test arm64 #102066

merged 6 commits into from
May 10, 2024

Conversation

davmason
Copy link
Member

@davmason davmason commented May 10, 2024

While trying to investigate #100114 I found an unrelated issue, the delegate that we pass to the profiler can be garbage collected and cause the process to crash. This fixes that issue, as well as adds code to read all of the profilee's stdout.

@davmason davmason added this to the 9.0.0 milestone May 10, 2024
@davmason davmason requested a review from a team May 10, 2024 06:30
@davmason davmason self-assigned this May 10, 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

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

I'd recommend explicitly caching that delegate, otherwise LGTM

public static int RunTest(String[] args)
{
ManualResetEvent _profilerDone = new ManualResetEvent(false);
PassCallbackToProfiler(() => _profilerDone.Set());
PassCallbackToProfiler(ProfilerDone);
Copy link
Member

Choose a reason for hiding this comment

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

I would explicitly cache this delegate given the correctness of the test depends on it. Right now we are relying on the C# compiler to do that caching implicitly. Some versions of the C# compiler (C# 11) do cache it in practice but this is not considered guaranteed behavior by the spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will push changes to explicitly cache the delegate because explicit is better, but isn't that link just talking about whether the C# compiler caches and reuses the same delegate in multiple callsites? It could still keep the delegate rooted even if it never uses the same one twice in old versions.

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.

LGTM, thanks!

@davmason davmason merged commit 279f29e into dotnet:main May 10, 2024
69 checks passed
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jun 10, 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