From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#70036: 30.0.50; Move file-truename to the C level Date: Wed, 27 Mar 2024 21:44:29 +0200 Message-ID: <861q7vihnm.fsf@gnu.org> References: <87le63xzjt.fsf@thornhill.no> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="34198"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 70036@debbugs.gnu.org To: Theodor Thornhill Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Wed Mar 27 20:46:28 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 1rpZE9-0008iP-IH for geb-bug-gnu-emacs@m.gmane-mx.org; Wed, 27 Mar 2024 20:46:25 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rpZDn-0006uQ-2P; Wed, 27 Mar 2024 15:46:03 -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 1rpZDl-0006u8-TJ for bug-gnu-emacs@gnu.org; Wed, 27 Mar 2024 15:46:02 -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 1rpZDl-0008B3-LP for bug-gnu-emacs@gnu.org; Wed, 27 Mar 2024 15:46:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rpZDl-0006Sm-JV for bug-gnu-emacs@gnu.org; Wed, 27 Mar 2024 15:46:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 27 Mar 2024 19:46: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.171156870224538 (code B ref 70036); Wed, 27 Mar 2024 19:46:01 +0000 Original-Received: (at 70036) by debbugs.gnu.org; 27 Mar 2024 19:45:02 +0000 Original-Received: from localhost ([127.0.0.1]:38406 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rpZCm-0006NS-Is for submit@debbugs.gnu.org; Wed, 27 Mar 2024 15:45:01 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:55284) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rpZCj-0006Mf-BL for 70036@debbugs.gnu.org; Wed, 27 Mar 2024 15:44:59 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rpZCd-0007ju-DL; Wed, 27 Mar 2024 15:44:51 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=qfTSGXNSrLrC8PkVZltd/1o/6TQCz+gybDRqAXiTDZU=; b=D4G5275VWdKw v08TmAUXEYn8JRaIZrOkDG4o/RjVqo+XrmFGesLwcDPTIBZic3GmTN2/ek+U4C3VkEQhjmtujklQP UCYoSUsdxM9X4LafDTLM4vQatZ6KAwTDBD0wOLxbTkfHSvvxwkU+VAQVDYQR0W/gcMcRmsL2p2i3I LNrPrJDFCptd0Sy3le8v5m5hp2ooEvCpH7bIkq8M1+XloU7nObV5zMWakYOR1tRWrGJiwCzjynhjR 0iFGr1qO11p71+XVC82M2d+vdYRcQyYPssu+sJrpdPV8a7Fmgvy2BucptP4OS6Qs46hgkDID0mKue ipUWIL/d4xYUjQ6wtLW70Q==; In-Reply-To: <87le63xzjt.fsf@thornhill.no> (bug-gnu-emacs@gnu.org) 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:282153 Archived-At: > 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. > - Does it have to be recursive? No, it doesn't. > Firstly, I'll show some benchmarks > > ``` > ;; Emacs 29 branch > > (benchmark-run 10000 > (file-truename "~/Work/some/long/path/to/parse/that/is/very/deep/deep/deep/super/duper/deep/deep.el")) > ;; (1.810892642 1 0.051070616) > > > ;; With new C implementation > > (benchmark-run 10000 > (file-truename "~/Work/some/long/path/to/parse/that/is/very/deep/deep/deep/super/duper/deep/deep.el")) > ;; (0.018811035 0 0.0) > ``` > > 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. > 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. > + 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.