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

Remove dead code from SemanticVersion class#14320

Closed
iSazonov wants to merge 1 commit into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:perf-cleanup-semantecversioniSazonov/PowerShell:perf-cleanup-semantecversionCopy head branch name to clipboard
Closed

Remove dead code from SemanticVersion class#14320
iSazonov wants to merge 1 commit into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:perf-cleanup-semantecversioniSazonov/PowerShell:perf-cleanup-semantecversionCopy head branch name to clipboard

Conversation

@iSazonov

@iSazonov iSazonov commented Dec 4, 2020

Copy link
Copy Markdown
Collaborator

PR Summary

Remove dead code from SemanticVersion class. It was added pending the development of semantic version story but we got no feedback and no progress. Currently we are waiting .Net for the story.

The PR speeds up startup scenario on my laptop up to 50 ms.

PR Context

Related #14268

PR Checklist

@iSazonov iSazonov added the CL-Performance Indicates that a PR should be marked as a performance improvement in the Change Log label Dec 4, 2020
@iSazonov iSazonov requested a review from rjmholt December 4, 2020 13:02
@iSazonov iSazonov closed this Dec 4, 2020
@iSazonov iSazonov reopened this Dec 4, 2020
@iSazonov

iSazonov commented Dec 4, 2020

Copy link
Copy Markdown
Collaborator Author

@daxian-dbw @rjmholt Could you please explain how the public static implicit operator Version(SemanticVersion semver) works? Original code defines PSObject psobj and not return it - looks like dead code. But if I remove it tests fail which means that the original code still implicitly returns PSObject!

Update: looks like a tricky bug in .Net Runtime. :-(

@daxian-dbw

Copy link
Copy Markdown
Member

@iSazonov psobj.Properties.Add adds the member to s_instanceMembersResurrectionTable, which allows PowerShell to retrieve the ETS members for the specific object, the System.Version result here, when access that object in PowerShell later. Put it another way, psobj.Properties.Add makes PowerShell remember that an instance member is added for the particular object result.

This code path shouldn't be removed. It's the reason why you see the following from a System.Verseion object.

PS C:\> $PSVersionTable.PSCompatibleVersions[-1]

Major  Minor  Build  Revision PSSemVerPreReleaseLabel    PSSemVerBuildLabel
-----  -----  -----  -------- -----------------------    ------------------
7      2      0      -1       preview.1

@daxian-dbw daxian-dbw closed this Dec 4, 2020
@daxian-dbw

Copy link
Copy Markdown
Member

Besides, this particular impact won't affect stable versions, because PSSemVerPreReleaseLabel and PSSemVerBuildLabel are null for stable versions. When profiling, I recommand you to build PowerShell with -ReleaseTag v7.2.0, so as to mimic a stable version.

@iSazonov

iSazonov commented Dec 5, 2020

Copy link
Copy Markdown
Collaborator Author

@daxian-dbw Thanks for clarify!
I was sure that we left this code only as "want get a feedback" and till .Net solution comes.
Based on our experience in PowerShell I shared in .Net proposal to enhance System.Version type instead of creating new .Net SemanticVersion class. If this will be implemented in any way we will fall in a breaking change so we can close the PR and add more in our documentation about PowerShell SemanticVersion class.

@iSazonov

iSazonov commented Dec 5, 2020

Copy link
Copy Markdown
Collaborator Author

When profiling, I recommand you to build PowerShell with -ReleaseTag v7.2.0, so as to mimic a stable version.

I see your point but:

  1. Modern live cycle development model assumes preview versions is stable enough and works as well as release. Users actively use Preview versions. I believe they shouldn't disappoint users and behave as well as releases.
    It also allows us to get reliable performance feedback early on, rather than after the final release.
  2. In SemanticVersion context PSSemVerPreReleaseLabel is null for stable version but PSSemVerBuildLabel could be not null - ex.: if we re-packaged we could increase the build label as SemVer standard says.

@iSazonov iSazonov deleted the perf-cleanup-semantecversion branch December 7, 2020 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.