From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Daniel Mendler Newsgroups: gmane.emacs.bugs Subject: bug#47711: [PATCH VERSION 2] Add new `completion-filter-completions` API and deferred highlighting Date: Fri, 13 Aug 2021 14:22:48 +0200 Message-ID: <38a06f3c-4a7a-755c-c99b-708f91afabfa@daniel-mendler.de> References: <3d3f894f-a6fa-53ae-5d50-c3aa9bffc73e@daniel-mendler.de> <56ab18b1-4348-9b2c-85bb-af9b76cd429a@daniel-mendler.de> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="29104"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 47711@debbugs.gnu.org, Stefan Monnier , 48841@debbugs.gnu.org, Dmitry Gutov To: =?UTF-8?Q?Jo=C3=A3o_?= =?UTF-8?Q?T=C3=A1vora?= Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Aug 13 14:24:13 2021 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 1mEWEO-0007IM-VC for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 13 Aug 2021 14:24:13 +0200 Original-Received: from localhost ([::1]:42568 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mEWEN-0007wi-F1 for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 13 Aug 2021 08:24:11 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:36356) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mEWEE-0007vo-7W for bug-gnu-emacs@gnu.org; Fri, 13 Aug 2021 08:24:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:57392) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mEWEE-00067C-11 for bug-gnu-emacs@gnu.org; Fri, 13 Aug 2021 08:24:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mEWED-00086J-LZ for bug-gnu-emacs@gnu.org; Fri, 13 Aug 2021 08:24:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Daniel Mendler Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 13 Aug 2021 12:24:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 47711 X-GNU-PR-Package: emacs Original-Received: via spool by 47711-submit@debbugs.gnu.org id=B47711.162885739031076 (code B ref 47711); Fri, 13 Aug 2021 12:24:01 +0000 Original-Received: (at 47711) by debbugs.gnu.org; 13 Aug 2021 12:23:10 +0000 Original-Received: from localhost ([127.0.0.1]:40705 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mEWDK-000850-Qv for submit@debbugs.gnu.org; Fri, 13 Aug 2021 08:23:10 -0400 Original-Received: from server.qxqx.de ([178.63.65.180]:52851 helo=mail.qxqx.de) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mEWDB-000847-Ko; Fri, 13 Aug 2021 08:23:01 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=qxqx.de; s=mail1392553390; h=Content-Transfer-Encoding:Content-Type:In-Reply-To: MIME-Version:Date:Message-ID:From:References:Cc:To:Subject:Sender:Reply-To: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=No8L/5f/jqE7VfR4Zr8RQeJNpAlnovp34dfprC2OpIA=; b=NVDxqieGZDnqxkTN6bD1l5Lybk XKH0kqn8ciZRmZw+/MId68EnF9wrXz5XBq9PW3+NEGcLSSbkH7oZ/LvNHUVkYBB3G2MrmqMEWev05 59CczGWRQCHXazW65/zaniWkgZYzTYafVKgjWRWtqAl9idZuD1K3RAOU5YOdF7dQHWx0=; In-Reply-To: Content-Language: en-US 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" Xref: news.gmane.io gmane.emacs.bugs:211743 Archived-At: On 8/13/21 2:05 PM, João Távora wrote: >> The existing API `completion-all-completions` >> necessarily has to allocate all the strings in order to attach >> highlighting and scoring. The new API solves this in a clean way by >> both deferring highlighting and scoring. > > I'm not sure you understand my alternative idea. As far as I > understand (and have actually measured) the lines: > > ;; Don't modify the string itself. > (setq str (copy-sequence str)) > > in minibuffer.el, in the function completion-pcm--hilit-commonality > > Are the cause of the problem that _I am talking about_ and that > I have actually measured. Again you may be referring to a > _different_ problem that I am unaware of. You are right that the call to `copy-sequence` is a major bottleneck in the filtering. However you are wrong that this line can simply be removed/disabled and the candidates can be modified. The API guarantees and has always guaranteed that the candidate strings are not mutated. It is important to keep this property since this will preclude many bugs due to string mutation. By separating the filtering and mutation (highlighting, scoring) my patch addresses the problem at hand in the proper way. Note that the UI also has no possibility to opt-out of the mutation. The UI is actually not the one being concerned about the mutation here, it is the backends (completion tables), which produce the strings. If one starts mutating these strings you will see bugs cropping up throughout Emacs where shared strings suddenly have spurious additional properties due to the completion filtering. Mutation would be a reasonable choice here if the problem could not be solved in a proper way. But in fact it can be solved in a proper way without mutating the strings at all as my patch shows. > If one removes these lines, the process becomes much faster, but there is a > problem with highlighting. My idea is indeed to defer highlighting by not > setting the 'face property directly on that shared string, but some > other property > that is read later from the shared string by compliant frontents. This solution is much more ad-hoc and you still mutate the string which is not allowed. > The advantage that I see is that those adaptations apart, it is a small > localized and effective change. Note that your idea also does not address the other issues which are addressed by my patch. The new API `completion-filter-completions` returns data which hasn't been available before, e.g., the end position, which cannot be fixed given the existing API. >> The main problem is that `completion-all-completions` allocates all the >> strings every time the completions are filtered. This is the same >> performance issue you encountered in fido-mode/icomplete-mode. > > OK. I encountered at least two different performance problems there, with > quite different causes. So let's stick to the string-allocation problem. Post > a code snippet that demonstrates the problem the way you see it/experience it? You can try my Vertico completion UI, which is available on GNU ELPA. It implements deferred highlighting and there the performance difference is perceivable. Currently Vertico uses an advice-based hack to avoid the over-eager string-allocations and the highlighting. >> The second problem addressed by the new API >> `completion-filter-completions` is that `completion-all-completions` is >> limited in what it can return. For example it cannot return the end >> position of the completion. > > And why is this a problem? Can you post an example of something you'd > like to do, but can't? Regardless, it does seem indeed like a "second" problem > (as you state) so perhaps something that can be addressed separately. Please look at the FIXMEs in minibuffer.el which address this. Currently only the beginning position of the completion boundary is returned, which is only half of the information. > I understand you've put time and effort into producing this work. We are > all indebted and I promise to read it. But every API writer in history of > programming has claimed those things and reality often shows otherwise. > So it's not that your work can't be those things you claim, maybe it is, but > generally the larger and broader the work the harder it is to reason about. I stand by my claim and I also stand by the claim that removing/disabling `copy-sequence` is not a proper way to address the issues at hand and will introduce many bugs in the long run. Please take your time to look at the patch in earnest. I would also like to see others chime in here with their opinion. Daniel