Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

Scheduler improvements #2026

Merged
merged 5 commits into from
Jan 11, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ internal SocketConnection(Socket socket, MemoryPool<byte> pool)

// TODO: Make this configurable
// Dispatch to avoid deadlocks
_input = new Pipe(new PipeOptions(pool, Scheduler.TaskRun, Scheduler.TaskRun));
_output = new Pipe(new PipeOptions(pool, Scheduler.TaskRun, Scheduler.TaskRun));
_input = new Pipe(new PipeOptions(pool, Scheduler.ThreadPool, Scheduler.ThreadPool));
_output = new Pipe(new PipeOptions(pool, Scheduler.ThreadPool, Scheduler.ThreadPool));

_receiveTask = ReceiveFromSocketAndPushToWriterAsync();
_sendTask = ReadFromReaderAndWriteToSocketAsync();
Expand Down
2 changes: 1 addition & 1 deletion src/System.IO.Pipelines/System.IO.Pipelines.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

In this case yes.

<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<PackageIconUrl>http://go.microsoft.com/fwlink/?linkid=833199</PackageIconUrl>
<!--<DefineConstants Condition="'$(Configuration)' == 'Debug'">$(DefineConstants);BLOCK_LEASE_TRACKING</DefineConstants>-->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

namespace System.Threading
{
internal class InlineScheduler : Scheduler
internal sealed class InlineScheduler : Scheduler
{
public override void Schedule(Action action)
{
Expand Down
7 changes: 5 additions & 2 deletions src/System.IO.Pipelines/System/Threading/Scheduler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@ namespace System.Threading
{
public abstract class Scheduler
{
public static Scheduler TaskRun { get; } = new TaskRunScheduler();
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because you said so here #2026 (review)

Copy link
Member

Choose a reason for hiding this comment

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

Should have been more specific, why the syntax change.

Copy link
Member Author

@benaadams benaadams Jan 11, 2018

Choose a reason for hiding this comment

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

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.

public static Scheduler Inline { get; } = new InlineScheduler();
private static ThreadPoolScheduler _threadPoolScheduler = new ThreadPoolScheduler();
private static InlineScheduler _inlineScheduler = new InlineScheduler();

public static Scheduler ThreadPool => _threadPoolScheduler;
public static Scheduler Inline => _inlineScheduler;

public abstract void Schedule(Action action);
public abstract void Schedule(Action<object> action, object state);
Expand Down
21 changes: 0 additions & 21 deletions src/System.IO.Pipelines/System/Threading/TaskRunScheduler.cs

This file was deleted.

83 changes: 83 additions & 0 deletions src/System.IO.Pipelines/System/Threading/ThreadPoolScheduler.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Threading.Tasks;

namespace System.Threading
{
internal sealed class ThreadPoolScheduler : Scheduler
{
public override void Schedule(Action action)
{
#if NETCOREAPP2_1
// Queue to low contention local ThreadPool queue; rather than global queue as per Task
Threading.ThreadPool.QueueUserWorkItem(_actionAsTask, action, preferLocal: true);
#elif NETSTANDARD2_0
Threading.ThreadPool.QueueUserWorkItem(_actionAsTask, action);
#else
Task.Factory.StartNew(action);
Copy link
Member

@davidfowl davidfowl Jan 11, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

#endif
}

public override void Schedule(Action<object> action, object state)
{
#if NETCOREAPP2_1
// Queue to low contention local ThreadPool queue; rather than global queue as per Task
Threading.ThreadPool.QueueUserWorkItem(_actionObjectAsTask, new ActionObjectAsTask(action, state), preferLocal: true);
#elif NETSTANDARD2_0
Threading.ThreadPool.QueueUserWorkItem(_actionObjectAsTask, new ActionObjectAsTask(action, state));
#else
Task.Factory.StartNew(action, state);
#endif
}

#if NETCOREAPP2_1 || NETSTANDARD2_0
// Catches only the exception into a failed Task, so the fire-and-forget action
// can be queued directly to the ThreadPool without the extra overhead of as Task
private readonly static WaitCallback _actionAsTask = state =>
{
try
{
((Action)state)();
}
catch (Exception ex)
Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

private readonly static WaitCallback _actionObjectAsTask = state => ((ActionObjectAsTask)state).Run();

private sealed class ActionObjectAsTask
{
private Action<object> _action;
private object _state;

public ActionObjectAsTask(Action<object> action, object state)
{
_action = action;
_state = state;
}

// Catches only the exception into a failed Task, so the fire-and-forget action
// can be queued directly to the ThreadPool without the extra overhead of as Task
public void Run()
{
try
{
_action(_state);
}
catch (Exception ex)
{
// Create faulted Task for the TaskScheulder to handle exception
// rather than letting it escape onto the ThreadPool and crashing the process
Task.FromException(ex);
Copy link
Member

@davidfowl davidfowl Jan 11, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

}
}
}
#endif
}
}