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
2 changes: 2 additions & 0 deletions src/System.Private.CoreLib/src/System.Private.CoreLib.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -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

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

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

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

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

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

<Compile Include="System\__Canon.cs" />
<Compile Include="System\AccessViolationException.cs" />
<Compile Include="System\Activator.cs" />
Expand Down
Loading