Skip to content

Use the portable timer implementation in CoreCLR #71864

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
Merged
jkotas merged 1 commit into from
Jul 9, 2022
Merged

Use the portable timer implementation in CoreCLR #71864

jkotas merged 1 commit into from
Jul 9, 2022

Conversation

@kouvel
Copy link
Contributor

@kouvel kouvel commented Jul 9, 2022

Fixes #46266

@kouvel kouvel added this to the 7.0.0 milestone Jul 9, 2022
@kouvel kouvel self-assigned this Jul 9, 2022
@ghost
Copy link

ghost commented Jul 9, 2022

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

Issue Details

Fixes #46266

Author: kouvel
Assignees: kouvel
Labels:

area-System.Threading
Milestone: 7.0.0

@kouvel
Copy link
Contributor Author

kouvel commented Jul 9, 2022

No significant difference in timer microbenchmarks:

Method Toolchain Mean Error StdDev
ShortScheduleAndDispose Before 137.8 ns 4.31 ns 4.96 ns
ShortScheduleAndDispose After 129.9 ns 3.48 ns 4.01 ns
LongScheduleAndDispose Before 131.5 ns 2.60 ns 2.43 ns
LongScheduleAndDispose After 129.9 ns 3.38 ns 3.89 ns
ScheduleManyThenDisposeMany Before 469,028,385.0 ns 10,482,945.31 ns 12,072,175.41 ns
ScheduleManyThenDisposeMany After 450,835,495.0 ns 28,191,768.65 ns 32,465,682.70 ns
ShortScheduleAndDisposeWithFiringTimers Before 153.8 ns 6.07 ns 6.24 ns
ShortScheduleAndDisposeWithFiringTimers After 160.6 ns 4.80 ns 5.33 ns
SynchronousContention Before 4,334,738,207.1 ns 26,386,498.36 ns 23,390,943.51 ns
SynchronousContention After 4,332,649,160.0 ns 26,337,511.15 ns 24,636,124.27 ns
AsynchronousContention Before 4,333,695,664.3 ns 12,912,192.76 ns 11,446,322.56 ns
AsynchronousContention After 4,338,598,086.7 ns 12,938,144.86 ns 12,102,348.73 ns

@am11
Copy link
Member

am11 commented Jul 9, 2022

Do we want to update NativeAOT's project file too? src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj

cc @MichalStrehovsky

@jkotas
Copy link
Member

jkotas commented Jul 9, 2022

Do we want to update NativeAOT's project file too?

This is switching CoreCLR to what NativeAOT used since forever.

jkotas
jkotas approved these changes Jul 9, 2022
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Nice!

@am11
Copy link
Member

am11 commented Jul 9, 2022

@jkotas, I meant these defaults:

runtime/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj

Lines 28 to 35 in a4b5376

<PropertyGroup>
<FeaturePortableThreadPool Condition="'$(FeaturePortableThreadPool)' == ''">false</FeaturePortableThreadPool>
<FeaturePortableThreadPool Condition="'$(TargetsUnix)' == 'true'">true</FeaturePortableThreadPool>
</PropertyGroup>
<PropertyGroup>
<FeaturePortableTimer Condition="'$(FeaturePortableTimer)' == ''">false</FeaturePortableTimer>
<FeaturePortableTimer Condition="'$(TargetsUnix)' == 'true'">true</FeaturePortableTimer>
</PropertyGroup>
on windows, it is false.

@jkotas
Copy link
Member

jkotas commented Jul 9, 2022 •
edited
Loading

NativeAOT is using native Windows OS threadpool and timers on Windows. We may want to change it, but not in this PR. It is not obvious that it would be an improvement.

@jkotas jkotas merged commit 2d41986 into dotnet:main Jul 9, 2022
@davidfowl
Copy link
Member

davidfowl commented Jul 9, 2022

❤️

@davidfowl
Copy link
Member

davidfowl commented Jul 9, 2022

The only nit would be to remove the comment saying that this is a Unix specific implementation of the timer queue

@jkotas jkotas mentioned this pull request Jul 11, 2022
Merged
@kouvel kouvel deleted the PTimer branch July 11, 2022 16:56
@ghost ghost locked as resolved and limited conversation to collaborators Aug 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotas jkotas jkotas approved these changes

@AntonLapounov AntonLapounov Awaiting requested review from AntonLapounov

@eduardo-vp eduardo-vp Awaiting requested review from eduardo-vp

@mangod9 mangod9 Awaiting requested review from mangod9

+1 more reviewer

@filipnavara filipnavara filipnavara approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@kouvel kouvel

Labels

area-System.Threading

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

Use Portable TimerQueue when using Portable ThreadPool

5 participants

@kouvel @am11 @jkotas @davidfowl @filipnavara