From: Eli Zaretskii <eliz@gnu.org>
To: "Clément Pit-Claudel" <cpitclaudel@gmail.com>
Cc: 41200@debbugs.gnu.org
Subject: bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined
Date: Fri, 15 May 2020 14:05:24 +0300 [thread overview]
Message-ID: <837dxd31cb.fsf@gnu.org> (raw)
In-Reply-To: <cd9163e0-ddf8-9443-8507-502dee728911@gmail.com> (message from Clément Pit-Claudel on Tue, 12 May 2020 22:41:24 -0400)
> Cc: 41200@debbugs.gnu.org
> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Tue, 12 May 2020 22:41:24 -0400
>
> > So I think if we want to support such large amounts of faces, we
> > should not store them in alists, but in a more efficient data
> > structure.
>
> Indeed, you're completely right; thanks! Replacing face_alist and Vface_new_frame_defaults with hash tables makes the worst example about 10 times faster, and with that change tooltips now take 30 to 50ms to display instead of 500-600ms in my real-life use case (my usual config). I have attached a patch.
>
> I left a few questions in the code; I hope that's OK. I have a few more questions that are not part of the code:
>
> * I removed the function frame-face-alist and changed the type of the variable face-new-frame-defaults. Both were documented as internal. Should I add an ELisp implementation of frame-face-alist for compatibility? (It wouldn't be a perfect shim, since modifying its return value wouldn't do the same). For face-new-frame-defaults it's a bit trickier, since the variable now holds a hash table. Should I change its name to make the change obvious, at least?
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.
> * 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?
> * I imagine that this change needs to be advertised somewhere, but I'm not sure where; NEWS?
Let's think about this after we figured out what changes are needed in
the current functions and variables.
> Lastly, do the following new profiles suggest other opportunities for improvement?
I don't think so, but if the behavior is now linear or sub-linear,
it's the best we can expect, since creating a new frame must walk over
all the faces.
> + // 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?
> + fset_face_hash
> + (f, make_hash_table(hashtest_eq, DEFAULT_HASH_SIZE, DEFAULT_REHASH_SIZE,
^^
Our C coding conventions are to leave one space between the function's
name and the opening parenthesis (here and elsewhere in the patch).
> -DEFUN ("frame-face-alist", Fframe_face_alist, Sframe_face_alist,
> +// QUESTION: Should I add an ELisp version of frame-face-hash?
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.
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.
> + 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.
> + 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.
> 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?
next prev parent reply other threads:[~2020-05-15 11:05 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-12 4:30 bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined Clément Pit-Claudel
2020-05-12 6:42 ` martin rudalics
2020-05-12 11:30 ` Clément Pit-Claudel
2020-05-12 15:12 ` martin rudalics
2020-05-12 17:19 ` Clément Pit-Claudel
2020-05-12 17:42 ` martin rudalics
2020-05-12 17:58 ` Eli Zaretskii
2020-05-13 14:58 ` martin rudalics
2020-05-12 15:27 ` Eli Zaretskii
2020-05-13 2:41 ` Clément Pit-Claudel
2020-05-13 14:58 ` martin rudalics
2020-05-13 15:13 ` Clément Pit-Claudel
2020-05-13 17:42 ` martin rudalics
2020-05-15 11:05 ` Eli Zaretskii [this message]
2020-05-15 14:59 ` Clément Pit-Claudel
2020-05-15 15:17 ` Eli Zaretskii
2020-05-15 15:33 ` Noam Postavsky
2020-05-15 16:22 ` Clément Pit-Claudel
2020-05-15 17:28 ` Eli Zaretskii
2020-05-15 18:50 ` Clément Pit-Claudel
2020-05-15 19:05 ` Eli Zaretskii
2020-05-15 19:23 ` Clément Pit-Claudel
2020-05-15 19:38 ` Eli Zaretskii
2020-05-15 19:52 ` Clément Pit-Claudel
2020-05-16 23:03 ` Juri Linkov
2020-05-16 23:43 ` Clément Pit-Claudel
2020-05-17 21:59 ` Juri Linkov
2020-05-18 1:19 ` Clément Pit-Claudel
2020-05-19 21:48 ` Juri Linkov
[not found] ` <83a71z135p.fsf@gnu.org>
2020-05-23 22:47 ` Juri Linkov
2020-05-24 2:33 ` Eli Zaretskii
2020-05-24 21:50 ` Juri Linkov
2020-06-08 0:21 ` Juri Linkov
2020-06-20 7:47 ` Eli Zaretskii
2020-06-20 16:55 ` Clément Pit-Claudel
2020-07-04 7:58 ` Eli Zaretskii
2020-09-13 2:53 ` Benson Chu
2020-05-15 14:03 ` Stefan Monnier
2020-05-15 14:34 ` Eli Zaretskii
2020-05-15 19:10 ` Clément Pit-Claudel
2020-05-15 21:23 ` Stefan Monnier
2020-05-16 8:45 ` martin rudalics
2021-04-06 6:35 ` Jashank Jeremy
2021-04-06 12:30 ` Eli Zaretskii
2021-04-06 15:07 ` Clément Pit-Claudel
2021-04-06 15:50 ` Eli Zaretskii
2021-04-23 3:56 ` Stefan Monnier
2021-05-12 20:29 ` Lars Ingebrigtsen
2021-05-13 3:56 ` Jashank Jeremy
2021-05-13 9:15 ` Lars Ingebrigtsen
2021-05-13 23:26 ` Jashank Jeremy
2021-06-12 12:15 ` Lars Ingebrigtsen
2021-06-13 3:19 ` Richard Stallman
2021-07-06 12:41 ` Aaron Jensen
2021-07-21 14:02 ` Lars Ingebrigtsen
2021-07-21 14:28 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-07-21 14:32 ` Clément Pit-Claudel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=837dxd31cb.fsf@gnu.org \
--to=eliz@gnu.org \
--cc=41200@debbugs.gnu.org \
--cc=cpitclaudel@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.