posh-git icon indicating copy to clipboard operation
posh-git copied to clipboard

Add build module

Open bergmeister opened this issue 4 years ago • 20 comments

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

bergmeister avatar Sep 15 '19 19:09 bergmeister

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
/*.*.*

rkeithhill avatar Sep 15 '19 19:09 rkeithhill

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?

bergmeister avatar Sep 15 '19 20:09 bergmeister

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

rkeithhill avatar Sep 15 '19 20:09 rkeithhill

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)?

bergmeister avatar Sep 15 '19 20:09 bergmeister

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 avatar Sep 15 '19 20:09 bergmeister

PR is ready to review now

bergmeister avatar Sep 15 '19 22:09 bergmeister

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.

rkeithhill avatar Oct 13 '19 20:10 rkeithhill

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.

bergmeister avatar Oct 14 '19 11:10 bergmeister

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

musm avatar Oct 17 '19 12:10 musm

Also helps with #635

rkeithhill avatar Dec 11 '19 03:12 rkeithhill

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

musm avatar Mar 19 '20 06:03 musm

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

eluchsinger avatar May 11 '20 08:05 eluchsinger

@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....

bergmeister avatar Jul 09 '20 18:07 bergmeister

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.

rkeithhill avatar Jul 09 '20 19:07 rkeithhill

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 ;-)

bergmeister avatar Jul 09 '20 21:07 bergmeister

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

rkeithhill avatar Jul 09 '20 21:07 rkeithhill

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.

dahlbyk avatar Jul 15 '20 03:07 dahlbyk

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.

bergmeister avatar Jul 15 '20 09:07 bergmeister

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 https://github.com/PowerShell/PowerShell/issues/6443#issuecomment-659534629). 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: https://github.com/PowerShell/PowerShell/issues/6443#issuecomment-656282020

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

musm avatar Jul 16 '20 16:07 musm

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

rkeithhill avatar Jul 19 '20 02:07 rkeithhill