-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Implement APIs for some threading metrics (CoreCLR) #22754
Conversation
kouvel
commented
Feb 21, 2019
- API review: https://github.com/dotnet/corefx/issues/35500
…ixes - API review: https://github.com/dotnet/corefx/issues/35500 - May depend on dotnet/coreclr#22754 - Fixed `Timer` implementation on Unixes. Previously there was only ever one timer request from the upper-level implementation and that is not the case anymore, so the lower-level "app domain timer" implementation needed to handle multiple timer requests.
There was no measurable change to perf from the changes |
…ixes - API review: https://github.com/dotnet/corefx/issues/35500 - May depend on dotnet/coreclr#22754 - Fixed `Timer` implementation on Unixes. Previously there was only ever one timer request from the upper-level implementation and that is not the case anymore, so the lower-level "app domain timer" implementation needed to handle multiple timer requests.
Some perf numbers:
Initial change:
After change to use thread-locals for more things (monitor lock contention counting in CoreCLR, all of the relevant counting in CoreRT):
Summary:
|
The RT implementation is broken, ignore those perf numbers, I'll fix and retest |
Perf numbers after fixes were similar to before, updated inline above |
@dotnet-bot test this please |
Ping for review please |
@dotnet-bot test Ubuntu arm Cross Checked crossgen_comparison Build and Test |
I have been waiting for API review to be done before looking at this. |
Oh ok, I'll ping that one as well |
The API review was approved. Couple of pending items is:
|
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.
LGTM
Why do you think we need the flush for hill climbing? Any reasonable processor will flush the caches to the main memory at reasonable rate. I would think that it is fine for the hill climbing to miss last few microsecond worth of updates. |
Hill climbing wants to know what the current counts are so that it can respond and having the flush may help it respond sooner. Some processors may take longer than others to sync those updates. I doubt that it would take long enough to sync those updates to be comparable to the hill climbing interval, I'm not sure about that. I don't think it will make a noticeable difference on recent processors, not sure about older ones. |
At the cost of slowing down every processor in the system... |
The hill climbing interval is between 10 and 200 ms, higher when the thread count is already at the min number of threads. I'm not sure if the perf hit would be significant on those time scales. I also doubt that the flush would improve anything, I just kept it because I don't have all of the information on why it was there to begin with. |
Changed my mind again, I'll just remove it. If not having a flush would be an issue it likely would have shown up in a bunch of other places before. |
@dotnet-bot test Windows_NT x64 Checked CoreFX Tests |
Merged in #24113 |
* Implement APIs for some threading metrics (CoreRT) - API review: https://github.com/dotnet/corefx/issues/35500 - May depend on dotnet/coreclr#22754 * Use thread-locals for counting, use finalizer instead of runtime to detect thread exit * Don't let the count properties throw OOM * Remove some flushes
* Implement APIs for some threading metrics (CoreRT) - API review: https://github.com/dotnet/corefx/issues/35500 - May depend on dotnet#22754 * Use thread-locals for counting, use finalizer instead of runtime to detect thread exit * Don't let the count properties throw OOM * Remove some flushes Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Implement APIs for some threading metrics (CoreRT) - API review: https://github.com/dotnet/corefx/issues/35500 - May depend on dotnet/coreclr#22754 * Use thread-locals for counting, use finalizer instead of runtime to detect thread exit * Don't let the count properties throw OOM * Remove some flushes Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Implement APIs for some threading metrics (CoreRT) - API review: https://github.com/dotnet/corefx/issues/35500 - May depend on dotnet/coreclr#22754 * Use thread-locals for counting, use finalizer instead of runtime to detect thread exit * Don't let the count properties throw OOM * Remove some flushes Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Implement APIs for some threading metrics (CoreRT) - API review: https://github.com/dotnet/corefx/issues/35500 - May depend on #22754 * Use thread-locals for counting, use finalizer instead of runtime to detect thread exit * Don't let the count properties throw OOM * Remove some flushes Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Expose and test APIs for some threading metrics (CoreFX) - API review: https://github.com/dotnet/corefx/issues/35500 - Depends on dotnet/coreclr#22754, dotnet/corert#7066 * Separate and expose pending local vs global work item count * Remove local/global variants of PendingWorkItemCount * Remove unrelated test * Add test for a fix to ThreadLocal.Values property throwing NullReferenceException when disposed Fix is in dotnet/corert#7066 * Fix build * Fix test * Add API compat baselines for uapaot * Fix test * Use RemoteExecutor for MetricsTest * Address feedback
…et/coreclr#7066) * Implement APIs for some threading metrics (CoreRT) - API review: https://github.com/dotnet/corefx/issues/35500 - May depend on dotnet/coreclr#22754 * Use thread-locals for counting, use finalizer instead of runtime to detect thread exit * Don't let the count properties throw OOM * Remove some flushes Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com> Commit migrated from dotnet/coreclr@447b655
…fx#37401) * Expose and test APIs for some threading metrics (CoreFX) - API review: https://github.com/dotnet/corefx/issues/35500 - Depends on dotnet/coreclr#22754, dotnet/corert#7066 * Separate and expose pending local vs global work item count * Remove local/global variants of PendingWorkItemCount * Remove unrelated test * Add test for a fix to ThreadLocal.Values property throwing NullReferenceException when disposed Fix is in dotnet/corert#7066 * Fix build * Fix test * Add API compat baselines for uapaot * Fix test * Use RemoteExecutor for MetricsTest * Address feedback Commit migrated from dotnet/corefx@34fe566