[netcore] Add Monitor.LockContentionCount icall #16538
Conversation
filipnavara
requested review from
akoeplinger,
BrzVlad,
EgorBo,
lambdageek,
marek-safar,
steveisok,
thaystg and
vargaz
as code owners
|
I guess I need to bump |
| gint64 | ||
| ves_icall_System_Threading_Monitor_Monitor_LockContentionCount (void) | ||
| { | ||
| #ifndef DISABLE_PERFCOUNTERS |
There was a problem hiding this comment.
Please favor if over #if, such as leaving the pointer null if not enabled.
There was a problem hiding this comment.
That is preexisting code that is already compiled-out in this way.
|
I was going to say that: but for netcore I don't think so. |
|
In general, |
| @@ -1438,3 +1438,15 @@ ves_icall_System_Threading_Monitor_Monitor_Enter (MonoObject *obj) | |||
| { | |||
| mono_monitor_enter_internal (obj); | |||
| } | |||
|
|
|||
| #if ENABLE_NETCORE | |||
There was a problem hiding this comment.
nit: #ifdef (we have a mix of #if ENABLE_NETCORE and #ifdef ENABLE_NETCORE makes sense to unify it
There was a problem hiding this comment.
Yeah, I wanted to do that... but then I reverted the whole experiment and lost this small change. :)
I noticed that there's a mix already. Unless I will need to amend the PR due to some other change I'd prefer to do the #if/#ifdef unification in separate PR once there is agreement on the preferred way.
There was a problem hiding this comment.
Not #if vs. #ifdef, though that is something, but if vs. #if[def]. Less code should be explicitly configuration dependent.
There was a problem hiding this comment.
@jaykrell I don't disagree, in general. In this specific instance it is really #if vs #ifdef though. The netcore version is significantly different from the regular runtime. It is not just an optional feature where we can easily rely on compiler dead code elimination and regular if.
|
@monojenkins build failed |
| @@ -154,4 +154,10 @@ ICALL_EXPORT | |||
| void | |||
| ves_icall_System_Threading_Monitor_Monitor_Enter (MonoObject *obj); | |||
|
|
|||
| #if ENABLE_NETCORE | |||
There was a problem hiding this comment.
Nit: ifdef here as well. Would be nice to get that consistent.
|
@steveisok could we enable corefx tests for this? |
|
@marek-safar Unfortunately the test would not work yet. Performance counters are globally disabled in netcore build in |
* [netcore] Complete Monitor.LockContentionCount implementation Fixes #16538
* Add Monitor.LockContentionCount icall * Add NOHANDLES around the new icall Commit migrated from mono/mono@e32fb11
…ono#17719) * [netcore] Complete Monitor.LockContentionCount implementation Fixes mono/mono#16538 Commit migrated from mono/mono@722c6f7
Contributes to #14828.
The PR just wires up the Monitor.LockContentionCount API to an icall. The actual implementation is not done yet.
I wanted to reuse the existing
thread_contentionsperformance counter but all performance counters unconditionally disabled on netcore builds. Also, the counter is 32-bit only while the managed API returns 64-bit values.The CoreCLR implementation uses a neat trick to reduce the cost of counting. It stores a per-thread contention counter (eg. in
MonoInternalThreadfield) and a global overflow counter. Up to 2^32 contentions could be recorded in the per-thread field with simple volatile incrementation. Once an overflow happens a slow path is taken that updates a global overflow counter. TheLockContentionCountAPI enumerates all threads and adds up together all the per-thread values along with the global overflow counter. I tried to reimplement that in Mono but I didn't find any way to enumerate all threads and getMonoInternalThreadfrom them.