New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve powershell startup time #8341
Conversation
Do you continue in the PR?
I don't see Microsoft.ApplicationInsights.dll in the table - no jitting? I searched "Insights" in our code base and have questions:
If
having
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. |
src/System.Management.Automation/engine/runtime/CompiledScriptBlock.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/runtime/CompiledScriptBlock.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/runtime/CompiledScriptBlock.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs
Outdated
Show resolved
Hide resolved
src/Modules/Unix/Microsoft.PowerShell.Utility/Microsoft.PowerShell.Utility.psd1
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertFrom-SddlString.ps1
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/runtime/CompiledScriptBlock.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.Commands.Utility/commands/utility/Import-LocalizedData.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.Commands.Utility/Microsoft.PowerShell.Commands.Utility.csproj
Show resolved
Hide resolved
@iSazonov Reply to your comment here:
No, that will be in a new PR. The planned work are as follows:
After the change, no IL code needs to be jitted for this dll at startup time.
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.
ApplicationInsight telemetry is done in console.dll at startup, as long as the user hasn't opted out.
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.
The reason to change
There is no interference from other pwsh processes when running the tests.
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 |
The macOS failure appears to be a test code issue:
(note that |
For the logging test that was failing in previous macOS CI, it's caused by removing |
But the dll is loaded at startup. Only "Type conversion depends on the type catalog" prevents us from removing the dll from TypeCatalog?
I understand. My request is - Does the telemetry delay startup or no? If yes we could postpone the telemetry call and loading the dll.
I think these outlies is important to investigate in the startup context. Outlier is unexpected delay. Why we see unexpected delay in the tests? |
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.
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.
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 |
src/System.Management.Automation/engine/runtime/CompiledScriptBlock.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/runtime/CompiledScriptBlock.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.Commands.Utility/commands/utility/ConvertFrom-SddlString.cs
Outdated
Show resolved
Hide resolved
@lzybkr @PaulHigin @TravisEz13 Can you please look at the new changes to Major change is on how we decide to skip AMSI/malicious code scan. The previous way to check for skipping An attacker can put arbitrary content in a The skip-scan condition is changed to:
The API |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LTGM. Just minor comments.
src/System.Management.Automation/engine/runtime/CompiledScriptBlock.cs
Outdated
Show resolved
Hide resolved
…us code scan at startup time for a regular session
Major change is about how we decide to skip AMSI/malicious code scan. A script block may comes from a .psd1 file but still has arbitrary code in it. An attacker can parse an .psd1 file to get an Ast and call Ast.GetScriptBlock() to create a script block. That script block comes from a .psd1 file that may contain arbitrary content. The skip-scan condition is changed to: - the script block should come from a .psd1 file - the script block is essentially a HashtableAst - the HashtableAst is safe.
Attempt to address comments + rebased to resolve the conflict in |
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 Here is the failure:
|
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 It's the price for security, I guess :/ But I think we can possibly further improve it by having an internal overload of |
|
Can you elaborate a bit on the |
@daxian-dbw See here for the discussion on that: #8120 |
I want to use Convertfrom-sddlstring in c# ? what is the equivalent method in c# and what library? |
PR Summary
With this PR, the startup time of
pwsh
gets about24.5%
improvement on average comparing with the PS Core 6.1.0 official release. The following is an example measurement result by usingBenchmarkDotNet v0.11.2
(the detailed information about this measurement result can be found at the end):Detailed Changes
SecuritySupport.IsProductBinary
and unnecessary AMSI/suspicious code scan at startup time for a regular sessionCompiledScriptBlockData.IsProductCode
to avoid unnecessary calls toIsProductBinary
, which attempts to retrieve catalog signature of the target file.PerformSecurityChecks
to skip AMSI and suspicious code scan for the.psd1
file that contains a safeHashtableAst
only.ReadOnlyBag
instead ofImmutableHashSet
so that we can avoid loading theSystem.Collections.Immutable.dll
completely.SHA1
withCRC32
when generating module analysis cache file nameSystem.Security.Cryptography.Algorithms.dll
at startupConvertFrom-SddlString
to C# to remove theUtility.psm1
file.Microsoft.ApplicationInsights.dll
and enable tiered compilation191.6ms
comparing with24.7ms
for Windows PowerShell.Microsoft.ApplicationInsights.dll
took about51.6ms
.Microsoft.ApplicationInsights.dll
and enable tiered compilation, the jitting time drops to about98.9ms
. Detailed information can be found at the end.NOTE: Each commit is self-contained and the purpose of the commit is described by the commit message. So it would be easier to review per commit.
Compared with Windows PowerShell
The optimized pwsh is still about
75ms
slower than Windows PowerShell. The main reason is jitting time difference, even though we crossgen all the assemblies in the table below except forpwsh.dll
.More work will be done to further bring down the jitting time at startup.
Reference Set Improvement
Impacting memory size loaded from disk (total): 5.9% less pages (606 / 10289 pages), 6% less memory (2.4 / 40.2 mb). The comparison of the image pages being loaded during startup before and after this PR is as follows:
Jitting Time Improvement
Windows PowerShell
Before crossgen'ing
Microsoft.ApplicationInsights.dll
and applying tiered compilationAfter crossgen
Microsoft.ApplicationInsights.dll
and applying tiered compilationStartup Detailed Measurement Record
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
to the beginning of the title and remove the prefix when the PR is ready.[feature]
if the change is significant or affects feature tests