universal-ctags / citre

A superior code reading & auto-completion tool with pluggable backends.
GNU General Public License v3.0
326 stars 26 forks source link

core, fix: readtags process problems on Windows #64

Closed AmaiKinono closed 3 years ago

AmaiKinono commented 3 years ago

@kaiwk Could you try this branch?

If you install Citre from melpa, you'll need to manually clone this repo, switch to fix-readtags-process branch, then following the instructions for manual installation in README to use this version in your Emacs.

I tried it on Windows, in the chromium repo. Emacs is still a bit laggy but not frozen, and I think those left-over processes you saw has gone.

Since my Emacs config is slow on windows, I can't tell if it's still the problem of Citre or not.

I have some more ideas for optimization (but they are also somewhat dangerous. Handling async process is not easy). If you still notice any laggy, please tell me.

kaiwk commented 3 years ago

I tried it on Windows, in the chromium repo. Emacs is still a bit laggy but not frozen, and I think those left-over processes you saw has gone.

Yes, same here. I'd say it's good enough for daliy use, thanks.

kaiwk commented 3 years ago

While *readtags-error* buffer has gone, I found there are many readtags process buffer created (they will disappear after a while):

SodaPDF-converted-2021630-11102

at the same time, company-capf gives error:

SodaPDF-converted-2021630-111010

AmaiKinono commented 3 years ago

@kaiwk could you try this again, with the latest commit?

Hopefully you won't feel any lag. I tested on Windows, in the chromium repo, with your config. It's buttery smooth when typing ;)

kaiwk commented 3 years ago

@AmaiKinono Hey, it's much better!

but i still notice a significant lag, you could reproduce by:

  1. turn subword-mode
  2. type SetEnabled|
  3. press M-backspace, Set|
  4. press backspace again, you will probably have a significant delay to get Se|

I think it's not the problem of subword-mode, you can also reproduce it by:

  1. type SetEnabled|
  2. keep pressing backspace
  3. you will probably have a significant delay from Set| to Se|

But with subword-mode enabled will increase the probability of the reproduction.

AmaiKinono commented 3 years ago

I can't reproduce this. There's no lag when I follow the steps you gave. Could you try with vanilla Emacs? You could do this by:

$ emacs -Q

then follow the installation guide in README. You also need to require company and set it up.

Btw, which version of Emacs are you using? I'm on 27.2.

AmaiKinono commented 3 years ago

I've found some of the completions are missing when citre-capf-optimize-for-popup is enabled. This is a serious bug.

Please, wait a while till I fix this, and I'll ping you.

AmaiKinono commented 3 years ago

I'll merge this as it's a good fix, and partially solves #61 (and the missing completion bug I mentioned above), but now I have absolute no idea how to deal with the lag you've experienced.

Notice: What I put below turns out to be my misunderstanding. See #67.


Please, let me explain the situation now. It's a misfortune caused by a series of coincendences.

First, Citre uses async process for readtags. This is because:

Second, we still need to wrap it as a synchronous interface, as Emacs is basically a synchronous toy. The capf, xref, imenu interfaces... they are all synchronous.

So we need a way to poll for the process to finish. The right way to do this is:

(while (accept-process-output readtags-process))

You may also think about something like:

(while (process-live-p readtags-process)
  (sleep-for 0.05))

Unfortunately, process-live-p can return nil, while there remains some output from that process that has not been processed. So using accept-process-output is the only reliable way for polling.

If you wrap (accept-process-output) using (while-no-input), then user input interrupts it. This is the reason why you can type smoothly in a huge project while using Citre. Surprisingly, this only works on Linux and macOS. It's reported here.

Let me put this again: On windows, you can't break a polling async process by user input. That's why you feel the lag.

I have several ideas to fix this:

  1. What we did before is to assemble a shell command to use readtags. Using some clever combinations of pipe, head, shell builtin variables... we can stop the readtags process after we've received one line.

    The disadvantage is this requires a POSIX shell being the inferiour shell of Emacs. Not good for Windows users. Besides, it increases the risk of (unintended) shell injection attack.

  2. Ask readtags to add an option to quit after printing n lines, so there's no need to use async process, then we eliminate all the async quirks once and for all.

    A similar approach is to grep the tags file. Grep has a --max-count option which could help. But we add another external dependency this way.

  3. Find a way to use the tags file so that we don't need to know if it contains relative paths beforehand, so there's no need to use async process.

The path I want to take is 3. It may take a few days, so I'm sorry @kaiwk that you can't use Citre for auto-completion in big projects for now. I suggest you temporarily do:

(setq-default citre-enable-capf-integration nil)

to disable the auto-completion part, and enjoy other tools in Citre for now.

AmaiKinono commented 3 years ago

Find a way to use the tags file so that we don't need to know if it contains relative paths beforehand

This can't be done easily.

We need the current working directory info (i.e. what are those relative paths relative to) before we filtering a tags file, or we can't write an accurate filter on the input field, conceptually like:

(or (eq path-from-tag target-absolute-path) (eq path-from-tag target-relative-path))

If we don't know CWD, we can't calculate target-relative-path from target-absolute-path.

The CWD info can be easily got if we require the tags file being generated by Universal Ctags (it has a TAG_PROC_CWD pseudo tag for this).

The problem is some users may use othet tools to create a tags file. I've solved #37 for this. The approach I took was to guess the directory of the tags file is the CWD, then we expand a relative path in the tags file against it. If it exists, we believe we've made a successful guess.

The problem is we need to get a tag with relative path, which takes us back to the dilemma.

One way to solve this is: when we can't find the TAG_PROC_CWD ptag, simply use the directory of the tags file as CWD. We minimize the harm of this approach by stating it clearly in the documents, and encouraging prople to use Universal Ctags.

@masatake if you are interested, could you read this and my comment above? It's OK if you don't have time. I think

when we can't find the TAG_PROC_CWD ptag, simply use the directory of the tags file as CWD

is a good enough solution.

masatake commented 3 years ago

@AmaiKinono, I have not read all the comments. I only read of the part of the last comment.

when we can't find the TAG_PROC_CWD ptag, simply use the directory of the tags file as CWD

If citre can't find the TAG_PROC_CWD ptag in the target tags file, I think citre should reject the tags file. There can be two issues in this approach.

A. if a user does "mv" the source tree, the tags file for the tree becomes inconsistent. B. users cannot use tags generators other than u-ctags. C. (add you finds)

For A, I'm thinking about providing a tool setting TAG_PROC_CWD of a tags file. The tool can be run lie: edittags -t tags set-ptags TAG_PROC_CWD /new/directory/where/src/is.

For B, adding TAG_PROC_CWD to the tags geneator the users want to use can be a solution if the generator is an open source software.

when we can't find the TAG_PROC_CWD ptag, simply use the directory of the tags file as CWD

This can be a fallback method. I guess client tools implemented on vim may choose this approach.

I come up with following pseudo code:

(unless (tag-files-has-PROC_CWD-p)
   (when (yes-or-no-p (format "the tags file has no PROC_CWD. Do you want set \"%s\" as PROC_CWD" tags-dir))
       (run-process "edittags -t tags set-ptags TAG_PROC_CWD %s" tags-dir))) 

(You may know that -D option of readtags can be used for finding PROC_CWD quickly).

AmaiKinono commented 3 years ago

adding TAG_PROC_CWD to the tags geneator the users want to use can be a solution if the generator is an open source software.

Ah, this clicks for me. I agree edittags is the correct solution, for the long run. We should:

I also suggest a remove-ptag action, as tags files in our test cases don't have TAG_PROC_CWD ptag, and we need keep the file content consistent before & after running unit tests.

(unless (tag-files-has-PROC_CWD-p)
  (when (yes-or-no-p (format "the tags file has no PROC_CWD. Do you want set \"%s\" as PROC_CWD" tags-dir))
      (run-process "edittags -t tags set-ptags TAG_PROC_CWD %s" tags-dir))) 

I think we don't want to prompt the user, since the additional info is updated everytime the tags file is updated (we check the latest modified time of it).

Another way is allow the caller to pass a guessed CWD. Since there's the "project" concept in the upper components, they will do a better job than citre-core.el.

Anyway, you've pointed the correct way in the long run, so I know we don't need a perfect solution at present. Thank you!