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

Emit ICorProfiler ModuleLoadFinished for dynamic modules #77068

Merged
merged 3 commits into from
Oct 17, 2022

Conversation

davmason
Copy link
Member

#63157 introduced a regression where we do not call ModuleLoadFinished for dynamic modules

Fixes #76016

@davmason davmason requested review from VSadov and a team October 14, 2022 23:34
@davmason davmason self-assigned this Oct 14, 2022
@ghost
Copy link

ghost commented Oct 14, 2022

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

Issue Details

#63157 introduced a regression where we do not call ModuleLoadFinished for dynamic modules

Fixes #76016

Author: davmason
Assignees: davmason
Labels:

area-Diagnostics-coreclr

Milestone: -

@davmason davmason merged commit 3ba1dd2 into dotnet:main Oct 17, 2022
@k15tfu
Copy link
Contributor

k15tfu commented Oct 21, 2022

@davmason Hi! It turned out to be a critical issue for us and for our customers. Will this fix be included in 7.0 rc3, is there anything we can do for that?

@davmason
Copy link
Member Author

The servicing PR for 7.0 is here: #77533

@dakersnar
Copy link
Contributor

It seems like this commit might have caused one of the regressions in this issue: dotnet/perf-autofiling-issues#9181

Specifically, the System.Memory.Span.BinarySearch regression.

@davmason
Copy link
Member Author

Hi @dakersnar,

Do you have any exiting investigation you can point me to? Off the top of my head I would be very surprised if this fix caused a regression in binary search, it is only called on module load and only if a profiler is attached. But stranger things have happened

@dakersnar
Copy link
Contributor

After a second look, I think this regression is likely noise and not related to this PR. The historical data indicates that the test is bimodal and we also had a BenchmarkDotnet update on the same day as this "regression".
image

carlossanlop pushed a commit that referenced this pull request Nov 3, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 1, 2022
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.

[ProfilerAPI] No more ModuleLoadFinished callbacks for dynamic modules
4 participants