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#45379: 28.0.50; Degraded Performance of describe-buffer-bindings Date: Thu, 13 May 2021 13:10:38 +0300 Message-ID: <83o8df0wrl.fsf@gnu.org> References: <02f717c6-dc96-4ba0-9117-2ef079ac556f@www.fastmail.com> 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="10228"; mail-complaints-to="usenet@ciao.gmane.io" Cc: stephen.berman@gmx.net, 45379@debbugs.gnu.org, juri@linkov.net, handa@gnu.org, monnier@iro.umontreal.ca, styang@fastmail.com To: Stefan Kangas Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu May 13 12:30:31 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 1lh8bu-0002TO-Nz for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 13 May 2021 12:30:30 +0200 Original-Received: from localhost ([::1]:38086 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lh8bt-0001yD-Og for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 13 May 2021 06:30:29 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:57092) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lh8J4-0004sE-Oi for bug-gnu-emacs@gnu.org; Thu, 13 May 2021 06:11:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:58076) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lh8J4-0006gq-HH for bug-gnu-emacs@gnu.org; Thu, 13 May 2021 06:11:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1lh8J4-0001NC-BA for bug-gnu-emacs@gnu.org; Thu, 13 May 2021 06:11:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 13 May 2021 10:11:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 45379 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: confirmed patch Original-Received: via spool by 45379-submit@debbugs.gnu.org id=B45379.16209006455255 (code B ref 45379); Thu, 13 May 2021 10:11:02 +0000 Original-Received: (at 45379) by debbugs.gnu.org; 13 May 2021 10:10:45 +0000 Original-Received: from localhost ([127.0.0.1]:41389 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lh8Ij-0001Md-A8 for submit@debbugs.gnu.org; Thu, 13 May 2021 06:10:45 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:46674) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lh8If-0001MM-GB for 45379@debbugs.gnu.org; Thu, 13 May 2021 06:10:40 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:36426) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lh8IY-0006Rn-AX; Thu, 13 May 2021 06:10:30 -0400 Original-Received: from 84.94.185.95.cable.012.net.il ([84.94.185.95]:2783 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lh8IX-0005yd-SV; Thu, 13 May 2021 06:10:30 -0400 In-Reply-To: (message from Stefan Kangas on Tue, 4 May 2021 18:31:10 -0500) 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:206415 Archived-At: > From: Stefan Kangas > Date: Tue, 4 May 2021 18:31:10 -0500 > Cc: Juri Linkov , martin rudalics , Eli Zaretskii , > 45379@debbugs.gnu.org, Stefan Monnier , > Stephen Berman , Kenichi Handa > > I finally had time/energy to look into this again! Sorry for taking > more time than expected. Thanks. And I have finally found enough free time to review this. A couple of comments below, and then I'm okay with installing these changes. > > But, I don't know whether the following part in the patch is correct or > > not. > > > > + /* Ignore `self-insert-command' for performance. */ > > + && !EQ (definition, Qself_insert_command)) > > (This is explained below.) And I have a comment for that explanation. > > Lisp_Object val, tem2; > > > > maybe_quit (); > > > > - if (i == stop) > > - { > > - if (i == to) > > - break; > > This is a bit complicated to follow, so I have cleaned it up. I don't see the modified code regarding this to/stop issue as more clear than the original one. In both cases there's a special test which then sets stop = to. I needed to read the new code several times to convince myself we perform the same amount of run-time tests inside the loop. So I'd prefer to leave this nit alone, as it was in the original code. If you find that somewhat unclear, how about adding a comment there explaining whatever it was unclear to you when you first read that? > > @@ -3047,10 +3035,12 @@ describe_vector (Lisp_Object vector, Lisp_Object prefix, Lisp_Object args, > > > > /* Make sure found consecutive keys are either not shadowed or, > > if they are, that they are shadowed by the same command. */ > > - if (CHAR_TABLE_P (vector) && i != starting_i) > > + if (CHAR_TABLE_P (vector) && i != starting_i > > + /* Ignore `self-insert-command' for performance. */ > > + && !EQ (definition, Qself_insert_command)) > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > To see if the shadowing is the same for an entire range, we need to run > shadow_lookup() for *once for each character* in that range to see if > they are shadowed. This is expensive. > > One observation is that we often have *very long* ranges of characters > where the value is "self-insert-command", as in: > > (lookup-key global-map "文") > > This is because a char-table will cover the range of all valid character > codes. [Note again that we use a char-table only if the keymap is > defined with `make-keymap' (as opposed to `make-sparse-keymap', which is > just a list)] > > Let's just assume that it is unlikely that there is any shadowing going > on for all of these self-inserting keys. If there is shadowing going > on, we are probably not looking at a keymap where we have the default > value is set to self-insert-command. > > So we basically say here: let's just not care about > `self-insert-command' and skip the check. Yes, we will in theory not > get a perfect result, as there will be some cases where we miss the > shadowing. OTOH, we are sure to have something that is not very slow. > (And in any case, I don't know of any examples where this will fail, and > if they exist we will in any case already be doing better than Emacs 27, > as this entire check is new in Emacs 28.) To tell the truth, I'm a bit worried by this "assumption", and so was Handa-san. This part of the change looks to me like simply ignoring a legitimate situation which we previously supported, and now will not, for the sole reason that the test is slow. Who can tell us what this could cause in some code somewhere in the community? "Don't know any examples where it will fail" is not very assuring, IMO. Is this part of the change what speeds up describe-buffer-bindings? Or is this just part of the speedup? In the latter case, how much faster will describe-buffer-bindings become without this "optimization"? And in the former case, I'd prefer to have this "optimization" controllable by some variable, which we could then use in the future as a "fire escape" if someone comes up with a use case where the code you want to remove is indeed needed. Alternatively, how about making the "Don't show key ranges if shadowed by different commands" feature, which triggered this regression, optional, by default off? Then people who want it could be warned that it might slow down describe-buffer-bindings, and will have to decide whether they care enough about the speed to have the feature turned on. In any case, at least some of this explanation should be in comments to the code, no matter whether we leave it alone or bypass it conditionally. If we introduce a variable to control this, some of this should be in the doc string of that variable. Thanks again for working on this, and sorry it took me so long to get to review it.