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

Really slow prompt inside large git repositories even after disabling git status. #2484

Closed
1 task
itsagam opened this issue Feb 25, 2021 · 32 comments · Fixed by #2520 or vladimir-kotikov/clink-completions#141
Assignees

Comments

@itsagam
Copy link

itsagam commented Feb 25, 2021

Purpose of the issue

  • Bug report (encountered problems/errors)

Version Information

Cmder v1.3.17.1082, ConEmu v191012

Description of the issue

Even after you disable git status with the instructions provided in the README file, prompts appear after much delay inside large git repos. Deleting vendor\clink-completions\git_prompt.lua does the trick, however.

@chrisant996
Copy link
Contributor

chrisant996 commented Feb 25, 2021

Apparently Cmder has two git prompt filters.

  • The one inside vendor\clink.lua checks the cmder.status and cmder.cmdstatus git config settings.
  • The one inside vendor\clink-completions\git_prompt.lua does not.
    • And the clink-completions repo is not specific to Cmder, so when adding a check it will need a way to know that it's embedded in Cmder and only check in that case.

Also, having two separate git prompt filters (and mercurial, and etc) might additionally be doubling the delay by doing everything twice.

@daxgames
Copy link
Member

So vendor\clink.lua checks the Git config for cmder.status and cmder.cmdstatus as a way to disable Git status checks for this very complaint. You can look in the README.md for how to disable Git status checks both globally and per Git Repo.

@chrisant996
Copy link
Contributor

chrisant996 commented Feb 25, 2021

Yes. But Cmder also includes a whole separate copy of the source control prompt filters, inside vendor\clink-completions.

The git prompt filter in vendor\clink.lua checks git config.
The git prompt filter in vendor\clink-completions\git_prompt.lua does not check git config.

So, the instructions and mechanism for opting out of git status aren't effective.

@itsagam
Copy link
Author

itsagam commented Feb 25, 2021

Yes. But Cmder also includes a whole separate copy of the source control prompt filters, inside vendor\clink-completions.

The git prompt filter in vendor\clink.lua checks git config.
The git prompt filter in vendor\clink-completions\git_prompt.lua does not check git config.

So, the instructions and mechanism for opting out of git status aren't effective.

Can confirm. Just tried Cmder today and had to spent quite a while going down this rabbit hole. Disabled git status with the instructions from the README and tried multiple solutions from this thread #447. They do disable the prompt but the delay is there. Clink.lua was iterating through all the lua files in the completions directory and executing them, one of which was another copy of the git_prompt_filter.

@daxgames
Copy link
Member

It is my understanding that setting the git_prompt_filter in vendor/clink.lua to a lower number thus setting a higher prority of 50 than git_prompt_filter in vendor/clink-completions/git-prompt.lua which is set at 60 and returning false from git_prompt_filter in vendor/clink.lua that no other prompt filters run after git_prompt_filter in vendor/clink.lua finishes.

The result should be a replacement of git_prompt_filter in vendor/clink-completions/git-prompt.lua withgit_prompt_filter in vendor/clink.lua

I can disable Git status as detailed in the README.md and see a difference in prompt speed even in a small repo like Cmder. I unfortunately do not have a 'large' repo to test with. Can yoy suggest something publicly available I can try with?

How can you tell clink.lua is looping all lua files in vendor/clink-completions/? Does it just become obvious if the git repo is large enough?

@chrisant996
Copy link
Contributor

I will double check this evening. Maybe I misread and the problem is something else. But clearly the git_prompt.lua prompt filter is somehow running.

@itsagam
Copy link
Author

itsagam commented Feb 26, 2021

I can disable Git status as detailed in the README.md and see a difference in prompt speed even in a small repo like Cmder. I unfortunately do not have a 'large' repo to test with. Can yoy suggest something publicly available I can try with?

How can you tell clink.lua is looping all lua files in vendor/clink-completions/? Does it just become obvious if the git repo is large enough?

Unfortunately, I cannot share the relevant repo (company property), but will share the contents of my ~/.gitconfig and a vod before and after deleting vendor/clink-completions/git_prompt.lua on the next weekday. Was a 3-second delay before and results were almost instantaneous afterwards.

@chrisant996
Copy link
Contributor

chrisant996 commented Feb 26, 2021

It is my understanding that setting the git_prompt_filter in vendor/clink.lua to a lower number thus setting a higher prority of 50 than git_prompt_filter in vendor/clink-completions/git-prompt.lua which is set at 60 and returning false from git_prompt_filter in vendor/clink.lua that no other prompt filters run after git_prompt_filter in vendor/clink.lua finishes.

@daxgames unfortunately it's a bug in git_prompt_filter in vendor\clink.lua:

All exit paths from git_prompt_filter return false.
Returning false means "continue filtering the prompt".
Here is a quote from the Clink v0.4.9 docs:

The filter function takes no arguments instead receiving and modifying the prompt through the clink.prompt.value variable. It returns true if the prompt filtering is finished, and false if it should continue on to the next registered filter.

But also, I think it would be a mistake for Cmder to try to block all other Clink prompt filters, as a way of trying to prevent one specific filter in clink-completions from running. It would also prevent cmder-powerline-prompt filters and various other custom prompt filters (all of my own, in fact).

I think a less invasive solution would be preferable.

@itsagam
Copy link
Author

itsagam commented Feb 26, 2021

Not really acquainted with how it is coded — I’m sure there’s a cause — so dumb question incoming: what’s the specific reason git_prompt is implemented in two places? Can’t we just delete one of the instances, and call it a day? Or is the git-related code in clink.lua and completions/git_prompt.lua working in tandem? Can’t we just merge the code if it is?

This is considering this is indeed the issue; disregard if it turns out be something else.

@chrisant996
Copy link
Contributor

chrisant996 commented Feb 26, 2021

what’s the specific reason git_prompt is implemented in two places? Can’t we just delete one of the instances, and call it a day?

One is part of the clink-completions vendor package, which is also used independently from Cmder.

Since Cmder needs some different behaviors, it's not as simple as just deleting one of them.

However, there are other options:

  1. Could use the Clink API settings.add to create a Clink setting for opting out. And then both implementations could use the same setting. (Requires new Clink; won't work with Clink 0.4.9). This would change how one opts out, but it would be possible to add code in vendor\clink.lua to automatically migrate the setting from git config.
  2. Could make a global variable or function in the vendor\clink.lua file for checking whether git status is opted out. Could make the clink-completions version check whether the Cmder-specific bar/func exists, and respond accordingly. Then clink-completions could with with/without Cmder, without needing to change how you opt out in Cmder.
  3. Could give up and make clink-completions look at cmder git config settings directly. Less clean of an approach, though.
  4. (There are many other variations of options, too.)

My vote would be for option 2, so far.

@daxgames
Copy link
Member

I am not a lua expert and only delve into it when absolutely required. The current opt out was done the way it was so a user could opt out globally or only for selected repos where operation was extremwly slow.

I assume performance could be gained by setting a global variable in vendor\clink.lua that the clink-completions git prompt filter could look for and return false instead of doing double work if in every git repo?

The above should be in addition to a global way to opt out controlles by vendor\clink.lua.

@chrisant996
Copy link
Contributor

chrisant996 commented Feb 27, 2021

I am not a lua expert and only delve into it when absolutely required. The current opt out was done the way it was so a user could opt out globally or only for selected repos where operation was extremwly slow.

I love that it can be configured per repo, per user, or globally, etc!

The current implementation does not successfully opt out, unfortunately. (Not with mridgers clink, and not with chrisant996 clink).

It does cut the cost in half, though.

I assume performance could be gained by setting a global variable in vendor\clink.lua that the clink-completions git prompt filter could look for and return false instead of doing double work if in every git repo?

The above should be in addition to a global way to opt out controlles by vendor\clink.lua.

That would enable the config to successfully opt out.

I'll look at differences between Cmder's filters and clink-completion's filters (it's all of them getting double-executed, not just git).

If they're similar enough, it may be reasonable to consolidate them into a single implementation. Or to tell the clink-completion ones to never run inside Cmder.

@chrisant996
Copy link
Contributor

chrisant996 commented Feb 27, 2021

vendor\clink.lua

  • finds the git branch and puts it in the prompt as "origin/master".
  • uses git status to pick what color to use for the branch name.

vendor\clink-completions\git_prompt.lua

  • reformats it as "master -> origin".
  • does not seem to run git status, so I don't know why it would be slow!

@itsagam can you share specific examples? "inside large git repositories" leaves everything to interpretation.

  • can you quantity "large", or give the URL of a specific large repo?
  • where "inside" the repo is the prompt slow? In the git root directory? Or only in subdirectories of the repo? Does it depend on how many files the parent directories of the current directory have?

Once I'm pointed to a specific example that reproduces the problem, then I can track down why it's slow, and there's a good chance I can just make a fix in the clink-completions scripts.

Update: I don't see anything in vendor\clink-completions\git_prompt.lua that looks like it could potentially not be fast. I'm also unable to reproduce it being slow -- it's always instantaneous for me in the repos I've tried. The VSCode repo seems fairly large and doesn't reproduce the problem for me. A specific example that reproduces the problem would be helpful for figuring out the root cause of the slowness.

@itsagam
Copy link
Author

itsagam commented Feb 27, 2021

The repo in question is a Unreal Engine 4 video-game around 100GB in size. It had git-lfs enabled. The prompt had around a 3-second delay whenever you ls / dir/ cd inside a directory in the repo directory. It’s possible I’m mistaken in some way, but I’m pretty darn sure deleting git_prompt.lua solved the issue. Give me till Monday, I’ll look at the code and tell you the problem.

@chrisant996
Copy link
Contributor

chrisant996 commented Feb 27, 2021

Interesting, thanks. Looking forward to the additional info.

Also, @itsagam is there only a delay after ls, dir, cd? Or is there a delay after all commands, and even just when pressing Enter with an empty command line?

@daxgames
Copy link
Member

@chrisant996 I am glad you found what I thought in the clink-completions git prompt in that it does not do much. It really only says are we in git repo, if so what is the branch set the prompt.

That is some of the limited lua I have actually read and contributed to. I think lime 3-4 lines of that file are mine.

@chrisant996
Copy link
Contributor

@chrisant996 I am glad you found what I thought in the clink-completions git prompt in that it does not do much. It really only says are we in git repo, if so what is the branch set the prompt.

Yes, you were right, sorry about that. I hadn't looked closely enough at specifically what the clink-completions prompt filter was actually doing (or rather not doing 😄).

@daxgames
Copy link
Member

No problem I really appreciate your interest and willingness to help.

@itsagam
Copy link
Author

itsagam commented Mar 1, 2021

  1. The git_prompt_filter in clink.lua checks for the git status setting, and doesn't run if it's false.

  2. The git_prompt_filter in clink-completions/git-prompt.lua runs either way.

  3. Crucially, the git_prompt_filter in clink.lua doesn't override the one in git-prompt.lua.

  4. The load_ini in git-prompt.lua is the offending function. For git repositories with git-lfs enabled, the .git/config can grow fairly large. Ours is 34k lines (attached). load_ini goes through each line and runs a regular expression. It's called twice separately in git-prompt.lua.

The solution is rather simple, don't execute git-prompt.lua if the git status setting is disabled. That should be done at the very least and what a user expects. Disabled means disabled. The git filters are also ran twice if it's enabled.

With git_prompt.lua:
With_git_prompt_lua

Without git_prompt.lua:
Without_git_prompt_lua

Configuration files: Files.zip

@chrisant996
Copy link
Contributor

Ah! Thank you, @itsagam, that makes sense.

@daxgames yeah we need that global variable as speculated. Cmder's git prompt can set it each time saying whether it skipped filtering the prompt, and clink-completions can check the variable.

Do you want to make the changes? Or I'm happy to, but I probably won't get to it for about a week.

@daxgames
Copy link
Member

@chrisant996 I can try to make the changes. Will give me a chance to do a little Lua.

@chrisant996
Copy link
Contributor

@chrisant996 I can try to make the changes. Will give me a chance to do a little Lua.

Sounds good. Most of my free time has gone to Clink itself lately, and this had fallen off my radar.

@nathonius
Copy link

Any update on this? Until we can containerize everything (we're getting there slowly but surely) we're rolling up our entire app into a single 1.3gb Angular project with 25 git submodules. 😁 Every command in that directory takes ~5s to execute. I'm not familiar with the architecture of clink + cmder, but if @daxgames you don't have time I can try and take a look.

@daxgames
Copy link
Member

daxgames commented Apr 6, 2021

I have been really busy at my day job and have been unable to look at this as I said I would. Unfortunately there is no end in sight.

There are two steps to this.

  1. make Cmder %cmder_root %\vendor\clink.lua set a global variable that git status is opted in[default] or out.

  2. That variable can be accessed from Clink completions %cmder_root %\vendor\clink-completions\git-promp.lua to turn off prompt filtering and more specifocally the load_ini function if set.

@chrisant996
Copy link
Contributor

Here is the repo for clink-completions.

@daxgames
Copy link
Member

daxgames commented Apr 7, 2021

got some free time - starting at look at this

@daxgames
Copy link
Member

daxgames commented Apr 8, 2021

vladimir-kotikov/clink-completions#141

@daxgames
Copy link
Member

daxgames commented Apr 8, 2021

Fixed Cmder: https://ci.appveyor.com/project/MartiUK/cmder/builds/38602809/artifacts

Requires the following two lines to be added to %cmder_root%vendor\clink-completions\git_prompt.lua functionload_ini(fileName)`:

local function load_ini(fileName)
    -- Check for Cmder configured Git Status Opt In/Out - See: https://github.com/cmderdev/cmder/issues/2484
    if cmderGitStatusOptIn == false then return nil end

@vladimir-kotikov
Copy link
Contributor

Thanks for the PR - i'll try to take a look ASAP.

However fixing this by using global variable to opt-out feels a little hacky to me - Ideally I'd like to fix git config parsing to make it faster and then use the same mechanism as Cmder uses by reading oprions out of git config. @itsagam is it possible to get the example of .gitconfig file that demonstrates slow parsing - I'll try to find out whether there's maybe a faster implementation of parser or it's possible to do some optimisations to parsing process.

@chrisant996
Copy link
Contributor

However fixing this by using global variable to opt-out feels a little hacky to me - Ideally I'd like to fix git config parsing to make it faster and then use the same mechanism as Cmder uses by reading oprions out of git config. @itsagam is it possible to get the example of .gitconfig file that demonstrates slow parsing - I'll try to find out whether there's maybe a faster implementation of parser or it's possible to do some optimisations to parsing process.

My suggestion to use a global variable was intended to avoid duplicating code. Having two implementations that each figure out whether to not do work seems like a missed opportunity for optimization. It also means if any issues are found later, they have to be fixed in two places.

@vladimir-kotikov
Copy link
Contributor

The code is already duplicated (well, partially) and adding a global variable will not help getting rid of this duplication (this is also undesirable, at least from clink-completions perspective as they still theoretically can work separately from Cmder). Rather this looks to me as a quick and dirty fix to disable prompt processing as there's no other enough performant ways to do this.

if any issues are found later, they have to be fixed in two places

That's not really applicable here - these files while both deal with shell prompt, do quite different things so it's unlikely that the same issue will affect them both

that each figure out whether to not do work seems like a missed opportunity for optimization

Well, as I said there's already a documented mechanism to disable prompt processing so IMO it looks natural that this mechanism will also be picked up by other prompt processing facilities, not only Cmder's embedded functionality. For me the only thing that prevents using it is that completions still have to parse git config for that which have sub-optimal performance and have to be tuned up first.

@chrisant996
Copy link
Contributor

chrisant996 commented Apr 9, 2021

I believe there is value in having one implementation for checking whether the git prompt should be suppressed.

If there is interest in making clink-completions support this by itself, then perhaps the code could be moved from Cmder into clink-completions, and Cmder could call an exported function. As you pointed out, clink-completions doesn't need to take a dependency on Cmder, so while there are situations where it makes sense to export from Cmder, this situation can be cleaner if clink-completions exports something (function or variable). Because Cmder already has a dependency on clink-completions.

However, this does feel like it might be over engineering a solution. I'm not convinced the cost is worth it. This started out as a simple and inexpensive tweak with huge benefit. The cost keeps growing, and I'm not sure what tangible benefits are gained by the added cost. I try to find a "sweet spot" that balances cost vs benefit, simply because my time is limited. Others may not have that limitation, in which case I get as excited as anyone about aiming for design perfection when there are no constraints! 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
@vladimir-kotikov @nathonius @itsagam @daxgames @chrisant996 and others