company-reftex icon indicating copy to clipboard operation
company-reftex copied to clipboard

Add option to disable `reftex-parse-all`

Open atreyasha opened this issue 2 years ago • 9 comments

Overview

This PR addresses #11 by adding an option to disable the call to reftex-parse-all, which could be the cause of the performance sink reported.

Tasks before merge

  • [x] Check if this PR addresses the performance issues reported in #11
  • [ ] Check if the optimizations here further improve performance. If so, we could add this link to our readme so other users could add these to their emacs configuration

atreyasha avatar Oct 01 '22 13:10 atreyasha

Thank you for that improvement.

Side note: I personally think a hook would have been better. You could have offered reftex-parse-all and reftex-parse-one as options for that hook but also give the user the possibility to use his own parser or, maybe, no parser at all. (Please don't be offended. That is just an opinion. I am happy about your efforts even if my approach would have been slightly different!)

I ask the OP to test your changes. (I do not have his LaTeX sources.)

TobiasZawada avatar Oct 02 '22 06:10 TobiasZawada

I ask the OP to test your changes. (I do not have his LaTeX sources.)

Done: https://emacs.stackexchange.com/questions/73745/company-reftex-slow-when-searching-for-labels?noredirect=1#comment120306_73745

TobiasZawada avatar Oct 02 '22 19:10 TobiasZawada

You could have offered reftex-parse-all and reftex-parse-one as options for that hook but also give the user the possibility to use his own parser or, maybe, no parser at all. (Please don't be offended. That is just an opinion. I am happy about your efforts even if my approach would have been slightly different!)

No problem and thanks for the clarification. Does #14 reflect the idea you had in mind?

In case I misunderstood something, it would be good if you could add a code suggestion so I can visualize this better.

Side note: I personally think a hook would have been better.

I see your point. The only thing I am hesitant about in regards to the hook is the (perhaps unnecessary) freedom provided to the user. AFAIK there are basically two ways to go about parsing the labels; either parse the single file or parse the multi-file document. Leaving this hook empty would render the parser useless. I also don't see any practical optimization that a user can make without going deep into reftex and patching stuff there (if there is anything to be patched at all). But if a user was able to do that, then it makes more sense to make upstream changes in reftex than company-reftex since we simply use the bindings available to us from reftex.

Do you know what I mean, or do you have another perspective?

I ask the OP to test your changes. (I do not have his LaTeX sources.)

Perfect, thank you.

atreyasha avatar Oct 02 '22 21:10 atreyasha

Hello everybody, I just wanted to tune in here on the problem. I implemented the change Tobias asked me for and tried it on my system. Sadly, the problem still persists. As soon as I enter a \ref{} command with the point; the autocomplete scans all my files for the Latex project.

UPDATE: The proposed changed did solve my problem. I did not recompile the package correctly. With the implemented changes all files still get scanned, but only once. Afterwards the performance is way better and the autocomplete suggestions turn up faster. This is a nice solution for my problem, thank you!

ls-ptr avatar Oct 04 '22 09:10 ls-ptr

Hi @Captn-LootALot. Thanks for reporting back regarding the performance. One thing to clarify though:

This is a nice solution for my problem

While setting company-reftex-labels-parse-all to nil does solve your current problem, this could introduce a new problem as originally described in https://github.com/TheBB/company-reftex/issues/11#issue-831685128. With this option being set to nil, if you change a label in one LaTeX file A and autocomplete \ref{} in a LaTeX file B (assuming they are part of a larger document); you will no longer (automatically) have access to the changed label in LaTeX file A. Whereas with company-reftex-labels-parse-all being t, this multifile document parsing would happen automatically. Just wanted to state this as a FYI.

Afterwards the performance is way better and the autocomplete suggestions turn up faster.

Overall, I am on board to merge this PR as it constitutes a trivial but effective change. As mentioned in https://github.com/TheBB/company-reftex/pull/13#issuecomment-1264736701, I am less comfortable with having a customizable hook unless @TobiasZawada or @TheBB have strong opinions in favour of the customizable hook?

PS: I created a new PR #14 for the customizable hook so we keep the diffs in separate branches

atreyasha avatar Oct 05 '22 08:10 atreyasha

Hello @atreyasha thank you for the heads-up. Whenever I need to scan all documents again I usually call company-reftex-labels inside my Emacs editor. This works just fine.

ls-ptr avatar Oct 10 '22 06:10 ls-ptr

I think there is a misunderstanding here.

Whenever I need to scan all documents again I usually call company-reftex-labels inside my Emacs editor

This PR will change how company-reftex-labels works, such that calling company-reftex-labels with the company-reftex-labels-parse-all variable being nil will no longer scan all ~documents~ files in a document. So the behaviour you mentioned that "works just fine" will no longer do the same thing post-merge. Below is a MWE to verify my point.

MWE

Environment (environment.tar.gz)

init.el

;;------------------------------------------------------------------------------
;; use-package and quelpa
;;------------------------------------------------------------------------------

;; setup archives
(require 'package)
(setq package-user-dir (file-name-directory (or load-file-name (buffer-file-name))))
(add-to-list 'package-archives '("gnu"   . "https://elpa.gnu.org/packages/"))
(add-to-list 'package-archives '("melpa" . "https://melpa.org/packages/"))
(package-initialize)

;; setup use-package
(unless (package-installed-p 'use-package)
  (package-refresh-contents)
  (package-install 'use-package))
(eval-and-compile
  (setq use-package-always-ensure t
        use-package-expand-minimally t))

;; setup quelpa
(unless (package-installed-p 'quelpa)
  (with-temp-buffer
    (url-insert-file-contents "https://raw.githubusercontent.com/quelpa/quelpa/master/quelpa.el")
    (eval-buffer)
    (quelpa-self-upgrade)))
(quelpa '(company-reftex :fetcher git
                         :url "https://github.com/TheBB/company-reftex"
                         :branch "as/parse-all-defcustom"))

;;------------------------------------------------------------------------------
;; company and reftex
;;------------------------------------------------------------------------------

;; enable RefTex in emacs latex mode
(add-hook 'latex-mode-hook 'turn-on-reftex)

;; install and enable company-reftex
(use-package company
  :config
  (setq company-idle-delay nil))
(global-company-mode t)
(use-package company-reftex)

;; disable reftex-parse-all for labels
(setq company-reftex-labels-parse-all nil)

;; add new backend
(add-to-list 'company-backends 'company-reftex-labels)

main.tex

\documentclass{article}

\begin{document}
  \include{file_1}
  \include{file_2}
\end{document}

%%% Local Variables:
%%% mode: latex
%%% TeX-master: t
%%% End:

file_1.tex

\label{Chapter 1}

%%% Local Variables:
%%% mode: latex
%%% TeX-master: "main"
%%% End:

file_2.tex

\label{Chapter 2}

%%% Local Variables:
%%% mode: latex
%%% TeX-master: "main"
%%% End:

Directory structure for latex files

.
├── file_1.tex
├── file_2.tex
└── main.tex

0 directories, 3 files

Steps to reproduce

  1. Launch emacs using the aforementioned simplified init.el via emacs -q -l /path/to/simplified/init.el.

    Note: This will dump installed packages in the same directory as init.el, so it makes sense to place this init.el outside of ~/.emacs.d/

  2. Open file_1.tex and file_2.tex

  3. Open the buffer for file_1.tex and execute company-reftex-labels with the cursor inside \ref{} in a new line. You should see autocompletions for Chapter 1 and Chapter 2:

    \label{Chapter 1}
    \ref{}  ;; cursor inside the curly brackets
    
    %%% Local Variables:
    %%% mode: latex
    %%% TeX-master: "main"
    %%% End:
    
  4. Add a new line in file_2.tex containing \label{foo} and save this file:

    \label{Chapter 2}
    \label{foo}
    
    %%% Local Variables:
    %%% mode: latex
    %%% TeX-master: "main"
    %%% End:
    
  5. Repeat step 3. You should see autocompletions for Chapter 1 and Chapter 2, but not for foo

  6. Evaluate (setq company-reftex-labels-parse-all t)

  7. Repeat step 3. You should see autocompletions for Chapter 1, Chapter 2 and foo

Hope this clarifies. I will wait for responses from @TobiasZawada and/or @TheBB before merging.

atreyasha avatar Oct 10 '22 11:10 atreyasha

Thank you for coming back to this. I tested your MWE and you are absolutely right. I do understand now that what I earlier wrote cannot be possible. I will take a look at my earlier setup and see what I did wrong. Again, thank you!

ls-ptr avatar Oct 11 '22 06:10 ls-ptr

Thanks @atreyasha, this is a nice fix. I often work with multi-file latex projects, and this PR improves performance drastically. I just call reftex-parse-all when I change a label in another file (which is not so common, so the performance increase is really worth the tradeoff).

WannabeSmith avatar Feb 26 '23 18:02 WannabeSmith