unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: "Clément Pit-Claudel" <cpitclaudel@gmail.com>
To: martin rudalics <rudalics@gmx.at>, Eli Zaretskii <eliz@gnu.org>
Cc: 41200@debbugs.gnu.org
Subject: bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined
Date: Wed, 13 May 2020 11:13:27 -0400	[thread overview]
Message-ID: <956a7cf7-bbf0-e15d-36d4-c82279c15f89@gmail.com> (raw)
In-Reply-To: <a0560d39-ec5c-9a1b-3c82-5005bf4c841a@gmx.at>

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]

On 13/05/2020 10.58, martin rudalics wrote:
>> 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.
> 
> GCC throws the following error here:

Woops, thanks. I misread the signature of make_hash_table.  I've attached an updated patch.



[-- Attachment #2: 0001-Store-frame-faces-in-hash-tables-instead-of-alists.patch --]
[-- Type: text/x-patch, Size: 12558 bytes --]

From 75d44050a9277c708c69afe09de334ab3c300da1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Pit-Claudel?= <clement.pitclaudel@live.com>
Date: Tue, 12 May 2020 21:48:32 -0400
Subject: [PATCH] Store frame faces in hash tables instead of alists

* lisp/emacs-lisp/edebug.el (edebug-eval-defun):
* lisp/progmodes/elisp-mode.el (elisp--eval-defun-1):
* lisp/faces.el (face-list):
* lisp/frame.el (frame-set-background-mode): Update to work with hash
tables instead of alists.

* src/frame.h (struct frame): Remove face_alist, add face_hash.
(fset_face_alist): Remove.
(fset_face_hash): New function.
* src/frame.c (make_frame): Initialize f->face_hash.
(Fmake_terminal_frame): Update to work with hash tables instead of
alists.
* src/xfaces.c (lface_from_face_name_no_resolve):
(Finternal_make_lisp_face):
(update_face_from_frame_parameter): Update to work with hash tables
instead of alists.
* src/xfaces.c (Fframe_face_hash): New function.
(Fframe_face_alist): Remove.
(init_xfaces): Compute face IDs from they face property, not from
their position in face_alist.
(syms_of_xfaces): Remove frame_face_alist, add frame_face_hash;
change Vface_new_frame_defaults into a hash table.
---
 lisp/emacs-lisp/edebug.el    |  3 +-
 lisp/faces.el                |  4 ++-
 lisp/frame.el                |  2 +-
 lisp/progmodes/elisp-mode.el |  3 +-
 src/frame.c                  | 21 ++++++++----
 src/frame.h                  |  8 ++---
 src/xfaces.c                 | 64 ++++++++++++++++++++----------------
 7 files changed, 59 insertions(+), 46 deletions(-)

diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 78461185d3..97ca964eef 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -488,8 +488,7 @@ edebug-eval-defun
 	   (set-default (nth 1 form) (eval (nth 2 form) lexical-binding)))
           ((eq (car form) 'defface)
            ;; Reset the face.
-           (setq face-new-frame-defaults
-                 (assq-delete-all (nth 1 form) face-new-frame-defaults))
+           (remhash (nth 1 form) face-new-frame-defaults)
            (put (nth 1 form) 'face-defface-spec nil)
            (put (nth 1 form) 'face-documentation (nth 3 form))
 	   ;; See comments in `eval-defun-1' for purpose of code below
diff --git a/lisp/faces.el b/lisp/faces.el
index e707f6f4b6..1764f3da75 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -179,7 +179,9 @@ face-font-registry-alternatives
 
 (defun face-list ()
   "Return a list of all defined faces."
-  (mapcar #'car face-new-frame-defaults))
+  (let ((faces nil))
+    (maphash (lambda (face _) (push face faces)) face-new-frame-defaults)
+    (nreverse faces)))
 
 (defun make-face (face)
   "Define a new face with name FACE, a symbol.
diff --git a/lisp/frame.el b/lisp/frame.el
index 6c2f774709..bab3c2a1c2 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -1227,7 +1227,7 @@ frame-set-background-mode
                          ;; during startup with -rv on the command
                          ;; line for the initial frame, because frames
                          ;; are not recorded in the pdump file.
-                         (assq face (frame-face-alist))
+                         (gethash face (frame-face-hash))
                          (face-spec-match-p face
                                             (face-user-default-spec face)
                                             ;; FIXME: why selected-frame and
diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index b737134f90..dc8688c864 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -1304,8 +1304,7 @@ elisp--eval-defun-1
 	((eq (car form) 'custom-declare-face)
 	 ;; Reset the face.
 	 (let ((face-symbol (eval (nth 1 form) lexical-binding)))
-	   (setq face-new-frame-defaults
-		 (assq-delete-all face-symbol face-new-frame-defaults))
+	   (remhash face-new-frame-defaults face-symbol)
 	   (put face-symbol 'face-defface-spec nil)
 	   (put face-symbol 'face-override-spec nil))
 	 form)
diff --git a/src/frame.c b/src/frame.c
index c871e4fd99..7cd43e4fec 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -946,6 +946,11 @@ make_frame (bool mini_p)
   rw->total_lines = mini_p ? 9 : 10;
   rw->pixel_height = rw->total_lines * FRAME_LINE_HEIGHT (f);
 
+  // QUESTION: is this where this should be initialized?
+  fset_face_hash
+    (f, make_hash_table(hashtest_eq, DEFAULT_HASH_SIZE, DEFAULT_REHASH_SIZE,
+                        DEFAULT_REHASH_THRESHOLD, Qnil, false));
+
   if (mini_p)
     {
       mw->top_line = rw->total_lines;
@@ -1254,7 +1259,7 @@ DEFUN ("make-terminal-frame", Fmake_terminal_frame, Smake_terminal_frame,
 {
   struct frame *f;
   struct terminal *t = NULL;
-  Lisp_Object frame, tem;
+  Lisp_Object frame;
   struct frame *sf = SELECTED_FRAME ();
 
 #ifdef MSDOS
@@ -1336,14 +1341,16 @@ DEFUN ("make-terminal-frame", Fmake_terminal_frame, Smake_terminal_frame,
   store_in_alist (&parms, Qminibuffer, Qt);
   Fmodify_frame_parameters (frame, parms);
 
-  /* Make the frame face alist be frame-specific, so that each
+  /* Make the frame face hash be frame-specific, so that each
      frame could change its face definitions independently.  */
-  fset_face_alist (f, Fcopy_alist (sf->face_alist));
-  /* Simple Fcopy_alist isn't enough, because we need the contents of
-     the vectors which are the CDRs of associations in face_alist to
+  fset_face_hash (f, Fcopy_hash_table (sf->face_hash));
+  /* Simple copy_hash_table isn't enough, because we need the contents of
+     the vectors which are the values in face_hash to
      be copied as well.  */
-  for (tem = f->face_alist; CONSP (tem); tem = XCDR (tem))
-    XSETCDR (XCAR (tem), Fcopy_sequence (XCDR (XCAR (tem))));
+  ptrdiff_t idx = 0;
+  struct Lisp_Hash_Table *table = XHASH_TABLE(f->face_hash);
+  for (idx = 0; idx < table->count; ++idx)
+    set_hash_value_slot (table, idx, Fcopy_sequence(HASH_VALUE (table, idx)));
 
   f->can_set_window_size = true;
   f->after_make_frame = true;
diff --git a/src/frame.h b/src/frame.h
index 476bac67fa..0a324414a6 100644
--- a/src/frame.h
+++ b/src/frame.h
@@ -158,8 +158,8 @@ #define EMACS_FRAME_H
      There are four additional elements of nil at the end, to terminate.  */
   Lisp_Object menu_bar_items;
 
-  /* Alist of elements (FACE-NAME . FACE-VECTOR-DATA).  */
-  Lisp_Object face_alist;
+  /* Hash table of FACE-NAME keys and FACE-VECTOR-DATA values.  */
+  Lisp_Object face_hash;
 
   /* A vector that records the entire structure of this frame's menu bar.
      For the format of the data, see extensive comments in xmenu.c.
@@ -661,9 +661,9 @@ fset_condemned_scroll_bars (struct frame *f, Lisp_Object val)
   f->condemned_scroll_bars = val;
 }
 INLINE void
-fset_face_alist (struct frame *f, Lisp_Object val)
+fset_face_hash (struct frame *f, Lisp_Object val)
 {
-  f->face_alist = val;
+  f->face_hash = val;
 }
 #if defined (HAVE_WINDOW_SYSTEM)
 INLINE void
diff --git a/src/xfaces.c b/src/xfaces.c
index 7d7aff95c1..4f97ba4971 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -1843,13 +1843,11 @@ lface_from_face_name_no_resolve (struct frame *f, Lisp_Object face_name,
   Lisp_Object lface;
 
   if (f)
-    lface = assq_no_quit (face_name, f->face_alist);
+    lface = Fgethash (face_name, f->face_hash, Qnil);
   else
-    lface = assq_no_quit (face_name, Vface_new_frame_defaults);
+    lface = Fgethash (face_name, Vface_new_frame_defaults, Qnil);
 
-  if (CONSP (lface))
-    lface = XCDR (lface);
-  else if (signal_p)
+  if (signal_p && NILP (lface))
     signal_error ("Invalid face", face_name);
 
   check_lface (lface);
@@ -2736,8 +2734,7 @@ DEFUN ("internal-make-lisp-face", Finternal_make_lisp_face,
     {
       global_lface = make_vector (LFACE_VECTOR_SIZE, Qunspecified);
       ASET (global_lface, 0, Qface);
-      Vface_new_frame_defaults = Fcons (Fcons (face, global_lface),
-					Vface_new_frame_defaults);
+      Fputhash(face, global_lface, Vface_new_frame_defaults);
 
       /* Assign the new Lisp face a unique ID.  The mapping from Lisp
 	 face id to Lisp face is given by the vector lface_id_to_name.
@@ -2763,7 +2760,7 @@ DEFUN ("internal-make-lisp-face", Finternal_make_lisp_face,
 	{
 	  lface = make_vector (LFACE_VECTOR_SIZE, Qunspecified);
 	  ASET (lface, 0, Qface);
-	  fset_face_alist (f, Fcons (Fcons (face, lface), f->face_alist));
+	  Fputhash (face, lface, f->face_hash);
 	}
       else
 	for (i = 1; i < LFACE_VECTOR_SIZE; ++i)
@@ -3508,7 +3505,7 @@ update_face_from_frame_parameter (struct frame *f, Lisp_Object param,
   /* If there are no faces yet, give up.  This is the case when called
      from Fx_create_frame, and we do the necessary things later in
      face-set-after-frame-defaults.  */
-  if (NILP (f->face_alist))
+  if (XFIXNAT(Fhash_table_count(f->face_hash)) == 0)
     return;
 
   if (EQ (param, Qforeground_color))
@@ -4174,14 +4171,14 @@ DEFUN ("internal-lisp-face-empty-p", Finternal_lisp_face_empty_p,
   return i == LFACE_VECTOR_SIZE ? Qt : Qnil;
 }
 
-
-DEFUN ("frame-face-alist", Fframe_face_alist, Sframe_face_alist,
+// QUESTION: Should I add an ELisp version of frame-face-hash?
+DEFUN ("frame-face-hash", Fframe_face_hash, Sframe_face_hash,
        0, 1, 0,
        doc: /* Return an alist of frame-local faces defined on FRAME.
 For internal use only.  */)
   (Lisp_Object frame)
 {
-  return decode_live_frame (frame)->face_alist;
+  return decode_live_frame (frame)->face_hash;
 }
 
 
@@ -6678,30 +6675,37 @@ DEFUN ("show-face-resources", Fshow_face_resources, Sshow_face_resources,
 
 #ifdef HAVE_PDUMPER
 /* All the faces defined during loadup are recorded in
-   face-new-frame-defaults, with the last face first in the list.  We
-   need to set next_lface_id to the next face ID number, so that any
-   new faces defined in this session will have face IDs different from
-   those defined during loadup.  We also need to set up the
-   lface_id_to_name[] array for the faces that were defined during
-   loadup.  */
+   face-new-frame-defaults.  We need to set next_lface_id to the next
+   face ID number, so that any new faces defined in this session will
+   have face IDs different from those defined during loadup.  We also
+   need to set up the lface_id_to_name[] array for the faces that were
+   defined during loadup.  */
 void
 init_xfaces (void)
 {
-  if (CONSP (Vface_new_frame_defaults))
+  int nfaces = XFIXNAT(Fhash_table_count (Vface_new_frame_defaults));
+  if (nfaces > 0)
     {
       /* Allocate the lface_id_to_name[] array.  */
-      lface_id_to_name_size = next_lface_id =
-	XFIXNAT (Flength (Vface_new_frame_defaults));
+      lface_id_to_name_size = next_lface_id = nfaces;
       lface_id_to_name = xnmalloc (next_lface_id, sizeof *lface_id_to_name);
 
       /* Store the faces.  */
-      Lisp_Object tail;
-      int i = next_lface_id - 1;
-      for (tail = Vface_new_frame_defaults; CONSP (tail); tail = XCDR (tail))
+      struct Lisp_Hash_Table* table = XHASH_TABLE(Vface_new_frame_defaults);
+      for (ptrdiff_t idx = 0; idx < nfaces; ++idx)
 	{
-	  Lisp_Object lface = XCAR (tail);
-	  eassert (i >= 0);
-	  lface_id_to_name[i--] = XCAR (lface);
+	  Lisp_Object lface = HASH_KEY(table, idx);
+          Lisp_Object face_id = Fget (lface, Qface);
+          // FIXME why is (get 'tab-line 'face) 0?
+          if (!FIXNATP (face_id))
+            // FIXME: I'm not sure what to do in this case
+            printf("Face %s has no id\n",  SDATA(SYMBOL_NAME (lface)));
+          else
+            {
+              int id = XFIXNAT(face_id);
+              eassert (id >= 0);
+              lface_id_to_name[id] = lface;
+            }
 	}
     }
   face_attr_sym[0] = Qface;
@@ -6855,7 +6859,7 @@ syms_of_xfaces (void)
   defsubr (&Sinternal_copy_lisp_face);
   defsubr (&Sinternal_merge_in_global_face);
   defsubr (&Sface_font);
-  defsubr (&Sframe_face_alist);
+  defsubr (&Sframe_face_hash);
   defsubr (&Sdisplay_supports_face_attributes_p);
   defsubr (&Scolor_distance);
   defsubr (&Sinternal_set_font_selection_order);
@@ -6881,7 +6885,9 @@ syms_of_xfaces (void)
 
   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, false);
 
   DEFVAR_LISP ("face-default-stipple", Vface_default_stipple,
     doc: /* Default stipple pattern used on monochrome displays.
-- 
2.17.1


  reply	other threads:[~2020-05-13 15:13 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 [this message]
2020-05-13 17:42         ` martin rudalics
2020-05-15 11:05     ` Eli Zaretskii
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

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=956a7cf7-bbf0-e15d-36d4-c82279c15f89@gmail.com \
    --to=cpitclaudel@gmail.com \
    --cc=41200@debbugs.gnu.org \
    --cc=eliz@gnu.org \
    --cc=rudalics@gmx.at \
    /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 public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).