Skip to content
This repository has been archived by the owner on Nov 1, 2020. It is now read-only.

[Unix ThreadPool] Ported Hill Climbing from CoreCLR to CoreRT #3794

Merged
merged 13 commits into from
Jun 8, 2017
Merged

[Unix ThreadPool] Ported Hill Climbing from CoreCLR to CoreRT #3794

merged 13 commits into from
Jun 8, 2017

Conversation

jkoritzinsky
Copy link
Member

Port the Hill climbing algorithm (used for determining how many threads to have in the thread pool) from CoreCLR to CoreRT.

@kouvel

@dnfclas
Copy link

dnfclas commented Jun 5, 2017 •
edited by kouvel
Loading

@jkoritzinsky,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot #Closed

jkotas
jkotas reviewed Jun 5, 2017
View reviewed changes
src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.Unix.cs Outdated
@@ -0,0 +1,380 @@
namespace System.Threading
Copy link
Member

@jkotas jkotas Jun 5, 2017 •
edited by kouvel
Loading

Choose a reason for hiding this comment

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

Could you please add license headers the new files? #Closed

jkotas
jkotas reviewed Jun 5, 2017
View reviewed changes
src/System.Private.CoreLib/src/System.Private.CoreLib.csproj Outdated
@@ -170,6 +170,8 @@
<Compile Include="System\Resources\ResourceSet.cs" />
<Compile Include="System\Runtime\InteropServices\CriticalHandle.cs" />
<Compile Include="System\Text\CodePageDataItem.cs" />
<Compile Include="System\Threading\ClrThreadPool.Unix.cs" />
<Compile Include="System\Threading\ClrThreadPool.HillClimbing.Unix.cs" />
Copy link
Member

@jkotas jkotas Jun 5, 2017 •
edited by kouvel
Loading

Choose a reason for hiding this comment

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

Is there anything Unix-specific in the file? Maybe call it just ClrThreadPool.HillClimbing.cs ? #Closed

Copy link
Member Author

@jkoritzinsky jkoritzinsky Jun 5, 2017 •
edited by kouvel
Loading

Choose a reason for hiding this comment

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

In this PR there isn't anything Unix-specific; however, in future PRs the rest of the thread pool implementation will have Unix-specific portions. The CPU utilization calculation specifically is the Unix-specific code that Hill climbing will touch.

I can change it if you would like me to do so.
#Closed

Copy link
Contributor

@sergiy-k sergiy-k Jun 6, 2017 •
edited by kouvel
Loading

Choose a reason for hiding this comment

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

Personally, I would like to see the code structured in such a way that would be easy to enable ClrThreadPool on Windows too if it is needed (for example, for experiential/perf comparison purposes). #Closed

Copy link
Member Author

@jkoritzinsky jkoritzinsky Jun 6, 2017 •
edited by kouvel
Loading

Choose a reason for hiding this comment

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

I've moved the code to be available on both Windows and Unix. #Closed

Copy link
Member

@jkotas jkotas Jun 6, 2017 •
edited by kouvel
Loading

Choose a reason for hiding this comment

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

We want the code to be structured such that it is possible to enable it on Windows. You have done a step towards that by renaming the files from *.Unix.cs to *.cs.

But we do not want to be compiling the code in by default on Windows, at least for now. Could you please add Condition="'$(TargetsWindows)'!='true' on the new files in the .csproj file? #Closed

kouvel
kouvel previously requested changes Jun 6, 2017
View reviewed changes
src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.cs Outdated

public static bool SetMaxThreads(int threads)
{
if (threads < ThreadPoolGlobals.processorCount || threads < s_minThreads)
Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The max thread count can be set to a value less than the processor count, just not zero #Closed

src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.cs Outdated

public static bool SetMinThreads(int threads)
{
if (threads < 0 || threads > s_maxThreads)
Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This work and the work in SetMaxThreads needs to happen inside a lock. If you're planning on doing the initialization stuff in a separate PR, it would be good to add a TODO so that this is not missed #Closed

src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.cs Outdated
private static int s_cpuUtilization = 85; // TODO: Add calculation for CPU utilization

private static int s_minThreads;
private static int s_maxThreads;
Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Please add a TODO to initialize these values #Closed

src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated
internal static partial class ClrThreadPool
{
// Config values pulled from CoreCLR
private static readonly HillClimbing s_threadPoolHillClimber = new HillClimbing(4, 20, 100, 8, 15, 300, 4, 20, 10, 200, 1, 200, 15);
Copy link
Member

Choose a reason for hiding this comment

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

Since there would only be one instance of HillClimbing, could put a public static readonly HillClimbing Instance field in the HillClimbing class itself, or could just make HillClimbing a static class

src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated

private class HillClimbing
{
public enum State
Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Does this need to be public? #Closed

Copy link
Member Author

@jkoritzinsky jkoritzinsky Jun 6, 2017 •
edited by kouvel
Loading

Choose a reason for hiding this comment

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

This needs to be the same accessibility as ForceChange below, which is public. This isn't actually used to track internal state. It's only used in logging states (which is currently TODO stubbed out). #Closed

Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Oh right, makes sense #Closed

src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated
_samples = new double[_samplesToMeasure];
_threadCounts = new double[_samplesToMeasure];

_currentSampleInterval = _randomIntervalGenerator.Next(_sampleIntervalLow, _sampleIntervalHigh);
Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

_sampleIntervalHigh + 1? #Closed

src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated
//
// Update the cumulative stats for this thread count
//
_secondsElapsedSinceLastChange += sampleDuration;
Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Similar to what you did for _secondsElapsedSinceLastChange it would be nice to indicate the units for sampleDuration and other such variables in its name #Closed

src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated
// Add the current thread count and throughput sample to our history
//
double throughput = numCompletions / sampleDuration;
//Worker Thread Adjustment Sample event
Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Please prefix TODOs with "TODO: " so that we can distinguish them from regular comments #Closed

src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated
//
// Set up defaults for our metrics
//
(double real, double imaginary) threadWaveComponent = (0, 0);
Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think it would be better to have a Complex struct that simplifies the math on these values, as it is done in CoreCLR #Closed

src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated
throughputWaveComponent = GetWaveComponent(_samples, sampleCount, _wavePeriod);
throughputWaveComponent = (throughputWaveComponent.real / averageThroughput, throughputWaveComponent.imaginary / averageThroughput);
(double real, double imaginary) intermediateErrorEstimate = GetWaveComponent(_samples, sampleCount, adjacentPeriod1);
throughputErrorEstimate = Abs((intermediateErrorEstimate.real / averageThroughput, intermediateErrorEstimate.imaginary / averageThroughput));
Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Abs could be a member of Complex #Closed

kouvel
kouvel reviewed Jun 6, 2017
View reviewed changes
src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated
// Record these numbers for posterity
//

// Worker Thread Adjustment stats event
Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

TODO: prefix please #Closed

kouvel
kouvel reviewed Jun 6, 2017
View reviewed changes
src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated

private class HillClimbing
{
public enum State
Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Does this need to be public? #Closed

Copy link
Member Author

@jkoritzinsky jkoritzinsky Jun 6, 2017 •
edited by kouvel
Loading

Choose a reason for hiding this comment

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

It needs to be public because it is a parameter of ForceChange, which is how the thread pool tells hill climbing that it made a mistake and fixes it. #Closed

kouvel
kouvel reviewed Jun 6, 2017
View reviewed changes
src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated

private class HillClimbing
{
public enum State
Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The naming feels a bit odd because some of these are states of hill climbing, and some are reasons for changing the thread count (one-time transitions, not a state). Maybe call this StateOrTransition or something like that? Any call to ChangeThreadCount should be given a transition value. May also split the states from the transitions into separate enums. #Closed

kouvel
kouvel reviewed Jun 6, 2017
View reviewed changes
src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated
}

private static (double real, double imaginary) GetWaveComponent(double[] samples, int numSamples, double period)
{
Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Would be good to add the asserts from the original sources as well. Also generally if you see any other opportunities for adding useful asserts, it would be nice to add them even though it may create some additional burden on getting things working, as it will validate your understanding of the code and potentially find issues. #Closed

kouvel
kouvel reviewed Jun 6, 2017
View reviewed changes
src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated
double cos = Math.Cos(w);
double coeff = 2 * cos;
double q0 = 0, q1 = 0, q2 = 0;
for(int i = 0; i < numSamples && i < samples.Length; ++i)
Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Could just have i < numSamples as the loop condition and assert in the body that i < samples.Length. An invalid i would trigger an assert that would be easier to debug than an exception, and we would get an exception from the inherent bound check anyway. #Closed

Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Actually the indexing seems to be different from the original code. It looks like it's trying to use the most recent numSamples samples from a circular buffer of samples. #Closed

kouvel
kouvel reviewed Jun 6, 2017
View reviewed changes
src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated
q2 = q1;
q1 = q0;
}
return ((q1 - q2 * cos) / samples.Length, (q2 * Math.Sin(w)) / samples.Length);
Copy link
Member

@kouvel kouvel Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

samples.Length => numSamples? #Closed

kouvel
kouvel reviewed Jun 7, 2017
View reviewed changes
src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated
public double Imaginary { get; }
public double Real { get; }

public static Complex operator*(double scalar, Complex complex) => new Complex(scalar * complex.Real, scalar * complex.Imaginary);
Copy link
Member

@kouvel kouvel Jun 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

double scalar, Complex complex

Since multiply is commutative, it would be good to provide an overload (even if it's not used) that takes parameters in the opposite order and calls into this overload

kouvel
kouvel reviewed Jun 7, 2017
View reviewed changes
src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated
return new Complex((lhs.Real * rhs.Real + lhs.Imaginary * rhs.Imaginary) / denom, (-lhs.Real * rhs.Imaginary + lhs.Imaginary * rhs.Real) / denom);
}

public static double Abs(Complex complex) => Math.Sqrt(complex.Real * complex.Real + complex.Imaginary * complex.Imaginary);
Copy link
Member

@kouvel kouvel Jun 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Probably better to make this an instance method

kouvel
kouvel reviewed Jun 7, 2017
View reviewed changes
src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated

private class HillClimbing
{
private struct Complex
Copy link
Member

Choose a reason for hiding this comment

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

nit: I usually prefer to put small helper class at the bottom of the scope (or moderately large ones in a separate file) so that it doesn't get in the way of the main functionality of this file, as these small things can sometimes add up to pages of code that is usually not of interest

kouvel
kouvel reviewed Jun 7, 2017
View reviewed changes
src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs Outdated
q2 = q1;
q1 = q0;
}
return new Complex((q1 - q2 * cos) / numSamples, (q2 * Math.Sin(w)) / numSamples);
Copy link
Member

Choose a reason for hiding this comment

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

This could be slightly simplified to use the Complex's divide operator

kouvel
kouvel reviewed Jun 7, 2017
View reviewed changes
src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.cs Outdated
@@ -0,0 +1,47 @@
using System.Diagnostics;
Copy link
Member

Choose a reason for hiding this comment

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

Please add the license header to this file as well

@kouvel kouvel dismissed their stale review June 7, 2017 01:18

Resolved

@kouvel
Copy link
Member

kouvel commented Jun 7, 2017 •
edited
Loading

CC @AntonLapounov #Closed

@kouvel
Copy link
Member

kouvel commented Jun 7, 2017 •
edited
Loading

I'm seeing this in the builds:

14:31:09   Unhandled Exception: System.Exception: JIT Failed
14:31:09      at Internal.JitInterface.CorInfoImpl.CompileMethod(IMethodCodeNode methodCodeNodeNeedingCode, MethodIL methodIL)
14:31:09      at ILCompiler.RyuJitCompilation.ComputeDependencyNodeDependencies(List`1 obj)
14:31:09      at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.ComputeDependencies(List`1 deferredStaticDependencies)
14:31:09      at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.ComputeMarkedNodes()
14:31:09      at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.get_MarkedNodeList()
14:31:09      at ILCompiler.RyuJitCompilation.CompileInternal(String outputFile, ObjectDumper dumper)
14:31:09      at ILCompiler.Compilation.ILCompiler.ICompilation.Compile(String outputFile, ObjectDumper dumper)
14:31:09      at ILCompiler.Program.Run(String[] args)
14:31:09      at ILCompiler.Program.Main(String[] args)

Not sure what that is about #Closed

@jkotas
Copy link
Member

jkotas commented Jun 7, 2017 •
edited by kouvel
Loading

This was caused by NuGet snafu. It should be fixed now. #Closed

@jkotas
Copy link
Member

jkotas commented Jun 7, 2017 •
edited by kouvel
Loading

@dotnet-bot test this please #Closed

@mattwarren
Copy link
Contributor

This is so cool, I just wish you'd done it a few months ago 😄 that way I wouldn't have needed to mess around with C++ code when I wrote The CLR Thread Pool 'Thread Injection' Algorithm!

These might be dumb questions, as I don't know much about CoreRT. But what was the reason for porting it, can CoreRT not use .cpp code? Also, could the port be copied back into the CoreCLR, so that there was only one (C#) version of the code and any fixes/improvements could be shared?

@MichalStrehovsky
Copy link
Member

These might be dumb questions, as I don't know much about CoreRT. But what was the reason for porting it, can CoreRT not use .cpp code?

I can't speak for CoreCLR, but if something can be written in C#, we prefer to have it in C# in CoreRT. We only have very little unmanaged code in this repo (and the majority of it is the garbage collector). We simply trust that managed code is superior to unmanaged code for most scenarios (reliability, maintenance costs, etc.). If we find a deficiency, we are in a great position to fix it - and make it better for everyone writing managed code in the process.

@kouvel
Copy link
Member

kouvel commented Jun 8, 2017

@mattwarren thanks for writing that article! I'll have to read through it more closely later.

Also, could the port be copied back into the CoreCLR, so that there was only one (C#) version of the code and any fixes/improvements could be shared?

Yes the idea is to move the C# code back to CoreCLR eventually, in the interest of maintenance and performance. Having code in C# also helps avoid a lot of p/invokes.

@jkotas
Copy link
Member

jkotas commented Jun 8, 2017

In other words, the CoreRT is C# runtime written in C#. It is similar to choices made by other runtimes: Swift runtime is written in Switch, Go runtime is written in Go, ... .

An interesting thing to note is that this approach requires AOT (for the base system at least). Otherwise, the system would not be able to startup. CoreCLR does not require AOT for the base system. It means that it cannot have as much code written in C#. The good chunk of CoreCLR startup path has to be C++.

reliability, maintenance costs, etc.

For example, the bug-prone glue that allows writing "manually managed code" in CoreCLR like HELPER_METHOD_FRAME or GC_PROTECT does not exist in CoreRT. Many man years were spent tracing down the bugs in the manually managed code, and there are still quite a few left for sure.

We have scheme in place to allow sharing CoreLib code between CoreCLR and CoreRT: https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/shared/README.md. We are migrating code into this shared partition over time to reduce our maintenance costs. We are at about ~40% shared. We believe that we can get to ~80% shared without unnatural acts. The threadpool rewritten in C# is certainly a good candidate for getting migrated over to the shared partition once ready.

@kouvel kouvel merged commit 71fd32d into dotnet:master Jun 8, 2017
@jkoritzinsky jkoritzinsky deleted the ClrThreadPoolHillClimbing branch June 8, 2017 20:43
@mattwarren
Copy link
Contributor

Glad you liked the blog post.

Funnily enough, one of the readers of the post also did their own port to C#, see https://github.com/cklutz/HillClimbing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sergiy-k sergiy-k sergiy-k left review comments

@jkotas jkotas jkotas left review comments

@kouvel kouvel kouvel approved these changes

Assignees
No one assigned
Labels
cla-already-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jkoritzinsky @dnfclas @kouvel @jkotas @mattwarren @MichalStrehovsky @sergiy-k