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 EventSource event for WaitHandle waits #94737

Merged
merged 36 commits into from
Dec 7, 2023
Merged

Add EventSource event for WaitHandle waits #94737

merged 36 commits into from
Dec 7, 2023

Conversation

verdie-g
Copy link
Contributor

@verdie-g verdie-g commented Nov 14, 2023 •
edited
Loading

Context: #94264

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 14, 2023
@verdie-g verdie-g mentioned this pull request Nov 14, 2023
Closed
@jkotas jkotas requested a review from kouvel November 14, 2023 22:23
@ghost
Copy link

ghost commented Nov 14, 2023

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

Issue Details

Context: #94264

Implementation details that need validation:

  • I've named the event just Wait. MonitorWait doesn't really fit because there name Monitor is not used where the event is emitted. ContentionWait could be confused with Contention.
  • It seems like event versions start at 0 with no suffix in the name. I've started this event at 1 and added a prefix, tell me if I need to change that
  • The start event includes the same properties as ContentionStart: ClrInstanceID, LockID, AssociatedObjectId but no ContentionFlags or LockOwnerThreadID
  • It could be interesting to add a ThreadID property. It would also help to have a IsThreadPoolThread but I don't think there is a concept of thread pool at that level
  • The new event is under the keyword contention (0x400). I think it makes sense to collect it whenever the contention events are collected
  • No changes are required on the Lock class because it doesn't support the wait operation
Author: verdie-g
Assignees: -
Labels:

area-System.Threading, area-VM-coreclr, community-contribution
Milestone: -

verdie-g
verdie-g commented Nov 14, 2023
View reviewed changes
src/coreclr/vm/ClrEtwAll.man Outdated Show resolved Hide resolved
src/coreclr/vm/ClrEtwAll.man Outdated Show resolved Hide resolved
src/coreclr/vm/ClrEtwAll.man Outdated Show resolved Hide resolved
src/coreclr/vm/ClrEtwAll.man Show resolved Hide resolved
src/coreclr/vm/ClrEtwAll.man Outdated Show resolved Hide resolved
@verdie-g verdie-g marked this pull request as ready for review November 14, 2023 23:47
@verdie-g verdie-g mentioned this pull request Nov 15, 2023
Closed
@verdie-g verdie-g changed the title Add etw event for Monitor.Wait Add EventSource event for WaitHandle waits Nov 17, 2023
This was referenced Nov 17, 2023
Closed
Open
Open
jkotas
jkotas reviewed Nov 17, 2023
View reviewed changes
src/coreclr/vm/syncblk.cpp Outdated Show resolved Hide resolved
kouvel
kouvel reviewed Nov 17, 2023
View reviewed changes
src/coreclr/inc/eventtracebase.h Outdated Show resolved Hide resolved
src/coreclr/vm/ClrEtwAll.man Outdated Show resolved Hide resolved
src/coreclr/vm/ClrEtwAll.man Outdated Show resolved Hide resolved
src/coreclr/vm/ClrEtwAll.man Outdated Show resolved Hide resolved
@verdie-g
Copy link
Contributor Author

It doesn't compile, the FireEtwWaitHandleWaitStart function is not generated. I'll continue tomorrow.

This was referenced Nov 18, 2023
Open
Closed
@verdie-g verdie-g marked this pull request as draft November 18, 2023 18:01
@build-analysis build-analysis bot mentioned this pull request Nov 18, 2023
Closed
@verdie-g verdie-g marked this pull request as ready for review November 20, 2023 22:06
verdie-g and others added 21 commits December 7, 2023 13:37
@verdie-g
Add existing event name to exception
f52bfc9
@verdie-g
Address comments
a743853
@verdie-g
Remove this as associated object in Condition
235774f
@verdie-g
Disable wait handle event geneneration
88d96ac
@verdie-g
Fix mono build
b554d49
@verdie-g
Pass associated object in Condition
45d4628
@verdie-g
Rename user data
8bdf215
@verdie-g
Adjust wait source in native wait
909bccc
@verdie-g
Fix task name
f1cf600
@verdie-g
Add test
e0d77ca
@verdie-g
Only emit event in native code
c6b1f6f
@verdie-g
Disable test for mono
e83a522
@verdie-g
Fix test
7c82bc4
@verdie-g
Assert.NotEqual doesn't support custom error message
500455a
@verdie-g
Make test more robust
3730b7f
@verdie-g
Make test even more robust
81b689f
@verdie-g
Revert "Only emit event in native code"
22ffb48 
This reverts commit 49c270a.
@verdie-g
Remove unrelated changes
c514edb
@kouvel @verdie-g
Raise the events in NativeAOT and Mono, some other updates
a5443af 
- Updated to send the events only for nonzero-timeout alertable waits
- Updated to try a zero-timeout wait first for waits other than Monitor.Wait before sending events
- Updated WaitHandle.cs to send events for WaitOne and WaitMultiple cases for NativeAOT/Mono
- Updated NativeAOT's and Mono's Monitor.Wait to pass in the associated object
- Some minor changes to the test, and some other miscellaneous fixes
@verdie-g
Fix merge
e868b0e
@verdie-g
Fix merge
e29adcf
kouvel
kouvel approved these changes Dec 7, 2023
View reviewed changes
Copy link
Member

@kouvel kouvel 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!

@verdie-g
Copy link
Contributor Author

verdie-g commented Dec 7, 2023

Thanks for all the help @kouvel! What should I do about the failed action?

@verdie-g verdie-g mentioned this pull request Dec 7, 2023
Merged
@build-analysis build-analysis bot mentioned this pull request Dec 7, 2023
Open
@kouvel
Copy link
Member

kouvel commented Dec 7, 2023

The console log for the failure is almost empty, not sure what might have happened. Previous failures of the test didn't look similar. It seems to have passed on rerun.

@kouvel kouvel merged commit 7bbfcc4 into dotnet:main Dec 7, 2023
190 checks passed
brianrob pushed a commit to microsoft/perfview that referenced this pull request Dec 8, 2023
@verdie-g
Support WaitHandleWait events (#1958)
dbf41bd 
* Add WaitHandleWaitStart to AnyStacks

New event introduced in dotnet/runtime#94737

* Fix compilation

* Reorder generated members

* Address PR comment
verdie-g added a commit to criteo-forks/runtime that referenced this pull request Dec 12, 2023
@verdie-g
Add EventSource event for WaitHandle waits (dotnet#94737)
a04deb4 
Add start/stop events for WaitHandle waits

Context: dotnet#94264
@github-actions github-actions bot locked and limited conversation to collaborators Jan 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vargaz vargaz vargaz left review comments

@ezsilmar ezsilmar ezsilmar left review comments

@noahfalk noahfalk noahfalk left review comments

@danmoseley danmoseley danmoseley left review comments

@jkotas jkotas jkotas left review comments

@lateralusX lateralusX lateralusX left review comments

@kouvel kouvel kouvel approved these changes

@marek-safar marek-safar Awaiting requested review from marek-safar marek-safar is a code owner

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

@lambdageek lambdageek Awaiting requested review from lambdageek lambdageek is a code owner

@thaystg thaystg Awaiting requested review from thaystg thaystg is a code owner

Assignees
No one assigned
Labels
area-System.Threading community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@verdie-g @kouvel @lateralusX @jkotas @vargaz @ezsilmar @noahfalk @danmoseley @MichalStrehovsky