Use GitStatusCache when it's installed. #208
Conversation
…ule is available.
… to avoid ordering dependency in $profile
| @@ -117,45 +117,73 @@ function Get-GitStatus($gitDir = (Get-GitDirectory)) { | |||
| $filesUnmerged = @() | |||
|
|
|||
| if($settings.EnableFileStatus -and !$(InDisabledRepository)) { | |||
| dbg 'Getting status' $sw | |||
| $status = git -c color.status=false status --short --branch 2>$null | |||
| } else { | |||
There was a problem hiding this comment.
Looks like this else block got lost in the shuffle?
There was a problem hiding this comment.
Yep, but removed intentionally. If I'm reading the original code correctly the empty status set by the else block serves to make the loop no-op (and has no additional references below). In this version the loop will only be hit when settings.EnableFileStatus is true (checked before checking whether to get it from the cache or from "git status"), so the else block is now dead code. Please correct me if I'm misunderstanding.
There was a problem hiding this comment.
This is what I was thinking we needed, but you're absolutely right that it's dead code.
if($settings.EnableFileStatus -and !$(InDisabledRepository)) {
if ($settings.EnableFileStatusFromCache -eq $null) {
$settings.EnableFileStatusFromCache = (Get-Module GitStatusCachePoshClient) -ne $null
}
if ($settings.EnableFileStatusFromCache) {
dbg 'Getting status from cache' $sw
...
} else {
dbg 'Getting status' $sw
$status = ...
}
} else {
$status = @()
# Never used
}
💯 This does seem to address all my concerns with the previous implementations.
To test this out:
So far the caching seems to be working exactly as I would expect! |
|
I did just introduce a merge conflict on you... 😿 |
|
If you run install.ps1 in git-status-cache-posh-client, it should do the download on your behalf as part of setup as well as add the import to your profile. I like the idea of the warning or auto-download in the case where you enable it through another path and will follow-up on cmarcusreid/git-status-cache-posh-client#4 (may be a few days before I get a chance). |
Conflicts: GitPrompt.ps1
Ironic that I would overlook an |
|
README clarified. Not sure how I forgot that. :) |
👍 I'm hoping to get a bit of visibility on this to kick the tires before merging, but I'm not too worried about it since the caching is optional. |
|
I just tested this and it rocks. Waaay faster. Thanks guys! |
Conflicts: GitUtils.ps1
|
So much nicer. posh-git was starting to go incredibly slow. |
|
Hi @cmarcusreid , this really works well! Can you consider adding the StashStatus from #215 into your Pipelines? I tried to file a PR at your repositories, but I'm no C++ guy and can't get the project building :-) |
|
Fails on windows 10, had to add -UseBasicParsing to all wgets in GitStatusCachePoshClient.ps1 to make install.ps1 to run without errors. Also required vcredist_x86 to be installed. |
|
Hey @MarkusAmshove! Stash support was added to the cache for in cmarcusreid/git-status-cache#12. Relevant integration for this PR is in 008836e. If you have both, posh-git should show it when the setting is enabled. If you started using the cache before this change, you may need to update your cache bits (grab the latest git-status-cache-posh-client and call Update-GitStatusCache to download.) You can check if the cache is returning stash information by calling Get-GitStatusFromCache: |
|
@Kantis I don't repro on Windows 10, so I wonder what's different about our environments. Could you clarify the following to help me get this fixed?
|
|
@cmarcusreid 2 When calling the install script Started IE once now and it fixed the error. Think vcredist might still be required though? |
|
@Kantis Thanks. -UseBasicParsing EDIT: Perfect. That explains it. Thanks! Regarding vcredist: I'll take a look to see if that dependency can be removed. Opened cmarcusreid/git-status-cache#13 for that feedback. Either way, I've logged issue cmarcusreid/git-status-cache-posh-client#9 on git-status-cache-posh-client and will get this fixed. |
|
Pipe to Instead, |
… useGitStatusCache Conflicts: GitUtils.ps1
|
The response of Get-GitStatusFromCache is built by Convert-FromJson. Unfortunately Convert-FromJson converts arrays to Object[] even though they're strings in this case. I don't think we have a way to eliminate the cast (without doing equivalent work ourselves), but I'm open to suggestions if someone sees a better solution. I've merged 3e13ee8 based on the AddRange note. |
We could use |
|
I would expect the LINQ version to have comparable cost to the earlier foreach as it also incurs an enumeration. Probably the next step if we want to drill into this is to profile each option and see if there's an appreciable difference. Do you feel this is a worthwhile investigation? |
|
I expect the LINQ version to always win. I improved the overhead of I suppose it's possible if you have a huge number of objects (approaching physical memory limits), then the pipeline might win, but for typical workloads sensitive to performance, you should prefer .Net methods over the pipeline where possible. |
|
Got it. Right now we're using the string[] cast. Any intuition on that versus the LINQ cast? |
|
They have differing semantics - LINQ cast will only do a C# cast whereas PowerShell [string[]] cast will convert each element of the array using PowerShell rules, e.g. calling ToString if the object is something other than a string. That said, a LINQ cast avoids a copy of the array, so in theory it should be better, but it can't be called without reflection (PowerShell does not provide a way to specify a generic argument only used in the return type). I measured the two options and LINQ is about 3x faster even with the reflection (tested with a 5000 strings in an object[]) using: # Create 5000 strings
$objArray = @("a") * 5000
$collection = [System.Collections.Generic.List[string]]::new()
$m = [System.Linq.Enumerable].GetMethod('Cast').MakeGenericMethod([string])
$collection.AddRange($m.Invoke($null, (,$objArray))) |
|
Excellent, thanks! I'll make this adjustment when I have a chance (probably sometime over the weekend). |
|
Working on this now, will push to this branch. |
|
Not thrilled with b07bce5, but it seems to work? PowerShell auto-enumerating stuff is tricky to work around. 😦 |
Per comments on b07bce5, I've inlined the reflected method call instead of wrapping it in a PowerShell function. I know a few folks have mentioned they have been running with this locally for a while. Update to this branch's latest and confirm your projects still work as expected? |
| dbg 'Getting stash count' $sw | ||
| $stashCount = $null | git stash list 2>$null | measure-object | Select-Object -expand Count | ||
| if ($settings.EnableFileStatusFromCache -eq $null) { | ||
| $settings.EnableFileStatusFromCache = (Get-Module GitStatusCachePoshClient) -ne $null |
There was a problem hiding this comment.
Are we sure we want to do this? It makes it harder to "turn off" this feature if something is misbehaving (or you want to test with it off). Right now, you'd have to unload (remove-module GitStatusCachePoshClient) and then be careful not to execute a command that would auto-load it again.
There was a problem hiding this comment.
It auto-selects based on the module's presence the first time through (when set to the default null value), but if you manually set it to false that'll stick on subsequent calls since it's no longer null.
|
It looks like rename operations currently break the prompt. Other operations look fine on my machine. Manual execution of the cast throws an InvalidOperationException. Unable to cast object of type System.Management.Automation.PSObject to type System.String. |
|
@cmarcusreid Which rename operations? Is this the rename of the prompt function that the Chocolatey install adds to your profile? If so, that will be removed before 0.7.0 is released. Importing the module now puts in place the default posh-git prompt. |
|
Rename operations on files in the index or working directories. :) I believe it's just the new cast bit in this PR; I'll take a look sometime this weekend if no one beats me to it. |
| @@ -159,13 +159,19 @@ function Get-GitStatus($gitDir = (Get-GitDirectory)) { | |||
|
|
|||
| $indexAdded.AddRange($castStringSeq.Invoke($null, (,@($cacheResponse.IndexAdded)))) | |||
| $indexModified.AddRange($castStringSeq.Invoke($null, (,@($cacheResponse.IndexModified)))) | |||
| $indexModified.AddRange($castStringSeq.Invoke($null, (,@($cacheResponse.IndexRenamed | Select-Object -ExpandProperty Old)))) | |||
There was a problem hiding this comment.
Cast was choking on the System.Management.Automation.PSObject returned by Select-Object.
| @@ -159,13 +159,19 @@ function Get-GitStatus($gitDir = (Get-GitDirectory)) { | |||
|
|
|||
| $indexAdded.AddRange($castStringSeq.Invoke($null, (,@($cacheResponse.IndexAdded)))) | |||
| $indexModified.AddRange($castStringSeq.Invoke($null, (,@($cacheResponse.IndexModified)))) | |||
| $indexModified.AddRange($castStringSeq.Invoke($null, (,@($cacheResponse.IndexRenamed | Select-Object -ExpandProperty Old)))) | |||
| $indexRenamedOld = $cacheResponse.IndexRenamed.Old | |||
There was a problem hiding this comment.
I just learned that you could do this today; what is this trick called (for search purposes)?
It returns either null (zero paths), a string (one path), or an object[] (many paths).
There was a problem hiding this comment.
Member enumeration but it is only available in PS v3 and higher. To make that work on PS v2, use:
$indexRenamedOld = foreach ($obj in $cacheResponse.IndexRenamed) { $obj.Old }The above is a bit faster than both Select-Object and Foreach-Object and shouldn't mess with the type.
There was a problem hiding this comment.
Drat. I'm reverting back to the initial foreach and add version for the renamed files as I don't see a way to use the AddRange version without doing more enumerations (extracting the property then casting from objects to strings). (Recall the earlier version with Select-Object didn't work due to Select-Object's return type.)
Please feel free to make additional tweaks if there's a faster approach.
|
🎉 Appreciate everyone's patience with this getting this merged. Thanks for the contribution, @cmarcusreid! |
Cache status information in external git-status-cache process and retrieve using git-status-cache-posh-client scripts. This pull request uses git-status-cache-posh-client's Get-GitStatusFromCache function when the module is installed. This function returns status information in a format that's easily consumable within posh-git.
The external cache is built on top of libgit2 and dramatically improves performance on large repositories. Cache hits do not become more expensive with larger repositories. Performance of both cache hits and cache misses with the cache is significantly faster than current status retrieval.
Inspired by:
My primary motivator was making posh-git usable on some very large repositories I interact with every day; this solution has worked great and has been baking for a while. Recognizing that there are previous attempts at this type of improvement I've attempted to incorporate feedback from the earlier conversations.
I'm happy to address any additional comments or feedback!
Performance
Cost for serving a cache hit in the cache process is generally between 0.1-0.3 ms, but this metric doesn't include the overhead involved in a full request.
The following measurements were taken on git repositories containing the specified file count. Each file was a text file containing a single sentence of text. Each case was run 5 times and the numbers reported below are averages. Each individual measurement was taken with a high resolution timer at 1 ms precision.
Request from git-status-cache-posh-client
The measurements below capture:
No files modified
10 files modified
Using git-status-cache-posh-client to back posh-git
The measurements below capture all the steps from the git-status-cache-posh-client section as well as the cost for posh-git to render the prompt. This extra step has a fixed cost of around 7-10 ms.
Costs were gathered using timestamps from posh-git's debug output. Timings for posh-git without git-status-cache and git-status-cache-posh-client are included for in the "posh-git without cache" column for reference.
No files modified
10 files modified