From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Theodor Thornhill via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#70036: 30.0.50; Move file-truename to the C level Date: Wed, 27 Mar 2024 22:56:41 +0100 Message-ID: <87frwbxrs6.fsf@thornhill.no> References: <87le63xzjt.fsf@thornhill.no> <861q7vihnm.fsf@gnu.org> Reply-To: Theodor Thornhill Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="28575"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 70036@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Wed Mar 27 22:57:38 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1rpbH7-0007Dv-OO for geb-bug-gnu-emacs@m.gmane-mx.org; Wed, 27 Mar 2024 22:57:38 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rpbGc-000520-6x; Wed, 27 Mar 2024 17:57:06 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rpbGZ-00051o-SS for bug-gnu-emacs@gnu.org; Wed, 27 Mar 2024 17:57:04 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1rpbGX-0002Q7-TL for bug-gnu-emacs@gnu.org; Wed, 27 Mar 2024 17:57:03 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rpbGX-0000EQ-KL for bug-gnu-emacs@gnu.org; Wed, 27 Mar 2024 17:57:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Theodor Thornhill Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 27 Mar 2024 21:57:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 70036 X-GNU-PR-Package: emacs Original-Received: via spool by 70036-submit@debbugs.gnu.org id=B70036.1711576615878 (code B ref 70036); Wed, 27 Mar 2024 21:57:01 +0000 Original-Received: (at 70036) by debbugs.gnu.org; 27 Mar 2024 21:56:55 +0000 Original-Received: from localhost ([127.0.0.1]:38473 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rpbGQ-0000E5-TB for submit@debbugs.gnu.org; Wed, 27 Mar 2024 17:56:55 -0400 Original-Received: from out-186.mta0.migadu.com ([2001:41d0:1004:224b::ba]:30095) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rpbGM-0000Dq-EV for 70036@debbugs.gnu.org; Wed, 27 Mar 2024 17:56:53 -0400 X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=thornhill.no; s=key1; t=1711576603; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=DmY8VxKtKGZycLFpBXkz2Lyt0CbxZrzKHlNH1rr9XwM=; b=ulHb5tHwxMXBKk8hYQ1XHYLLSAr2+VGz0GOP9lVCqLSuFHkA1eLxYYqNf5k4cKy5y46U+a h6MsbsmurX6fGgdBW6w21hpNtwfHdA7Z/EwxjA16ddvHCotoJ0Nxb/rwrlT3aBs/6i6bVd +Xxqw9WrZ1cx2000NecGli43U7MNO1gUC9B6aWZ8zIEd0A48RV9xnCHaqKrkaySyTANpp2 0T5iCT8/Fttms8wJjwkKVwIhi4d7nH06IniokZ0qWyLkmSDCnVqZckch4QfFQhcD37o8zI 9ZBTwpG27l0/ZqCuNIJhJU+5Q7EoQHGJljJVz2AKub9VfApFV16So5JWrD41pQ== In-Reply-To: <861q7vihnm.fsf@gnu.org> X-Migadu-Flow: FLOW_OUT X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:282161 Archived-At: Eli Zaretskii 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" >> >> 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