Skip to content

Use the portable timer implementation in CoreCLR - #71864

Merged
jkotas merged 1 commit into
dotnet:mainfrom
kouvel:PTimer
Jul 9, 2022
Merged

Use the portable timer implementation in CoreCLR#71864
jkotas merged 1 commit into
dotnet:mainfrom
kouvel:PTimer

Conversation

@kouvel

@kouvel kouvel commented Jul 9, 2022

Copy link
Copy Markdown
Contributor

Fixes #46266

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

ghost commented Jul 9, 2022

Copy link
Copy Markdown

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

kouvel commented Jul 9, 2022

Copy link
Copy Markdown
Contributor Author

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

am11 commented Jul 9, 2022

Copy link
Copy Markdown
Member

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

cc @MichalStrehovsky

@jkotas

jkotas commented Jul 9, 2022

Copy link
Copy Markdown
Member

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

This is switching CoreCLR to what NativeAOT used since forever.

@jkotas jkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice!

@am11

am11 commented Jul 9, 2022

Copy link
Copy Markdown
Member

@jkotas, I meant these defaults:

<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

jkotas commented Jul 9, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

❤️

@davidfowl

Copy link
Copy Markdown
Member

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

@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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use Portable TimerQueue when using Portable ThreadPool

5 participants