Remove extra check that the system dll exists

[iSazonov]

PR Summary

Related #9884

Remove extra check that the system dll exists. We need only one check, which is performed during first attempt to load this assembly.

PR Context

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
  • OR
  • I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
    • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode (which runs in a different PS Host).
    • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
    • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
    • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).

[TravisEz13]

This is still needed for Operating System before Windows 10

[TravisEz13]

cc @PaulHigin

[PaulHigin]

Actually I feel this is the right fix. We catch the DllNotFoundException and set the s_hadMissingWldpAssembly field there. I don't see a need to check it every time at the top of the method.

[iSazonov]

Yes, it is already in the PR description.

I just do not understand why the security fix (mentioned in related issue) led to this problem if it is a long-standing code? Or is there still another problem?

Update: It seems wldp.dll is more slow after the security fix
After remove the extra file check we have for debug compilation:

for release compilation:

Output from test script is:

Without bp:     150
With bp:        1041

So we have still a noticeable delay in wldp.dll.
Is it acceptable to cache policy for each path or MSFT team will investigate wldp internally? For Pester scenario we can get a very large cache.

[PaulHigin]

LGTM

[PaulHigin]

Actually I feel this is the right fix. We catch the DllNotFoundException and set the s_hadMissingWldpAssembly field there. I don't see a need to check it every time at the top of the method.

[daxian-dbw]

@iSazonov Three questions:

1. Is P/Invoke in general becomes slower or only the P/Invoke calls to wldp.dll becomes slower?
2. the trace you collected, are they from Windows PowerShell 5.1 or the latest PowerShell Core?
3. how much improvement do you get with this change?

[PaulHigin]

@iSazonov I can contact the wldp team but I need more information. Can you clearly summarize how you ran your perf tests and obtained the traces? I don't know what 'with bp' and 'without bp' means... We can't cache filepath information and need to rely on wldp APIs to be performant. Thanks!

[daxian-dbw]

I don't know what 'with bp' and 'without bp' means...

I believe it's with breakpoint and without breakpoint.

But @iSazonov did you share your test script somewhere?

[PaulHigin]

I took a look at the original issue (#9884), and found the problem. The issue is not related to the wldp.dll, however I still feel this fix is correct and should be taken.

The problem is the debugger disabling work that went into the current preview builds. The script debugger is now disabled if the system is locked down, and any attempt to change debugger state is guarded by a system lock down check. System lock down state is normally cached, but for the (normal) case where no policy is applied then a check is always performed for debug hooks (__PSLockdownPolicy) for testing purposes, and these checks include environment and registry settings. Note that this perf problem only occurs when a debug breakpoint is set.

I'll work on a fix for this and create a new PR.

[iSazonov]

I used a test script from original issue:
File c:\temp\b.ps1

Get-PSBreakpoint | Remove-PSBreakpoint
$sw = [System.Diagnostics.StopWatch]::startnew()
#"Without bp:`t$($sw.Elapsed.Ticks)"
$null = Set-PSBreakpoint -Line 2 -Script $PSCommandPath
$sw = [System.Diagnostics.StopWatch]::startnew()
#"With bp:`t$($sw.Elapsed.Ticks)"

You can uncomment lines to see elapsed times.

Test with breakpoint:

1. Run pwsh
2. Run PerfView
3. Start collect in PerfVew
4. Run in PowerShell Core session:

for ($i = 0 ; $i -lt 1000; $i++) {
     c:\temp\b.ps1
}

5. Wait for the end of the script block and then stop collect

Now comment "$null = Set-PSBreakpoint -Line 2 -Script $PSCommandPath" line in the test file and repeat the test steps for testing without breakpoint.

[iSazonov]

@daxian-dbw

Is P/Invoke in general becomes slower or only the P/Invoke calls to wldp.dll becomes slower?

I don't know. I guess if P/Invoke were the case then we would see a slowdown in other places too.

the trace you collected, are they from Windows PowerShell 5.1 or the latest PowerShell Core?

It is for latest PowerShell Core (my local build - I collect for debug (to see code paths) and release (to see real delays)).

how much improvement do you get with this change?

Visually the test script (b.ps1 file) runs ~3x faster in with-breakpoint case but still 4-5x slower than without-breakpoint case.

[iSazonov]

@PaulHigin

I took a look at the original issue (#9884), and found the problem. The issue is not related to the wldp.dll, however I still feel this fix is correct and should be taken.
The problem is the debugger disabling work that went into the current preview builds.

It is not clear why mentioned Windows security fix cause the original issue. It seems you found another issue.
Trace for the PR build shows

that we spend a lot of time in SecuritySupport.GetSaferPolicy() that is advapi32.SaferIdentifyLevel P/Invoke. Perhaps the security patch did slow down the API, not wldp.dll.

[iSazonov]

I look Get-Module -ListAvailable -All - it seems it is slower now too (SaferIdentifyLevel consumes up too 48% time in debug build).
Also we have #10264.

[PaulHigin]

@iSazonov No, the issue I found is the same as #9884. I simply repro'd that issue (setting debug break point causes serious perf problem) and found it was a regression from the debugger disabling change. I think there is also a PInvoke perf problem as well, apparently due to a Windows security fix. But this particular issue is due to a regression caused by a previous code change.

I'll create a PR with the fix.

[iSazonov]

@TravisEz13 The PR is ready to merge.

[ghost]

🎉v7.0.0-preview.3 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes