NotificationsYou must be signed in to change notification settings
Fork5.1k
Star16.6k

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy 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 intodotnet:mainfromkouvel:PTimer

Jul 9, 2022

Merged

Use the portable timer implementation in CoreCLR#71864

jkotas merged 1 commit intodotnet:mainfromkouvel:PTimer

Jul 9, 2022

Conversation

Copy link

Contributor

kouvel commentedJul 9, 2022

Fixes#46266

Use the portable timer implementation in CoreCLR

4777f90

Fixes#46266

kouvel added the area-System.Threading label

Jul 9, 2022

kouvel added this to the7.0.0 milestone

Jul 9, 2022

kouvel requested review fromAntonLapounov,eduardo-vp andmangod9

July 9, 2022 00:07

kouvel self-assigned this

Jul 9, 2022

Copy link

ghost commentedJul 9, 2022

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

Issue Details

Fixes#46266

Author:	kouvel
Assignees:	kouvel
Labels:	`area-System.Threading`
Milestone:	7.0.0

Copy link

ContributorAuthor

kouvel commentedJul 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

Copy link

Member

am11 commentedJul 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

Copy link

Member

jkotas commentedJul 9, 2022

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

This is switching CoreCLR to what NativeAOT used since forever.

jkotas approved these changes

Jul 9, 2022

View reviewed changes

Copy link

Member

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!

Copy link

Member

am11 commentedJul 9, 2022

@jkotas, I meant these defaults:

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

Lines 28 to 35 ina4b5376

	<PropertyGroup>
	<FeaturePortableThreadPoolCondition="'$(FeaturePortableThreadPool)' == ''">false</FeaturePortableThreadPool>
	<FeaturePortableThreadPoolCondition="'$(TargetsUnix)' == 'true'">true</FeaturePortableThreadPool>
	</PropertyGroup>
	<PropertyGroup>
	<FeaturePortableTimerCondition="'$(FeaturePortableTimer)' == ''">false</FeaturePortableTimer>
	<FeaturePortableTimerCondition="'$(TargetsUnix)' == 'true'">true</FeaturePortableTimer>
	</PropertyGroup>

on windows, it isfalse.

Copy link

Member

jkotas commentedJul 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.