Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

Merged
daxian-dbw merged 2 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
Fs00:masterCopy head branch name to clipboard
Dec 12, 2020
Merged

Retrieve PowerShell version in PSVersionInfo using assembly informational version instead of FileVersionInfo#14332
daxian-dbw merged 2 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
Fs00:masterCopy head branch name to clipboard

Conversation

@Fs00

@Fs00 Fs00 commented Dec 6, 2020

Copy link
Copy Markdown
Contributor

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

iSazonov commented Dec 6, 2020

Copy link
Copy Markdown
Collaborator

@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

ghost commented Dec 6, 2020 •
edited by ghost
Loading

Copy link
Copy Markdown

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

Fs00 commented Dec 6, 2020

Copy link
Copy Markdown
Contributor Author

@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

iSazonov commented Dec 6, 2020

Copy link
Copy Markdown
Collaborator

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

iSazonov commented Dec 6, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

iSazonov commented Dec 12, 2020

Copy link
Copy Markdown
Collaborator

@Fs00 Thanks for your contribution!

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

ghost commented Feb 12, 2021

Copy link
Copy Markdown

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

Handy links:

@ALIENQuake

ALIENQuake commented Feb 13, 2021

Copy link
Copy Markdown

@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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.