-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Really slow prompt inside large git repositories even after disabling git status. #2484
Comments
Apparently Cmder has two git prompt filters.
Also, having two separate git prompt filters (and mercurial, and etc) might additionally be doubling the delay by doing everything twice. |
So |
Yes. But Cmder also includes a whole separate copy of the source control prompt filters, inside The git prompt filter in 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. |
It is my understanding that setting the The result should be a replacement of I can disable Git status as detailed in the How can you tell |
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. |
Unfortunately, I cannot share the relevant repo (company property), but will share the contents of my |
@daxgames unfortunately it's a bug in All exit paths from
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. |
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 This is considering this is indeed the issue; disregard if it turns out be something else. |
One is part of the Since Cmder needs some different behaviors, it's not as simple as just deleting one of them. However, there are other options:
My vote would be for option 2, so far. |
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 The above should be in addition to a global way to opt out controlles by |
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.
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. |
@itsagam can you share specific examples? "inside large git repositories" leaves everything to interpretation.
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 Update: I don't see anything in |
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 |
Interesting, thanks. Looking forward to the additional info. Also, @itsagam is there only a delay after |
@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. |
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 😄). |
No problem I really appreciate your interest and willingness to help. |
The solution is rather simple, don't execute Configuration files: Files.zip |
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. |
@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. |
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. |
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.
|
Here is the repo for clink-completions. |
got some free time - starting at look at this |
Fixed Cmder: https://ci.appveyor.com/project/MartiUK/cmder/builds/38602809/artifacts Requires the following two lines to be added to
|
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 |
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. |
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.
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
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. |
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! 😁 |
Purpose of the issue
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.The text was updated successfully, but these errors were encountered: