-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add the concept of "notification profilers" to the runtime #53122
Conversation
Tagging subscribers to this area: @tommcdon Issue DetailsThis allows multiple profilers to register if they only request a subset of profiler behavior that is multiple profiler safe - ReJIT, IL modification, ELT stubs, etc are forbidden for these profilers. The basic approach is relatively straightforward, instead of calling one profiler back we call back a list of them. Items left:
Documentation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't look too closely at the thread safety around loading/unloading, but the general gist of the work looks good to me! Excited to see where this goes 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! Comments inline on some small stuff I noticed.
COR_PRF_MONITOR_CLR_EXCEPTIONS | | ||
COR_PRF_ENABLE_STACK_SNAPSHOT | | ||
COR_PRF_USE_PROFILE_IMAGES | | ||
COR_PRF_DISABLE_ALL_NGEN_IMAGES, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't some of these side-effecting? Since the notification profilers are intended to be SxS perhaps we should scope it back to the options that are allowable on attach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good conversation to have. I was taking the approach to not ban any side effects, but rather ban side effects that would be non trivial to implement (ELT hooks) or are hard to provide in a way that is consistent and reproducible (IL rewriting).
I am worried about being too limiting in what scenarios we support. It is unfortunate to look back in a few releases and think "we could have done it except we arbitrarily decided to block this thing that is actually safe"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind taking the risk as long as you think you've had suitable time to consider it and test the additional APIs : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although one other thought, if we are going to allow side-effects then 'NotificationOnly' feels like a misleading name. What about calling them "SxS", "Cooperative", or another name similar to that?
9821289
to
94864df
Compare
…ationOnly, call UpdateGlobalEventMask before returns
…tal race conditions
94864df
to
6cf6559
Compare
SArray doesn't work with classes that aren't POD since it doesn't initialize the backing memory and uses operator= not the constructor to assign elements
This allows multiple profilers to register if they only request a subset of profiler behavior that is multiple profiler safe - ReJIT, IL modification, ELT stubs, etc are forbidden for these profilers.
The basic approach is relatively straightforward, instead of calling one profiler back we call back a list of them.
Items left:
Testing:
Documentation