Retrieve productVersion using informational version attribute in AmsiUtils.Init() #15527
Conversation
|
Since it is about startup performance please share perf results for just startup. Either |
|
Here are the results in milliseconds of running master branch: This PR: |
Please do all perf test on Release ( Start-PSBuild -Configuration Release -CrossGen) |
|
Same test done on Release build: master branch: This PR: |
|
@Fs00 Thanks for perf test sharing! Please add this in the PR description. |
| // (This has occurred during Exchange set up). | ||
| hostname = GetProcessHostName(currentProcess.ProcessName); | ||
| } | ||
| string appName = string.Concat("PowerShell_", Environment.ProcessPath, "_", PSVersionInfo.ProductVersion); |
There was a problem hiding this comment.
Can 'Environment.ProcessPath' throw? We should have a try/catch here so that AMSI is always initialized.
There was a problem hiding this comment.
Actually yes, it might throw (https://github.com/dotnet/runtime/blob/a5e626a015a37cf332e993804cd8bb42943c14c2/src/libraries/System.Private.CoreLib/src/System/Environment.Windows.cs#L104).
Should I then catch a generic Exception here?
There was a problem hiding this comment.
Yes, I think a generic Exception, and then create the default hostname as before.
There was a problem hiding this comment.
I've added a catch block as you requested. I kept using PSVersionInfo.ProductVersion in the catch block (before the PR a placeholder version of 0.0.0.0 was used) since we can safely assume that it will never throw anything.
There was a problem hiding this comment.
Btw, I'm a bit uncertain on using Process.ProcessName as a "safe" fallback - it doesn't look much safer than Environment.ProcessPath given that it could potentially throw an InvalidOperationException (https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.process.processname?view=net-5.0#exceptions).
There was a problem hiding this comment.
Good point, but since it is basically the same as before at least we don't introduce a possible regression.
There was a problem hiding this comment.
If the assumption is that Environment.ProcessPath may throw while PSVersionInfo.ProductVersion will never throw, then a comment should be added in the catch block to call this out.
[edited] comment added.
|
Not for the PR but while we are here - what do you think about using new c# feature - source generator - for getting ProductVersion? |
|
@iSazonov That sounds like a nice thing to try. Not just the product version, any reflection tasks that can be deterministically done at build time can be considered to be lifted to the source generator. |
|
Thanks for merging! |
|
@Fs00 Thanks for your contribution! |
|
🎉 Handy links: |
PR Summary
This PR slightly improves PowerShell start-up performance on Windows by using
AssemblyInformationalVersionAttributeinAmsiUtils.Initto retrieve PowerShell product version instead ofProcessModule.FileVersionInfo, which is notably slower.Also, the code that handled possible errors coming from
ProcessModule.FileVersionInfohas been removed.Here is the startup performance difference before and after the PR (launched
Measure-Command { .\pwsh -noprofile -c exit }15 times on a Release build):master branch:
356,89, 357,19, 356,40, 362,44, 354,41, 355,71, 359,56, 358,05, 350,51, 365,41, 356,74, 357,24, 357,06, 353,06, 353,05
Best: 350,51 ms
Worst: 365,41 ms
Average: 356,91 ms
This PR:
358,31, 347,59, 350,51, 352,66, 348,42, 351,90, 350,34, 354,94, 351,55, 339,00, 344,30, 345,26, 347,64, 351,45, 343,43
Best: 339,00 ms
Worst: 358,31 ms
Average: 349,15 ms
Contributes to #14268.
PR Context
This chance for optimization has been discovered by @iSazonov in #14332 (comment).
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header