More work will be done to further bring down the jitting time at startup.

Do you continue in the PR?

After crossgen Microsoft.ApplicationInsights.dll and applying tiered compilation

I don't see Microsoft.ApplicationInsights.dll in the table - no jitting?

I searched "Insights" in our code base and have questions:

1. Do we really need types from the dll in CorePsTypeCatalog.cs? We could remove 239 lines.
2. The dll is used for telemetry. Do we really need load the dll at startup? Seems we could delay the telemetry
3. The telemetry is a static class. We have other static classes. I guess some static classes is initialized at startup (I guess they is initialized at first use time) - does this slow down startup? Make sense to analyze this and try to delay static classes initializations?

If ReadOnlyHashSet was rewrited I think we could look other classes used in PowerShell Core engine. Maybe it won't speed up the startup but increase performance.

Outliers
  PSStartupTime.Pwsh61: Core           -> 2 outliers were removed
  PSStartupTime.WinPS: Core            -> 4 outliers were removed
  PSStartupTime.pwsh62_optimized: Core -> 1 outlier  was  removed

having Outliers it is not good in the test - seems interference from other processes.

Intel Core i7-6700 CPU 3.40GHz (Skylake), 1 CPU, 8 logical and 4 physical cores

I think that an ordinary user computer is 2 physical cores and it would be correct to limit the test to two physical cores. (This may be related to tied compilation)

I restarted CI-MacOs.

@iSazonov Reply to your comment here:

More work will be done to further bring down the jitting time at startup.

Do you continue in the PR?

No, that will be in a new PR. The planned work are as follows:

• Generate PDB for crossgen'ed assemblies that can be used by PerfView/WPA, so that I can do further stack analysis on CPU time.
• Crossgen'ed assemblies still leave a high cost of jitting time comparing with ngen. It's probably by design as ngen is targeting a specific CPU while crossgen is targeting a general x64 architecture, but I need to contact .NET Core team to get better understanding and see if there are other options (profile-guided-opt?)
• Ad-hoc code changes to avoid unnecessary work at startup time.

I don't see Microsoft.ApplicationInsights.dll in the table - no jitting?

After the change, no IL code needs to be jitted for this dll at startup time.

Do we really need types from the dll in CorePsTypeCatalog.cs? We could remove 239 lines.

Yes we do. Type conversion depends on the type catalog. Not all .NET Core assemblies shipped with PS Core are loaded at startup. Today, user can use a type even it's from an assembly that is not loaded yet, because powershell knows which assembly the type is from by looking in the type catalog. Without it, the user needs to know which assembly the type is in, and then load the assembly first before using the type. The type catalog is also used in tab completion, so that powershell can tab complete types that are not loaded yet.

The dll is used for telemetry. Do we really need load the dll at startup? Seems we could delay the telemetry

ApplicationInsight telemetry is done in console.dll at startup, as long as the user hasn't opted out.

The telemetry is a static class. We have other static classes. I guess some static classes is initialized at startup (I guess they is initialized at first use time) - does this slow down startup? Make sense to analyze this and try to delay static classes initializations?

The work that is done in this PR are based on the analysis of Reference Set and Jitting time. Some ad-hoc analysis on the static members might be useful, but I think our efforts should be guided by the measurement data, so our time can be spent on things that matters most.

If ReadOnlyHashSet was rewrited I think we could look other classes used in PowerShell Core engine. Maybe it won't speed up the startup but increase performance.

The reason to change ImmutableHashSet to ReadOnlyHashSet is because this only use of ImmutableHashSet causes System.Collections.Immutable.dll to be loaded at startup, which brings in 74 pages loaded from disk. This shows up as part of the reference set analysis. It might be useful to look at other classes, but again, I think our efforts should be guided by the measurement data.

having Outliers it is not good in the test - seems interference from other processes.

There is no interference from other pwsh processes when running the tests. BenchmarkDotNet starts each process many times, and sometimes there are outliers.

I think that an ordinary user computer is 2 physical cores and it would be correct to limit the test to two physical cores. (This may be related to tied compilation)

The baseline is the same, and here we care most about the percentage of improvement instead of the actual startup time. As for multi-core background jitting, with changes in this PR, only 20.5ms are spent in the multi-core background jitting, very small number comparatively, so I don't think it has noticeable effect on the improvement percentage.

The macOS failure appears to be a test code issue:

2018-11-27T12:24:13.7076000Z Description: Basic os_log tests on MacOS/
2018-11-27T12:24:13.7079830Z Name:        Verifies scriptblock logging
2018-11-27T12:24:13.7084250Z message:
2018-11-27T12:24:13.7088810Z did not recieve at least 18 records but 47 instead.
2018-11-27T12:24:13.7094210Z stack-trace:
2018-11-27T12:24:13.7097880Z at <ScriptBlock>, /Users/vsts/agent/2.142.1/work/1/s/test/tools/Modules/PSSysLog/PSSysLog.psm1: line 904
2018-11-27T12:24:13.7098430Z 904:             throw "did not recieve at least $MinimumCount records but $($log.Count) instead."

(note that receive is spelled wrong). According to the text, the test expects >= 18, but 47 is >= 18, so the validation must be wrong.

For the logging test that was failing in previous macOS CI, it's caused by removing Utility.psm1. The test script $testScriptPath has Write-Verbose in it, so when running & $powershell -NoProfile -SettingsFile $configFile -Command $testScriptPath, it triggers the auto-loading of Utility module. Before this PR, that results in 19 event logs. With this PR, two events that record the starting and completion of Utility.psm1 execution are gone, so the total number of event logs becomes 17. I have update the test code, also update PSSysLog.psm1 to correct the wrong message as @SteveL-MSFT pointed out above.

Yes we do. Type conversion depends on the type catalog. Not all .NET Core assemblies shipped with PS Core are loaded at startup.

But the dll is loaded at startup. Only "Type conversion depends on the type catalog" prevents us from removing the dll from TypeCatalog?

ApplicationInsight telemetry is done in console.dll at startup, as long as the user hasn't opted out.

I understand. My request is - Does the telemetry delay startup or no? If yes we could postpone the telemetry call and loading the dll.

There is no interference from other pwsh processes when running the tests. BenchmarkDotNet starts each process many times, and sometimes there are outliers.

I think these outlies is important to investigate in the startup context. Outlier is unexpected delay. Why we see unexpected delay in the tests?
It is not for the PR but we shouldn't ignore this.

But the dll is loaded at startup. Only "Type conversion depends on the type catalog" prevents us from removing the dll from TypeCatalog?

Not sure what you mean by "But the dll is loaded at startup". Most of the assemblies included in the type catalog file are not loaded at startup.

Even for the dlls that are loaded at startup, type resolution would be much faster for a type from those dlls because we can avoid the effort to search types from all loaded assemblies to find the one.

I understand. My request is - Does the telemetry delay startup or no? If yes we could postpone the telemetry call and loading the dll.

The telemetry definitely play a role in the startup time, but I don't know how much after crossgen'ing the dll because I'm not able to analyze the stack info in PerfView/WPA with crossgen'ed assemblies yet.

I think these outlies is important to investigate in the startup context.

Every run of the test starts those processes many times for measurement, including the warm-up. Not every test has outliers in it. I think it's more related to the factors that is out of our control, such as the OS.

BTW, I replaced the detailed test record with a most recent run (after 2nd attempt to address comment commit). You can see that there is only 1 outlier and it's for measuring pwsh 6.1.

@lzybkr @PaulHigin @TravisEz13 Can you please look at the new changes to PerformSecurityChecks() in CompiledScriptBlock.cs ?

Major change is on how we decide to skip AMSI/malicious code scan. The previous way to check for skipping .psd1 files causes security vulnerabilities.

An attacker can put arbitrary content in a .psd1 file, and call [Parser]::ParseFile() to get an AST from the .psd1 file. Then the attacker can create a script block from the Ast by calling $myAst.GetScriptBlock(). That script block then will be considered from a .psd1 file and skip the code scan.

The skip-scan condition is changed to:

• the script block should come from a .psd1 file
• the script block is in fact a HashtableAst
• the HashtableAst is safe, by calling IsSafeValueVisitor.IsAstSafe(hashtableAst, GetSafeValueVisitor.SafeValueContext.Default).

The API IsSafeValueVisitor.IsAstSafe has been used in GetSafeValueVisitor.GetSafeValue, which is used in module script analysis code, Import-PowerShellDataFile, and the public API Ast.SafeGetValue().

We have many comments in the PR that is a problem for GitHub web interface. If security concerns have been fixed we could merge and continue with hash function in follow PR.

@daxian-dbw Seems you could merge.

@lzybkr Would you like to take another look before I merge?

Attempt to address comments + rebased to resolve the conflict in Microsoft.PowerShell.Commands.Utility.csproj.
@lzybkr Can you please take another look? Thanks!

The failed test is most likely caused by #8346. It's a feature-level test, but the commits in that PR don't have the [feature] tag, so the feature tests didn't run for that PR CI.

Here is the failure:

2018-11-30T19:46:25.1215536Z     [-] ConvertFrom-Json deserializes an array of PSObjects (in multiple lines) as a single string. 32ms
2018-11-30T19:46:25.1604605Z       JsonSerializationException: Unexpected end when reading JSON. Path '', line 1, position 3.
2018-11-30T19:46:25.1617847Z       ArgumentException: Conversion from JSON failed with error: Unexpected end when reading JSON. Path '', line 1, position 3.
2018-11-30T19:46:25.1618346Z       at <ScriptBlock>, /home/vsts/work/1/s/test/powershell/Modules/Microsoft.PowerShell.Utility/Pester.Commands.Cmdlets.Json.Tests.ps1: line 1457

Did we lose a bit of performance with latest commits?

Yeah, it looks so to me. The average startup time improvement compared with PS Core 6.1 drops to 24.5% in my tests with changes in the master branch. It's a 1.5% drop on average comparing with before my changing to IsSafeAst. I have updated the number and the example test record in the PR description.

It's the price for security, I guess :/ But I think we can possibly further improve it by having an internal overload of IsSafeAst. The current IsSafeAst always creates a new instance of IsSafeValueVisitor, but in this case here, we should reuse the same instance instead of creating a new object every time, which might add GC pressure given how frequently this method will be called. But doing this shouldn't help much on the startup time as we only call this method once during the startup pwsh -noprofile -c echo 1. I will pursue this lead in my follow-up PRs, which will be out soon.

1. My comment above:

In fact, we come from one point - ScriptBlock.CreateDelayParsedScriptBlock("string", isProductCode: true).
I don't review in depth but I don't found the pattern "isProductCode: false". Perhaps we could implement separate code path.

2. We have some frozen PRs from @powercode which can increase overall performance and speed up startup too.

3. I will push a PR for using Unicode simple case folding that seed up too.

Can you elaborate a bit on the unicode simple case folding? I'm not sure what that is.

@daxian-dbw See here for the discussion on that: #8120