bug#70036: 30.0.50; Move file-truename to the C level

[Theodor Thornhill via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Wed, 27 Mar 2024 20:08:54 +0100
>> From:  Theodor Thornhill via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> During the last couple of weeks I've been studying Eglots performance
>> and have been noticing a couple of things that I find very
>> interesting. It seems like `file-truename` is in the hot path due to the
>> fact that every request to the lsp server has to create the source file
>> location, and in every response we have to parse the location the
>> relevant file. `file-truename` is used for this, and its performance
>> isn't really up to snuff for what it provides, afaict.
>> 
>> Below I've supplied some benchmarks and profile reports along with the
>> actual patch. Before we discuss the patch itself, I want to get some
>> answers to the following:
>> 
>>  - Is there a reason that this function should be supplied at the lisp
>>    level?
>
> No, we could have it implemented in C.  It just never was needed,
> until now, and the processing there is not trivial, to say the least.

Yeah, the source is complicated, but it seems most of it is to gradually
chop the path shorter and shorter.


>
>>  - Does it have to be recursive?
>
> No, it doesn't.
>

I guess making it iterable is something worth checking out anyway, so
I'll look into that a little further.

>> Firstly, I'll show some benchmarks
>> 

[...]

>> 
>> As you can see, the C implementation, though naive for now is two orders
>> of magnitude faster, and makes a noticeable difference when running an
>> lsp server in emacs.
>
> Yes, but comparing a partial implementation is not very useful, since
> the complete one could be much more expensive.
>

No doubt. The most interesting part of that benchmark is maybe to see
that the current version is very slow, not that my function is very
fast. I'd guess that even if I'd lose an order of magnitude keeping
feature parity we're better off.

>> As for the patch - it now relies on wordexp to resolve the paths, and I
>> believe there is no real feature parity with the old variant as for now,
>> but I haven't seen any issues thus far. If this approach is accepted I
>> will of course make sure we have feature parity, unless that isn't
>> wanted.
>
> We cannot rely on wordexp and we cannot rely on realpath: both are not
> portable enough.
>

OK - for my education on the portability argument. Is that because of
Emacs support targets like haiku and old versions of windows, or
something else inherent in these functions?

>> +  CHECK_STRING (filename);
>> +  char *c_filename = SSDATA (filename);
>> +
>> +  wordexp_t we;
>> +  wordexp(c_filename, &we, 0);
>> +
>> +  char *truename = realpath(we.we_wordv[0], NULL);
>> +  wordfree(&we);
>> +
>> +  if (!truename)
>> +    return result;
>> +
>> +  result = build_string(truename);
>
> You cannot pass Lisp strings to libc functions like that: you need to
> do 2 things first:
>
>   . call expand-file-name
>   . encode the file name with ENCODE_FILE
>
> This is needed because relative file names in Emacs are relative to
> the current buffer's directory, not relative to the current directory
> of the Emacs process, and because file names with non-ASCII characters
> need to be encoded to match the encoding expected by file-related APIs
> in libc.  Likewise, when you get a file name from a libc function, you
> need to decode it with DECODE_FILE, before you create a Lisp string
> from it
>
>> +  free(truename);
>
> IMO, this should be xfree, not free.  And for that to work, we need to
> call realpath with 2nd argument non-NULL, but pointing to a buffer we
> allocated with xmalloc, or maybe a stack-based buffer.  (But since we
> cannot rely on realpath, this could be a moot point.)
>
> Thanks.

Thanks for the pointers here. I'll take note of them and investigate
further.


Another much simpler way to improve Eglot performance her could be to
allow for the relevant functions to execute through handlers, to not
break other parts of Emacs. For example `find-buffer-visiting` could
allow to run through a simpler function that merely expands and looks up
the current file, considering that the LSP server likely reports on
files that are already existing, and likely most symlink shenanigans
aren't an issue here. Just thinking out loudly on this.

Theo