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.

Sign up for GitHub

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

Jump to bottom

Add tests for notification profilers #57429

Merged
merged 9 commits into from
Aug 17, 2021
Merged

Add tests for notification profilers #57429

merged 9 commits into from
Aug 17, 2021

Conversation

davmason
Copy link
Member

@davmason davmason commented Aug 15, 2021 •
edited
Loading

Add CI tests for notification profilers, and fix a couple things to make life with multiple profilers easier

  • Reduced default timeout for profiler detach - we used to try at 300ms, 600ms, 5s, 10s, and then 10 minutes. The check is just a read of an int, so there's no reason to wait 10 minutes
  • Got rid of a couple asserts that were wrong since switching from the dedicated profiler attach thread to the diagnostics server implementation
  • ProfControlBlock::GetProfilerInfo would only work for active profilers, not attaching profilers, which could cause subtle bugs

@davmason davmason added this to the 6.0.0 milestone Aug 15, 2021
@davmason davmason requested a review from a team August 15, 2021 05:22
@davmason davmason self-assigned this Aug 15, 2021
@ghost
Copy link

ghost commented Aug 15, 2021

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

Issue Details

Add CI tests for notification profilers, and fix a couple things to make life with multiple profilers easier

  • Reduced default timeout for profiler detach - we used to try at 300ms, 600ms, and then 10 minutes
  • Got rid of a couple asserts that were wrong since switching from the dedicated profiler attach thread to the diagnostics server implementation
  • ProfControlBlock::GetProfilerInfo would only work for active profilers, not attaching profilers, which could cause subtle bugs
Author: davmason
Assignees: davmason
Labels:

area-Diagnostics-coreclr
Milestone: 6.0.0

@hoyosjs hoyosjs linked an issue Aug 16, 2021 that may be closed by this pull request
Work remaining for notification profilers #54211
Closed
8 tasks
hoyosjs
hoyosjs approved these changes Aug 17, 2021
View reviewed changes
src/coreclr/dlls/mscorrc/mscorrc.rc
@@ -738,8 +738,8 @@ BEGIN
IDS_E_PROF_NOT_ATTACHABLE "Loading profiler failed. The profiler COM object was instantiated, but the profiler does not support attaching to a live process. The profiler must be loaded at application startup by using a launcher program included with the profiler (if any) or by setting the COR_ENABLE_PROFILING and COR_PROFILER environment variables before launching the application to be profiled. Profiler CLSID: '%s'"
IDS_E_PROF_UNHANDLED_EXCEPTION_ON_LOAD "Loading profiler failed. There was an unhandled exception while trying to instantiate the profiler COM object. Please ensure the CLSID is associated with a valid profiler designed to work with this version of the runtime. Profiler CLSID: '%s'."
IDS_PROF_ATTACH_REQUEST_RECEIVED "The CLR received a request to attach a profiler. Profiler CLSID: '%s'."
IDS_PROF_DETACH_INITIATED "The profiler currently in use has requested to be detached from the process. The CLR has disabled communication with the profiler and will unload the profiler when it is safe to do so."
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to new line these? All others don't.

Copy link
Member Author

Choose a reason for hiding this comment

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

They are printed to the debugger and without a newline it would get printed all on one line. I added newlines to all the profiler ones so they are consistent

src/tests/profiler/native/multiple/multiple.cpp
return clsid;
}

HRESULT MultiplyLoaded::InitializeCommon(IUnknown* pICorProfilerInfoUnk)
Copy link
Member

Choose a reason for hiding this comment

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

nit: Should this be multiple instead of multiply?

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 was using multiply as an adverb, not a verb 😄

@jaredpar jaredpar mentioned this pull request Aug 17, 2021
Closed
@davmason
Copy link
Member Author

The one remaining failure is unrelated to my changes, going to merge now

@davmason davmason merged commit e9e9f10 into dotnet:main Aug 17, 2021
@davmason davmason deleted the multiple_profilers_test branch August 17, 2021 18:32
@runfoapp runfoapp bot mentioned this pull request Aug 17, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hoyosjs hoyosjs hoyosjs approved these changes

Assignees

@davmason davmason

Labels
area-Diagnostics-coreclr
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Work remaining for notification profilers
2 participants
@davmason @hoyosjs