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

Avoid expensive file-truename call when possible #3430

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

vemv
Copy link
Member

@vemv vemv commented Aug 24, 2023

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 vemv requested a review from bbatsov August 24, 2023 15:55
@vemv vemv marked this pull request as ready for review August 24, 2023 15:59
@bbatsov
Copy link
Member

bbatsov commented Aug 25, 2023

Did you get some noticeable speedup from this?

@vemv
Copy link
Member Author

vemv commented Aug 25, 2023

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.

@bbatsov
Copy link
Member

bbatsov commented Aug 25, 2023

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
Copy link
Member

bbatsov commented Aug 25, 2023

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

@vemv
Copy link
Member Author

vemv commented Aug 25, 2023

Full disclosure, the biggest fix will be this one #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.

@bbatsov
Copy link
Member

bbatsov commented Aug 25, 2023

That would be nice.

@vemv vemv marked this pull request as draft August 27, 2023 19:06
@vemv
Copy link
Member Author

vemv commented Sep 17, 2023

I'll retake this PR after we get to address #3468 - otherwise not cool to further touch this area with a release around the corner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants