Scheduler improvements #2026

benaadams · 2018-01-06T05:21:23Z

Scheduler Devirtualization

Seal internal types TaskRunScheduler and InlineScheduler
Maintain shared TaskRunScheduler and InlineScheduler in derived typed members; rather than base type
Expose via base type property that will devirtualize on inlining when the Jit sees the derived types (as type is maintained above, and derived type is sealed) JIT: improve return types in cases with spill temps coreclr#15766

Resolves: #2027

Fast Fire-and-Forget Tasks for TaskRunScheduler

Queue directly to ThreadPool; when available, rather than have the extra overhead of a Task which is ignored.
On netcoreapp2.1 use QueueUserWorkItem(preferLocal: true) which is lower contention and more closely matches Task behaviour dotnet/corefx#12442
If the Action fails create a failed Task so it can be captured as an unobserved task exception as per Task.Run

Resolves: #2002

Rename TaskRun -> ThreadPool #2026 (review)

/cc @davidfowl @pakrym

benaadams · 2018-01-06T05:26:56Z

myget seems unhappy

21:23:12   Retrying 'FindPackagesByIdAsyncCore' for source 'https://dotnet.myget.org/F/dotnet-corefxlab/FindPackagesById()?id='Microsoft.NETCore.Compilers''.
21:23:12   Response status code does not indicate success: 503 (Service Unavailable).

davidfowl · 2018-01-06T05:57:44Z

src/System.IO.Pipelines/System/Threading/Scheduler.cs

+        private static TaskRunScheduler _taskRunScheduler = new TaskRunScheduler();
+        private static InlineScheduler _inlineScheduler = new InlineScheduler();
+
+        public static Scheduler TaskRun => _taskRunScheduler;


benaadams · 2018-01-06T06:22:42Z

myget issue https://github.com/dotnet/coreclr/issues/15769

benaadams · 2018-01-06T12:02:31Z

src/System.IO.Pipelines/System/Threading/ThreadPoolScheduler.cs

+            {
+                ((Action)state)();
+            }
+            catch (Exception ex)


Do you want to catch the Exception? Currently assuming so as side-effect is quite bad - however is it always wrapped (for logging etc)

KrzysztofCwalina · 2018-01-10T21:54:33Z

What are we doing with this PR? We have an API review of these APIs on Friday (I think) and it would be good to merge or close this one before.

@benaadams, @pakrym, @terrajobst, @davidfowl

benaadams · 2018-01-11T01:41:35Z

What are we doing with this PR?

Its just shooting yourself in the foot; perf-wise, not to use QueueUserWorkItem over Task.Run if you aren't interested in the Task (especially the preferLocal: true variant).

davidfowl · 2018-01-11T04:34:41Z

src/System.IO.Pipelines/System/Threading/Scheduler.cs

@@ -5,8 +5,11 @@ namespace System.Threading
 {
    public abstract class Scheduler
    {
-        public static Scheduler TaskRun { get; } = new TaskRunScheduler();


Why this change?

Because you said so here #2026 (review)

Should have been more specific, why the syntax change.

Storing it in a Scheduler field means the exact type gets lost for:

Scheduler.ThreadPool.Schedule((() => {})

So it remains a virtual call.

Storing it in an exact type field ThreadPoolScheduler means when Scheduler.ThreadPool inlines the Jit knows exactly what it is and can devitalize it to a direct call.

Won't directly help pipelines as the Pipe stores the configured values in a Scheduler field; but will help anyone calling it directly.

davidfowl · 2018-01-11T05:11:23Z

src/System.IO.Pipelines/System/Threading/ThreadPoolScheduler.cs

+#elif NETSTANDARD2_0
+            Threading.ThreadPool.QueueUserWorkItem(_actionAsTask, action);
+#else
+            Task.Factory.StartNew(action);


Should this just be QueueUserWorkItem (I'm aware it ends up going to the global queue).

Doesn't exist on netstandard1.1 came in at netstandard2.0 😢

davidfowl · 2018-01-11T05:20:22Z

src/System.IO.Pipelines/System.IO.Pipelines.csproj

@@ -2,7 +2,7 @@
  <Import Project="..\..\tools\common.props" />
  <PropertyGroup>
    <Description>An abstraction for doing efficient asynchronous IO</Description>
-    <TargetFramework>netstandard1.1</TargetFramework>
+    <TargetFrameworks>netstandard1.1;netstandard2.0;netcoreapp2.1</TargetFrameworks>


This is probably the most unfortunate part of this change but meh..

Meh indeed. It's in implementation details with zero bearings on the user's experience.

It's in implementation details with zero bearings on the user's experience.

In this case yes.

davidfowl · 2018-01-11T05:20:45Z

src/System.IO.Pipelines/System/Threading/ThreadPoolScheduler.cs

+                {
+                    // Create faulted Task for the TaskScheulder to handle exception
+                    // rather than letting it escape onto the ThreadPool and crashing the process
+                    Task.FromException(ex);


This is pretty atypical. I think it should be left to crash. We don't do this anywhere else.

K, will clean up the code a lot to remove it

benaadams added 2 commits January 6, 2018 05:01

Support Scheduler devirtualization

1d328a2

Fast fire-and-forget Tasks

d32726a

benaadams mentioned this pull request Jan 6, 2018

Fast TaskRunScheduler #1999

Closed

davidfowl reviewed Jan 6, 2018

View reviewed changes

Rename TaskRun -> ThreadPool

cc7549e

benaadams force-pushed the sched-improves branch from 64916f6 to cc7549e Compare January 6, 2018 12:01

benaadams commented Jan 6, 2018

View reviewed changes

joshfree assigned pakrym Jan 8, 2018

benaadams closed this Jan 11, 2018

benaadams reopened this Jan 11, 2018

Fix build

f3ffc2b

davidfowl reviewed Jan 11, 2018

View reviewed changes

Don't catch throw

89322e7

benaadams force-pushed the sched-improves branch from bb36d0f to 89322e7 Compare January 11, 2018 05:54

davidfowl merged commit addc27e into dotnet:master Jan 11, 2018

dotnet deleted a comment from juliusfriedman Jan 11, 2018

dotnet deleted a comment from juliusfriedman Jan 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scheduler improvements #2026

Scheduler improvements #2026

benaadams commented Jan 6, 2018 •

edited

Loading

benaadams commented Jan 6, 2018

davidfowl Jan 6, 2018

benaadams Jan 6, 2018

benaadams commented Jan 6, 2018

benaadams Jan 6, 2018

KrzysztofCwalina commented Jan 10, 2018

benaadams commented Jan 11, 2018 •

edited

Loading

davidfowl Jan 11, 2018

benaadams Jan 11, 2018

davidfowl Jan 11, 2018

benaadams Jan 11, 2018 •

edited

Loading

davidfowl Jan 11, 2018 •

edited

Loading

benaadams Jan 11, 2018

davidfowl Jan 11, 2018

terrajobst Jan 12, 2018

davidfowl Jan 12, 2018

davidfowl Jan 11, 2018 •

edited

Loading

benaadams Jan 11, 2018

benaadams Jan 11, 2018

Scheduler improvements #2026

Scheduler improvements #2026

Conversation

benaadams commented Jan 6, 2018 • edited Loading

benaadams commented Jan 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Jan 6, 2018

Choose a reason for hiding this comment

KrzysztofCwalina commented Jan 10, 2018

benaadams commented Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

davidfowl Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Jan 6, 2018 •

edited

Loading

benaadams commented Jan 11, 2018 •

edited

Loading

benaadams Jan 11, 2018 •

edited

Loading

davidfowl Jan 11, 2018 •

edited

Loading

davidfowl Jan 11, 2018 •

edited

Loading