cider icon indicating copy to clipboard operation
cider copied to clipboard

Avoid expensive `file-truename` call when possible

Open vemv opened this issue 2 years ago • 7 comments

See also https://clojurians.slack.com/archives/C0617A8PQ/p1692876466054909

file-truename can be slow (different Emacs projects work around this in a number of ways). Apparently, it checks for symlinks as it traverses each directory of a given path.

This PR observes the buffer-file-truename buffer-local variable, that has that info already computed for us.

https://www.gnu.org/software/emacs/manual/html_node/elisp/Buffer-File-Name.html#index-buffer_002dfile_002dtruename

I've tried it successfully locally, and over TRAMP.

vemv avatar Aug 24 '23 15:08 vemv

Did you get some noticeable speedup from this?

bbatsov avatar Aug 25 '23 11:08 bbatsov

I couldn't reproduce any slowness to begin with.

Perhaps I could if I set up an intentionally slow TRAMP connection, but that's quite the setup.

I found it enough to check that values were equivalent for all cases.

vemv avatar Aug 25 '23 11:08 vemv

I'm just way of making changes without being able to demonstrate the value of the change (as the proposed code is a bit more complex than the original one).

bbatsov avatar Aug 25 '23 11:08 bbatsov

I guess you can also try to have whoever complained about this to test your proposed fix?

bbatsov avatar Aug 25 '23 11:08 bbatsov

Full disclosure, the biggest fix will be this one https://github.com/clojure-emacs/cider/pull/3435

Still, this looks to me like a worthwhile fix:

  • we're using a built-in value that's already computed for us
  • other Emacs packages have detected a slowness in file-truename

Therefore it would seem sensible to prevent a performance issue, instead of waiting for someone to experience it (which doesn't even guarantee that we will get it reported!)

Perhaps I can tidy up things by extracting the helper to a defun? And show with buttercup tests its equivalence.

vemv avatar Aug 25 '23 11:08 vemv

That would be nice.

bbatsov avatar Aug 25 '23 11:08 bbatsov

I'll retake this PR after we get to address https://github.com/clojure-emacs/cider/issues/3468 - otherwise not cool to further touch this area with a release around the corner

vemv avatar Sep 17 '23 15:09 vemv