Skip to content

Conversation

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Dec 4, 2020

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
Copy link
Collaborator Author

iSazonov commented Dec 4, 2020

@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
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
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
Copy link
Collaborator Author

iSazonov commented Dec 5, 2020

@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
Copy link
Collaborator Author

iSazonov commented Dec 5, 2020

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