Skip to content

Comments

Retrieve PowerShell version in PSVersionInfo using assembly informational version instead of FileVersionInfo#14332

Merged
daxian-dbw merged 2 commits intoPowerShell:masterfrom
Fs00:master
Dec 12, 2020
Merged

Retrieve PowerShell version in PSVersionInfo using assembly informational version instead of FileVersionInfo#14332
daxian-dbw merged 2 commits intoPowerShell:masterfrom
Fs00:master

Conversation

@Fs00
Copy link
Contributor

@Fs00 Fs00 commented Dec 6, 2020

PR Summary

This PR changes PSVersionInfo static constructor to retrieve PowerShell version data from AssemblyInformationalVersionAttribute, instead of using FileVersionInfo class.

PR Context

This change is a first step towards making System.Management.Automation single-file-application-friendly ‒ without this PR, PSVersionInfo static constructor throws an exception when used in a single-file application due to Assembly.Location returning "" ‒ and provides a small performance boost at startup (which contributes a tiny bit to #14268) since it does not parse System.Management.Automation DLL to retrieve the version anymore.
Here is the performance of the two changed lines:

  • before PR: ~0,23ms
  • after PR: ~0,03ms (~7 times faster)

PR Checklist

@ghost ghost assigned daxian-dbw Dec 6, 2020
@iSazonov
Copy link
Collaborator

iSazonov commented Dec 6, 2020

@Fs00 Thanks for your contribution!
I hope that we will soon be able to use .Net 6.0 Source Generators. So I'm going to refactor PSVersionInfo class and move all version constants to new GitInfo class. Later we will be able to generate it with a simple source generator and thereby remove all reflection overhead.
I will use your idea in follow PR.

@ghost
Copy link

ghost commented Dec 6, 2020 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@iSazonov iSazonov added the CL-Performance Indicates that a PR should be marked as a performance improvement in the Change Log label Dec 6, 2020
@Fs00
Copy link
Contributor Author

Fs00 commented Dec 6, 2020

@iSazonov Good to know.
Since it isn't sure when the refactoring with source generators will be done, could this PR be merged in the meantime? It would still be an improvement on the current situation and would allow me to test System.Management.Automation on single-file applications to see if there are any other issues.

@iSazonov
Copy link
Collaborator

iSazonov commented Dec 6, 2020

Since it isn't sure when the refactoring with source generators will be done, could this PR be merged in the meantime?

I see with the PR PowerShell loads less assembles at startup - it is great perf improvement! I think we will merge the PR.

@iSazonov
Copy link
Collaborator

iSazonov commented Dec 6, 2020 •
edited
Loading

I see System.Diagnostics.FileVersionInfo.dll is still loaded on Windows in AmsiUtils.Init()

PowerShell/src/System.Management.Automation/security/SecuritySupport.cs

Lines 1355 to 1357 in ea3036d

var processModule = PsUtils.GetMainModule(currentProcess);
hostname = string.Concat("PowerShell_", processModule.FileName, "_",
processModule.FileVersionInfo.ProductVersion);

@PaulHigin @TravisEz13 Do we really need the FileVersionInfo.ProductVersion? Can we use the same approach as @Fs00 in the PR?

Also GetMainModule() is very expensive. Could we use Assembly.GetEntryAssembly().FullName? On my laptop it is 5 ms vs 27 ms.
Or P/Invoke GetModuleFileName? In .Net 6.0 the GetModuleFileName is exposed as System.Environment.ProcessPath.

@iSazonov iSazonov mentioned this pull request Dec 9, 2020
Closed
15 tasks
@daxian-dbw daxian-dbw merged commit ea2da20 into PowerShell:master Dec 12, 2020
@iSazonov
Copy link
Collaborator

iSazonov commented Dec 12, 2020

@Fs00 Thanks for your contribution!

@iSazonov iSazonov added this to the 7.2.0-preview.2 milestone Dec 12, 2020
@ghost
Copy link

ghost commented Feb 12, 2021

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

Handy links:

@ALIENQuake
Copy link

ALIENQuake commented Feb 13, 2021

@Fs00 Late to the party but please accept my gratitude for your efforts to bring PowerShell closer to single-file-application-friendly!

@Fs00 Fs00 mentioned this pull request Mar 13, 2021
Merged
22 tasks
@Fs00 Fs00 mentioned this pull request Jun 5, 2021
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@iSazonov iSazonov iSazonov approved these changes

Assignees

@daxian-dbw daxian-dbw

Labels

CL-Performance Indicates that a PR should be marked as a performance improvement in the Change Log

Projects

None yet

Milestone

7.2.0-preview.2

Development

Successfully merging this pull request may close these issues.

4 participants

@Fs00 @iSazonov @ALIENQuake @daxian-dbw