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 VirtualCallStubManager stats capturing on EE shutdown #102073

Merged
merged 2 commits into from
May 13, 2024

Conversation

yurai007
Copy link
Contributor

Before this change logging stats to StubLog file doesn't work because on EE shutdown we don't shut down manager. Since it's only about logging we can perform uninit earlier.

Part of #84834, cc @dotnet/samsung

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 10, 2024
Copy link
Contributor

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

Before this change logging stats to StubLog file doesn't work because on EE shutdown
we don't shut down manager. Since it's only about logging we can perform uninit earlier.
@jkotas
Copy link
Member

jkotas commented May 10, 2024

Before this change logging stats to StubLog file doesn't work because on EE shutdown we don't shut down manager

What is the exact situation where it does not work? Is the whole block with the call unreachable?

src/coreclr/vm/ceemain.cpp Outdated Show resolved Hide resolved
@jkotas jkotas requested a review from janvorli May 10, 2024 15:18
@yurai007
Copy link
Contributor Author

Before this change logging stats to StubLog file doesn't work because on EE shutdown we don't shut down manager

What is the exact situation where it does not work? Is the whole block with the call unreachable?

I can explain giving a bit more background. Couple of weeks ago I tried capture VirtualCallStubManager stats, however I noticed that only empty StubLog was created. It turned out that whole block in EEShutDownHelper was bypassed because fIsDllUnloading was set to false. It happened on both RISC-V and x64 platforms.

@jkotas
Copy link
Member

jkotas commented May 11, 2024

Hmm, you are right. We do not seem to call this with fIsDllUnloading=true on Unix and so this block is never executed. @janvorli It sounds like another corner case shutdown bug that we have inherited from framework.

@yurai007 For this specific fix, could please move this logging call and also WriteJitHelperCountToSTRESSLOG to be right after Interpreter::PrintPostMortemData above? PrintPostMortemData is similar kind of logging, so we should have all those calls together.

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.

Thanks

@jkotas jkotas merged commit a591a77 into dotnet:main May 13, 2024
87 of 89 checks passed
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
)

* Fix VirtualCallStubManager stats capturing on EE shutdown

Before this change logging stats to StubLog file doesn't work because on EE shutdown
we don't shut down manager. Since it's only about logging we can perform uninit earlier.

* Address code review comments
@github-actions github-actions bot locked and limited conversation to collaborators Jun 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-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.

3 participants