From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.ciao.gmane.io!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined Date: Fri, 15 May 2020 18:17:53 +0300 Message-ID: <835zcxgrby.fsf@gnu.org> References: <8fd8896a-cd5c-66f4-4792-f65cac4dc4f5@gmail.com> <83lflx896q.fsf@gnu.org> <837dxd31cb.fsf@gnu.org> <789d786d-a07a-65c1-c0e4-433e4c18d64e@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="ciao.gmane.io:159.69.161.202"; logging-data="74395"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 41200@debbugs.gnu.org To: =?UTF-8?Q?Cl=C3=A9ment?= Pit-Claudel Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri May 15 17:22:12 2020 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 1jZcA7-000J9p-Q8 for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 15 May 2020 17:22:11 +0200 Original-Received: from localhost ([::1]:40842 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jZcA6-00065A-PM for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 15 May 2020 11:22:10 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:50978) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jZc74-0001rf-TJ for bug-gnu-emacs@gnu.org; Fri, 15 May 2020 11:19:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:55175) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jZc74-0006UX-GV for bug-gnu-emacs@gnu.org; Fri, 15 May 2020 11:19:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1jZc74-0003pW-BD for bug-gnu-emacs@gnu.org; Fri, 15 May 2020 11:19: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: Fri, 15 May 2020 15:19:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 41200 X-GNU-PR-Package: emacs Original-Received: via spool by 41200-submit@debbugs.gnu.org id=B41200.158955589714639 (code B ref 41200); Fri, 15 May 2020 15:19:02 +0000 Original-Received: (at 41200) by debbugs.gnu.org; 15 May 2020 15:18:17 +0000 Original-Received: from localhost ([127.0.0.1]:38488 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jZc6L-0003o1-5g for submit@debbugs.gnu.org; Fri, 15 May 2020 11:18:17 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:43348) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jZc6I-0003nl-UB for 41200@debbugs.gnu.org; Fri, 15 May 2020 11:18:15 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:43269) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jZc6C-0006Kp-NJ; Fri, 15 May 2020 11:18:08 -0400 Original-Received: from [176.228.60.248] (port=4485 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1jZc6B-0007nx-3R; Fri, 15 May 2020 11:18:07 -0400 In-Reply-To: <789d786d-a07a-65c1-c0e4-433e4c18d64e@gmail.com> (message from =?UTF-8?Q?Cl=C3=A9ment?= Pit-Claudel on Fri, 15 May 2020 10:59:36 -0400) 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:180338 Archived-At: > Cc: 41200@debbugs.gnu.org > From: Clément Pit-Claudel > Date: Fri, 15 May 2020 10:59:36 -0400 > > > I'd like to keep the old face-new-frame-defaults and frame-face-alist > > for compatibility, but mention in the doc strings that they no longer > > return modifiable values, and perhaps deprecate them. > > Done for frame-face-alist. But how can one do a read-only variable? Using the new variable watchers facility? At the very least mention in the doc string. I don't think using watchable symbols is needed here, it sounds gross. If there's an outcry that this breaks too much code out there, we could revisit this. > >> * The name face_hash isn't ideal, since there's already a distinct notion of face hashes (hash codes). Can you think of a better name? > > > > face_hash_table? > > Thanks, good idea. I also liked Stefan's frame_face_map. "Map" is too general, IMO, it doesn't tell enough about the kind of object it is. > >> + // QUESTION: is this where this should be initialized? > > > > Yes, I think so. But do we need to do anything when frame is deleted > > as well? > > I'm not sure (I don't know how garbage collection works on the C side in Elisp; I assumed the map would be collected). I guess so. > > You mean, frame-face-alist, right? Yes, most definitely: I imagine a > > lot of code out there uses that, and we wouldn't want to break that. > > Done. > > I looked around, but I didn't find many uses at all (for example, there are none in ELPA). I think this is likely because the function is documented as "For internal use only." > > There are no uses of face-new-frame-defaults in ELPA either; online, I found many copies of lisp-mode and emacs-lisp-mode, which refer it, and a few functions derived from edebug-eval-defun, which references it. That means we will probably be able to remove it earlier than I feared. > > And I'm not sure we should have it only in Lisp: perhaps we should > > maintain the alist as well, and add/remove to/from it when a face is > > added or removed in the hash table. Otherwise this change of > > internals will have painful effect on packages that use the current > > APIs. > > frame-face-alist is likely less crucial than face-new-frame-defaults, because it was already a function, so the return value has to be mutated in place to modify it (it couldn't be directly assigned). For both of these, however, how would we ensure that the alist remains in sync with the hashmap (that is, how do we catch modifications?) I thought about updating the alist when the hash-table is modified. Since you always know whether the face is already in the hash-table, you don't need to scan the alist looking for it. I'd really like to know that no one is using these before we make the final decision about this. > >> + Lisp_Object lface = HASH_KEY(table, idx); > >> + Lisp_Object face_id = Fget (lface, Qface); > >> + // FIXME why is (get 'tab-line 'face) 0? > > > > A bug, I guess. > > As far as I can see, these IDs are assigned by Finternal_make_lisp_face, and I *think* it is never called for tab-line? Most probably. > Should I make sure that it is? Yes, I think so. > If so, where from? I think the problem is that tab-line is declared a basic face, but its defface form is not in faces.el. > >> + if (!FIXNATP (face_id)) > >> + // FIXME: I'm not sure what to do in this case > > > > I'm not sure I understand why do you need to look at the existing > > face's 'face' property? The original code didn't. > > The original code iterated over face-frame-alist in the order in which entries were added to it. If I understand correctly, iteration order isn't guaranteed on hash tables (is it?), so I had to find a different source of truth for these. Maybe we should store the ID with the face? I think we wanted to get rid of the 'face' property of the faces at some point. > >> DEFVAR_LISP ("face-new-frame-defaults", Vface_new_frame_defaults, > >> doc: /* List of global face definitions (for internal use only.) */); > >> - Vface_new_frame_defaults = Qnil; > >> + Vface_new_frame_defaults = > >> + make_hash_table (hashtest_eq, DEFAULT_HASH_SIZE, DEFAULT_REHASH_SIZE, > >> + DEFAULT_REHASH_THRESHOLD, Qnil, Qnil); > > > > Why do we need to start with a non-default hash-table? > > I wanted to use `eq' instead of `eql' as the test (is that what you were asking?) No, I was asking why not start with it as nil and actually make a hash-table when we first need it.