Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add build module #698

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Add build module #698

wants to merge 16 commits into from

Conversation

bergmeister
Copy link
Contributor

@bergmeister bergmeister commented Sep 15, 2019

Related: #692
This targets the master branch.

It creates the module in an out folder by copying the content of the src folder into it first and then merging the .ps1 files into the main .psm1 files that are dot sourced by looking for the pattern $PSScriptRoot\ScriptName.ps1 and replacing it with the content and wrapping it into a #region block. The result is stored in a folder with the name being the version, so that one can just add to to the PSModulePath env variable if one wanted to.

This cuts the import time roughly in half for a pure import from a started up PS (from 800 to 400 ms on my machine) or reduced the import time by 25% when the import happens in the $PROFILE (don't know why the benefit is smaller in this case but probably because the import is generally slow in the $Profile due to PS being busy at the start)

I've adapted the test logic to use the built module if the CI env variable is set and the path exists and in AppVeyor I keep running tests the old way, then build and then make them run against the built module
I've adapted the test logic to use the

@rkeithhill
Copy link
Collaborator

Rather than put this in a top-level out dir, can you read the module version number from the psd1 file and create/build into that dir e.g. .\1.0.0.0. That would satisfy @dahlbyk desire to make it possible to put the repo path in $env:PSModulePath and have import-module work. You'll need to add this to the .gitgnore file:

# ignore release dirs
/*.*.*

@bergmeister
Copy link
Contributor Author

Awesome, I've applied both your suggestions. Shall I adapt the build as part of this PR as well to test against the built version?

@rkeithhill
Copy link
Collaborator

That would be great. Maybe add build script parameters along the lines of -BuildOnly and -TestOnly?

@bergmeister
Copy link
Contributor Author

For the moment I make it conditionally use the built folder based on the existence of the CI environment variable for the moment just to see if it works.
Do we want to run tests twice now (before and after build)?

@@ -1,4 +1,9 @@
$modulePath = Convert-Path $PSScriptRoot\..\src
# Test against built folder in CI (or just set $env:CI locally for that)
if ($env:CI) {
Copy link
Collaborator

@rkeithhill rkeithhill Sep 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works on a build machine. Can we also add a PowerShell variable check here like:

if ($env:CI -or $PoshGitBuild)

That way if we build & test from the build.ps1 script on a dev machine, it will run the Pester tests against the release dir. And because that $PoshGitBuild variable goes away after the build script exits, I can still run tests from the src folder module when I doing dev work.

@bergmeister
Copy link
Contributor Author

bergmeister commented Sep 15, 2019

Where is the definition of the Travis build so that I can add the call to the build script? Otherwise I'll have to change the code to only use the build if the build version directory exists.
For the moment, it seems 1 test breaks on AppVeyor (probably some hard-coded assumption), will need to take some time to look into that and fix it.

@bergmeister bergmeister changed the title Add build module WIP Add build module Sep 15, 2019
@bergmeister bergmeister changed the title WIP Add build module Add build module Sep 15, 2019
@bergmeister
Copy link
Contributor Author

PR is ready to review now

@rkeithhill
Copy link
Collaborator

I'm seeing about a 176 out of 1242 mS improvement on startup or about a 14% improvement. That's good but not as good as I'd hoped. Here's how I tested:

C:\Users\Keith
10-13 13:58:21 4> $res = @(); for ($i=0; $i -lt 100; $i++) { $res += Measure-Command { pwsh -nop -wd $home\GitHub\rkeithhill\vscode-powershell -command "& { import-module $home\GitHub\dahlbyk\posh-git\src\posh-git.psd1}" }; Start-Sleep -Seconds 3 }; $res | Measure-Object -Property TotalMilliseconds -AllStats                                                
Count             : 100
Average           : 1242.651353
Sum               : 124265.1353
Maximum           : 2306.8213
Minimum           : 1155.3381
StandardDeviation : 174.263883117032
Property          : TotalMilliseconds


C:\Users\Keith
10-13 14:11:37 5> $res = @(); for ($i=0; $i -lt 100; $i++) { $res += Measure-Command { pwsh -nop -wd $home\GitHub\rkeithhill\vscode-powershell -command "& { import-module $home\GitHub\bergmeister\posh-git\1.0.0.0\posh-git.psd1}" }; Start-Sleep -Seconds 3 }; $res | Measure-Object -Property TotalMilliseconds -AllStats                                        
Count             : 100
Average           : 1066.09344
Sum               : 106609.344
Maximum           : 1642.2115
Minimum           : 1007.3209
StandardDeviation : 87.9550066181624
Property          : TotalMilliseconds

That said, I'm still in favor of merging this.

@dahlbyk What do you think? This impacts your build & release process.

Copy link
Collaborator

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bergmeister
Copy link
Contributor Author

bergmeister commented Oct 14, 2019

Yes, I also found the speed-up when the import happens in $PROFILE or directly after the startup is only 10-20%. When the import happened in an idle PowerShell session manually, then the the import was twice as fast. I can only guess that there are other threads that are still active immediately after PS startup that reduce the resources available for the speedup and therefore lead to a smaller speedup.
But since it is still quite a big improvement equivalent to the startup time of pwsh itself, it's worthwhile IMHO and with computers getting even more cores in the future, the relative speedup of this optimisation will probably increase more with the number of cores.

@musm
Copy link

musm commented Oct 17, 2019

@bergmeister thanks for improving startup times! It has been annoying with powershell 6, which is twice as to load than powershell 5.

@rkeithhill
Copy link
Collaborator

Also helps with #635

@rkeithhill rkeithhill requested a review from dahlbyk December 29, 2019 21:07
@musm
Copy link

musm commented Mar 19, 2020

bump? Would be nice to improve startup times if this helps with that.

@eluchsinger
Copy link

eluchsinger commented May 11, 2020

Can we merge this in? I don't know why, but my PS Core takes long to start when I have posh-git installed.

@dahlbyk

@bergmeister
Copy link
Contributor Author

bergmeister commented Jul 9, 2020

@rkeithhill @dahlbyk Can you please speedup the review process or if one of you two is busy I expect the other person to take over? It's been nearly a year now! I would not want to do it but otherwise I will consider publishing a forked version....

@rkeithhill
Copy link
Collaborator

You know, some people actually do go on vacation. :-) But I'm back now. That said, I don't do the build/final publish of the module. That's something that only @dahlbyk can do AFAIK. So I've left this PR up to him to approve/merge.

@bergmeister
Copy link
Contributor Author

You know, some people actually do go on vacation. :-) But I'm back now. That said, I don't do the build/final publish of the module. That's something that only @dahlbyk can do AFAIK. So I've left this PR up to him to approve/merge.

Yes I appreciate and respect that people go on holiday or are being busy for a while due to personal/work reasons, especially since this is an OSS project without company backing. But a year is what is called a sabbatical not vacation ;-)

@rkeithhill
Copy link
Collaborator

Understood. I've already approved this but @dahlbyk needs to as well and he's a very busy fellow these days. 🤷‍♂️

@dahlbyk
Copy link
Owner

dahlbyk commented Jul 15, 2020

Burnout. Worse this year than in the past.

I have the few weeks off work, so hoping to dig back into posh-git during that time. Appreciate your contribution and patience.

@bergmeister
Copy link
Contributor Author

bergmeister commented Jul 15, 2020

Sorry to hear that and recovery has priority in this case @dahlbyk. How would you feel about handing the maintenance/ownership over to @rkeithhill (assuming he'd be happy with that) to help you relieve from the burden/pressure of maintenance.

@musm
Copy link

musm commented Jul 16, 2020

I do want to mention that the extremely slow pwsh load times already is a big grievance for powershell users (see this 2 year old issue PowerShell/PowerShell#6443 (comment)). I use posh-git everyday, but it adds a painful 0.5s to 1s to my load time. For example see also this user with similar reports: PowerShell/PowerShell#6443 (comment)

I do want to thank the work of the development team in trying to improve the situation.

@rkeithhill
Copy link
Collaborator

@musm To set expectations if this PR is merged, the perf improvement is only in the range of 10-20%.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants