Enable multicore JIT in the compilers #23173

agocke · 2017-11-15T01:00:08Z

Here is the output from my perf testing tool currently in review that
shows a significant difference in csc.exe after the change:

// * Detailed results *
PlaceholderBenchmarkRunner.PlaceholderMethod: Job-TGYVCW(Toolchain=Perf.ExternalProcessToolchain, LaunchCount=0, RunStrategy=Monitoring, TargetCount=25, WarmupCount=0) [Commit=HEAD^]
Runtime = ; GC =
Mean = 4.8018 s, StdErr = 0.0215 s (0.45%); N = 25, StdDev = 0.1073 s
Min = 4.6717 s, Q1 = 4.7297 s, Median = 4.7892 s, Q3 = 4.8438 s, Max = 5.1998 s
IQR = 0.1141 s, LowerFence = 4.5585 s, UpperFence = 5.0149 s
ConfidenceInterval = [4.7214 s; 4.8822 s] (CI 99.9%), Margin = 0.0804 s (1.67% of Mean)
Skewness = 1.87, Kurtosis = 7.98

PlaceholderBenchmarkRunner.PlaceholderMethod: Job-TGYVCW(Toolchain=Perf.ExternalProcessToolchain, LaunchCount=0, RunStrategy=Monitoring, TargetCount=25, WarmupCount=0) [Commit=HEAD]
Runtime = ; GC =
Mean = 3.6409 s, StdErr = 0.0650 s (1.78%); N = 25, StdDev = 0.3249 s
Min = 3.4381 s, Q1 = 3.5510 s, Median = 3.5781 s, Q3 = 3.6189 s, Max = 5.1720 s
IQR = 0.0679 s, LowerFence = 3.4491 s, UpperFence = 3.7209 s
ConfidenceInterval = [3.3975 s; 3.8842 s] (CI 99.9%), Margin = 0.2433 s (6.68% of Mean)
Skewness = 4.16, Kurtosis = 19.76

Total time: 00:07:11 (431.13 sec)

// * Summary *

BenchmarkDotNet=v0.10.9, OS=Windows 10 Redstone 1 (10.0.14393)
Processor=Intel Xeon CPU E5645 2.40GHz, ProcessorCount=24
Frequency=2337892 Hz, Resolution=427.7358 ns, Timer=TSC
.NET Core SDK=2.1.1-preview-007094
  [Host] : .NET Core 2.0.0 (Framework 4.6.00001.0), 64bit RyuJIT

Toolchain=Perf.ExternalProcessToolchain  LaunchCount=0  RunStrategy=Monitoring
TargetCount=25  WarmupCount=0

            Method | Commit |    Mean |    Error |   StdDev | Rank |
------------------ |------- |--------:|---------:|---------:|-----:|
 PlaceholderMethod |   HEAD | 3.641 s | 0.2433 s | 0.3249 s |    1 |
 PlaceholderMethod |  HEAD^ | 4.802 s | 0.0804 s | 0.1073 s |    2 |

// * Legends *
  Commit : Value of the 'Commit' parameter
  Mean   : Arithmetic mean of all measurements
  Error  : Half of 99.9% confidence interval
  StdDev : Standard deviation of all measurements
  Rank   : Relative position of current benchmark mean among all benchmarks (Arabic style)
  1 s    : 1 Second (1 sec)

Pilchie · 2017-11-15T01:10:10Z

But are both of those runs are with compilers that are neither ngen'd or crossgen'd

agocke · 2017-11-15T17:44:57Z

@Pilchie that is true. Do you think a full matrix is necessary?

Pilchie · 2017-11-15T19:38:11Z

To be clear, I'm not against the change, I just don't think we will see nearly as much of an effect in practice.

agocke · 2017-11-15T19:53:02Z

@Pilchie Somewhat true. Notably, if we override the compiler via the Microsoft.NETCore.Compilers NuGet package, those binaries never get crossgened, so this is pretty representative of that scenario (without the xplat compiler server).

Pilchie · 2017-11-15T20:01:15Z

Fair point. Like I said, I'm not against this change :) There is also some support for Tiered JIT in .NET Core that we should evaluate for similar scenarios. I think it's still experimental, but it might be worth the experiment.

agocke · 2017-11-15T23:16:56Z

test windows_release_unit32_prtest please

agocke · 2017-11-15T23:21:21Z

I've looked into getting crossgen running -- it's quite a bit of work. I'd recommend not blocking this PR. I'm going to open an issue on CLI to get better support for this.

Pilchie · 2017-11-16T00:08:07Z

Fine with me :)

agocke · 2017-11-16T18:43:36Z

ping @dotnet/roslyn-compiler for review

AlekseyTs · 2017-11-17T18:34:28Z

src/Compilers/Core/CommandLine/ConsoleUtil.cs

@@ -3,6 +3,8 @@
 using Microsoft.CodeAnalysis;
 using System;
 using System.IO;
+using System.Reflection;
+using System.Runtime;


It looks like the only change is addition of usings. Consider reverting changes to this file, if the usings are not required.

AlekseyTs · 2017-11-17T18:37:25Z

src/Compilers/Shared/BuildClient.cs

+                    out bool hasShared,
+                    out string keepAlive,
+                    out string sessionKey,
+                    out string errorMessage))


out string errorMessage)) [](start = 20, length = 25)

out string errorMessage)) [](start = 20, length = 25)

IMHO, changes like that do not make code more readable. I would say quite the opposite. #Closed

AlekseyTs · 2017-11-17T18:43:03Z

src/Compilers/Shared/BuildClient.cs

            {
                textWriter.WriteLine(errorMessage);
                return RunCompilationResult.Failed;
            }

+            if (!TryEnableMulticoreJitting(out errorMessage))
+            {
+                textWriter.WriteLine(errorMessage);


textWriter [](start = 16, length = 10)

textWriter [](start = 16, length = 10)

It looks like the textWriter can be null. Why is it safe to call WriteLine of off it? #Closed

I don't think this textWriter can ever be null. We check for null at the top of the file and, if it is, we set it to Console.Out, which can never be null.

AlekseyTs · 2017-11-17T18:44:49Z

src/Compilers/Shared/BuildClient.cs

@@ -86,6 +89,37 @@ internal RunCompilationResult RunCompilation(IEnumerable<string> originalArgumen
            return new RunCompilationResult(exitCode);
        }

+        private static bool TryEnableMulticoreJitting(out string errorMessage)


out string errorMessage [](start = 54, length = 23)

Consider passing the TextWriter and writing to it instead. #Closed

I'd prefer to centralize access to I/O as much as possible. I'm not even that pleased at directly accessing a TextWriter in this method, but due to the multitargeting and reference restrictions there isn't really a suitable abstraction I could put this in.

AlekseyTs · 2017-11-17T18:58:23Z

src/Compilers/Shared/BuildClient.cs

+                // Enable multi-core JITing
+                // https://blogs.msdn.microsoft.com/dotnet/2012/10/18/an-easy-solution-for-improving-app-launch-performance/
+                var profileRoot = Path.Combine(
+                    Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData),


LocalApplicationData [](start = 72, length = 20)

Is this a per user folder? Is there an alternative that can be shared across all users? #Closed

It is a per-user folder. Shared temporary storage isn't usually accessible for non-admin users. I could see this being a security risk, so I think we should be conservative for now.

AlekseyTs · 2017-11-17T19:06:02Z

Done with review pass (iteration 1). #Closed

Here is the output from my perf testing tool currently in review that shows a significant difference in csc.exe after the change: ``` // * Detailed results * PlaceholderBenchmarkRunner.PlaceholderMethod: Job-TGYVCW(Toolchain=Perf.ExternalProcessToolchain, LaunchCount=0, RunStrategy=Monitoring, TargetCount=25, WarmupCount=0) [Commit=HEAD^] Runtime = ; GC = Mean = 4.8018 s, StdErr = 0.0215 s (0.45%); N = 25, StdDev = 0.1073 s Min = 4.6717 s, Q1 = 4.7297 s, Median = 4.7892 s, Q3 = 4.8438 s, Max = 5.1998 s IQR = 0.1141 s, LowerFence = 4.5585 s, UpperFence = 5.0149 s ConfidenceInterval = [4.7214 s; 4.8822 s] (CI 99.9%), Margin = 0.0804 s (1.67% of Mean) Skewness = 1.87, Kurtosis = 7.98 PlaceholderBenchmarkRunner.PlaceholderMethod: Job-TGYVCW(Toolchain=Perf.ExternalProcessToolchain, LaunchCount=0, RunStrategy=Monitoring, TargetCount=25, WarmupCount=0) [Commit=HEAD] Runtime = ; GC = Mean = 3.6409 s, StdErr = 0.0650 s (1.78%); N = 25, StdDev = 0.3249 s Min = 3.4381 s, Q1 = 3.5510 s, Median = 3.5781 s, Q3 = 3.6189 s, Max = 5.1720 s IQR = 0.0679 s, LowerFence = 3.4491 s, UpperFence = 3.7209 s ConfidenceInterval = [3.3975 s; 3.8842 s] (CI 99.9%), Margin = 0.2433 s (6.68% of Mean) Skewness = 4.16, Kurtosis = 19.76 Total time: 00:07:11 (431.13 sec) // * Summary * BenchmarkDotNet=v0.10.9, OS=Windows 10 Redstone 1 (10.0.14393) Processor=Intel Xeon CPU E5645 2.40GHz, ProcessorCount=24 Frequency=2337892 Hz, Resolution=427.7358 ns, Timer=TSC .NET Core SDK=2.1.1-preview-007094 [Host] : .NET Core 2.0.0 (Framework 4.6.00001.0), 64bit RyuJIT Toolchain=Perf.ExternalProcessToolchain LaunchCount=0 RunStrategy=Monitoring TargetCount=25 WarmupCount=0 Method | Commit | Mean | Error | StdDev | Rank | ------------------ |------- |--------:|---------:|---------:|-----:| PlaceholderMethod | HEAD | 3.641 s | 0.2433 s | 0.3249 s | 1 | PlaceholderMethod | HEAD^ | 4.802 s | 0.0804 s | 0.1073 s | 2 | // * Legends * Commit : Value of the 'Commit' parameter Mean : Arithmetic mean of all measurements Error : Half of 99.9% confidence interval StdDev : Standard deviation of all measurements Rank : Relative position of current benchmark mean among all benchmarks (Arabic style) 1 s : 1 Second (1 sec) ```

AlekseyTs

LGTM

agocke · 2017-11-29T00:40:26Z

test this please

agocke · 2017-11-29T00:40:49Z

@jaredpar For ask-mode approval

VSadov

LGTM

agocke · 2017-11-30T19:52:23Z

@MeiChin-Tsai for ask-mode approval

MeiChin-Tsai · 2017-11-30T20:14:47Z

Andy, multi-core jit is also guided with training data, correct? if so, how do we intend to keep these IBC data up to date?

Otherwise, approved. However I would consider to train with property NGEN configuration so we carry only necessary IBC data.

agocke · 2017-11-30T20:39:25Z

@MeiChin-Tsai I'm not sure what you're asking. To my knowledge, we regularly produce IBC data for the desktop compiler via OptProf runs before inserting into Visual Studio. Is that sufficient to keep IBC data up-to-date?

MeiChin-Tsai · 2017-11-30T20:49:50Z

@agocke - Let me follow up with Brian Robin off line. I believe that multi-core JIT also uses IBC data to predetermine what gets JIT in the multi-core while user program is running. IBC data is also driving the partial NGEN. When you combine both technology, you will hope that most of JIT time for startup is saved by NGEN. multi-core JIT is trying to cover what is not being NGEN ahead of time. So naively I will assume that they should be different set of IBC data.

MeiChin-Tsai · 2017-12-08T18:26:33Z

@agocke - multi-core jit does not use IBC data to guide but use previous run to guide the background jitting. So you are fine here. thanks.

* dotnet/master: (39 commits) Prefer by-val methods over in methods in overload resolution (dotnet#23122) Update build to account for additional Mono.Cecil assemblies Fix build break Enable multicore JIT in the compilers (dotnet#23173) Separate debugging workspace service and EnC service (dotnet#23630) UseInferredMemberName: use one code style option and share more code (dotnet#23506) Add negative test cases Remove unnecessary Mono.Cecil reference Updated formatting of decompiler license notice Use sentence case for the decompiler legal notice Use ConcurrentDictionary to avoid locks in IsGeneratedCode and HasHiddenRegions Recognize condition with logical negation Refactor so that GetSymbolInfo can be called last Change the code fix to find the expression so that we don't need to unwrap the argument Add check for null argument syntax Fix argument names Formatting adjustments Add VB tests for omitted arguments Add WorkItem attributes Pool the Stopwatch instance used in the analyzer driver ...

Revert "Enable multicore JIT in the compilers (#23173)"

agocke added the Area-Compilers label Nov 15, 2017

agocke requested review from jaredpar and a team November 15, 2017 01:00

jasonmalinowski changed the base branch from post-dev15.5-contrib to master November 17, 2017 17:46

AlekseyTs reviewed Nov 17, 2017

View reviewed changes

agocke force-pushed the multicore-jit branch from 40d422a to c0168e2 Compare November 17, 2017 19:34

AlekseyTs approved these changes Nov 17, 2017

View reviewed changes

VSadov approved these changes Nov 29, 2017

View reviewed changes

MeiChin-Tsai added the Approved to merge label Nov 30, 2017

agocke merged commit eed147d into dotnet:master Dec 8, 2017

agocke deleted the multicore-jit branch December 8, 2017 19:04

agocke mentioned this pull request Jan 4, 2018

Reduce Critical Path JIT Time for csc.exe on .NET Core #10691

Closed

jcouv added this to the 15.6 milestone Jan 9, 2018

jaredpar mentioned this pull request Feb 9, 2018

Intermittent error "csc.dll exited with code 255" #23846

Open

jaredpar added a commit that referenced this pull request Feb 20, 2018

Merge pull request #24945 from agocke/revert-multicore-jit

1bc9334

Revert "Enable multicore JIT in the compilers (#23173)"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable multicore JIT in the compilers #23173

Enable multicore JIT in the compilers #23173

agocke commented Nov 15, 2017

Pilchie commented Nov 15, 2017

agocke commented Nov 15, 2017

Pilchie commented Nov 15, 2017

agocke commented Nov 15, 2017

Pilchie commented Nov 15, 2017

agocke commented Nov 15, 2017

agocke commented Nov 15, 2017

Pilchie commented Nov 16, 2017

agocke commented Nov 16, 2017

AlekseyTs Nov 17, 2017

AlekseyTs Nov 17, 2017 •

edited

Loading

AlekseyTs Nov 17, 2017 •

edited

Loading

agocke Nov 17, 2017

AlekseyTs Nov 17, 2017 •

edited

Loading

agocke Nov 17, 2017

AlekseyTs Nov 17, 2017 •

edited

Loading

agocke Nov 17, 2017

AlekseyTs commented Nov 17, 2017 •

edited

Loading

AlekseyTs left a comment

agocke commented Nov 29, 2017

agocke commented Nov 29, 2017

VSadov left a comment

agocke commented Nov 30, 2017

MeiChin-Tsai commented Nov 30, 2017

agocke commented Nov 30, 2017

MeiChin-Tsai commented Nov 30, 2017

MeiChin-Tsai commented Dec 8, 2017

Enable multicore JIT in the compilers #23173

Enable multicore JIT in the compilers #23173

Conversation

agocke commented Nov 15, 2017

Pilchie commented Nov 15, 2017

agocke commented Nov 15, 2017

Pilchie commented Nov 15, 2017

agocke commented Nov 15, 2017

Pilchie commented Nov 15, 2017

agocke commented Nov 15, 2017

agocke commented Nov 15, 2017

Pilchie commented Nov 16, 2017

agocke commented Nov 16, 2017

AlekseyTs Nov 17, 2017

Choose a reason for hiding this comment

AlekseyTs Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

agocke Nov 17, 2017

Choose a reason for hiding this comment

AlekseyTs Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

agocke Nov 17, 2017

Choose a reason for hiding this comment

AlekseyTs Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

agocke Nov 17, 2017

Choose a reason for hiding this comment

AlekseyTs commented Nov 17, 2017 • edited Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

agocke commented Nov 29, 2017

agocke commented Nov 29, 2017

VSadov left a comment

Choose a reason for hiding this comment

agocke commented Nov 30, 2017

MeiChin-Tsai commented Nov 30, 2017

agocke commented Nov 30, 2017

MeiChin-Tsai commented Nov 30, 2017

MeiChin-Tsai commented Dec 8, 2017

AlekseyTs Nov 17, 2017 •

edited

Loading

AlekseyTs Nov 17, 2017 •

edited

Loading

AlekseyTs Nov 17, 2017 •

edited

Loading

AlekseyTs Nov 17, 2017 •

edited

Loading

AlekseyTs commented Nov 17, 2017 •

edited

Loading