From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.ciao.gmane.io!not-for-mail From: =?UTF-8?Q?Cl=C3=A9ment?= Pit-Claudel Newsgroups: gmane.emacs.bugs 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 Message-ID: <956a7cf7-bbf0-e15d-36d4-c82279c15f89@gmail.com> References: <8fd8896a-cd5c-66f4-4792-f65cac4dc4f5@gmail.com> <83lflx896q.fsf@gnu.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------8E749A14CB345923344EA199" Injection-Info: ciao.gmane.io; posting-host="ciao.gmane.io:159.69.161.202"; logging-data="53297"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 Cc: 41200@debbugs.gnu.org To: martin rudalics , Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Wed May 13 17:14:27 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 1jYt5V-000DeS-It for geb-bug-gnu-emacs@m.gmane-mx.org; Wed, 13 May 2020 17:14:25 +0200 Original-Received: from localhost ([::1]:57536 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jYt5U-00086e-K7 for geb-bug-gnu-emacs@m.gmane-mx.org; Wed, 13 May 2020 11:14:24 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:34426) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jYt5B-00082x-2J for bug-gnu-emacs@gnu.org; Wed, 13 May 2020 11:14:05 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:47557) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jYt58-0005tj-3k for bug-gnu-emacs@gnu.org; Wed, 13 May 2020 11:14:04 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1jYt57-0005VM-RQ for bug-gnu-emacs@gnu.org; Wed, 13 May 2020 11:14:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: =?UTF-8?Q?Cl=C3=A9ment?= Pit-Claudel Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 13 May 2020 15:14:01 +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.158938281921122 (code B ref 41200); Wed, 13 May 2020 15:14:01 +0000 Original-Received: (at 41200) by debbugs.gnu.org; 13 May 2020 15:13:39 +0000 Original-Received: from localhost ([127.0.0.1]:59103 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jYt4k-0005Ub-E1 for submit@debbugs.gnu.org; Wed, 13 May 2020 11:13:39 -0400 Original-Received: from mail-qk1-f178.google.com ([209.85.222.178]:35382) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jYt4i-0005UP-Lt for 41200@debbugs.gnu.org; Wed, 13 May 2020 11:13:37 -0400 Original-Received: by mail-qk1-f178.google.com with SMTP id f13so17088885qkh.2 for <41200@debbugs.gnu.org>; Wed, 13 May 2020 08:13:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language; bh=yH5NH6Cfnb91IZSxOHzfp/0FujUazO3CIIyay8K0JlM=; b=Ao4wJyYVYv7OFrextPlfPFUoKLwu2cJIGxp6304+nNBH/93J4/iB4WkGK4lcnYk7m6 Az14CSc70QkMr/vQMtNb2GkmgUC+Ja1HGJMK5pE5mD+GcKuWo3Q+zg+5WTjeUik0SsEe 5lEPxxQ4fVkiBOqj0XvlgGehPeSlwLbAN6LHrzTBur2a/RLIU89wE/AiIuYomh5+txHX xF4e54VG22IuC+naRem0BxLnIKTK+TCPRxyjt1vAOFjZz6XCNUQ15X3kuRx6SD7W/KbC bjddY+2pyd091EfyMYAT9rQjM7FZAMQ2xrwbZXRljCMM9Sc2WdWNc+VPlw73epZAu3bp BPkA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=yH5NH6Cfnb91IZSxOHzfp/0FujUazO3CIIyay8K0JlM=; b=Vk9vrA7mBJZ6JqyZnVQO79LjO9QRE9HpJ6rFuWoTgDiuElssyheyGBUQ8tKH3h3jB0 yuSOjJr8/9eZstfXKQ6nkwO0eckVQzRNp5eaMD4v9fy+4zbVzRt6mjY6x3tPsoLmfcr9 hG+W5QJCl8C0fN2euGifr0IKUoYfye6lfu6Aeso8AvvC/zyGYLOhTYLEpy5GKuXOX8my IViHmTPYqgJ6ePWFG0AgM47oY6M1o451ZXPaqSCPbQfqygFF4OSGWdXTuol7rGMd6QhI TG9aNuZFdBE4secRxo1qkdOpUmxMp0CUlM7FE84wcAIK1vXQfUBDiFlFK0P6c+bM14Hi oa1g== X-Gm-Message-State: AOAM532FQyXOjkUvvUO6uJxO4yxLBmYFbzhyZGnATEqx8iR3DI12adtY Ye3Sh6zZsYO291+jzJRsxUXLH92QiUc= X-Google-Smtp-Source: ABdhPJz7tIySTfFw9v4J6at1MtKkJXePmT/KYwL05UL0bmXAPlVgi9fADo1ZYLd9NXfTykOwwRPpgA== X-Received: by 2002:a05:620a:914:: with SMTP id v20mr119050qkv.107.1589382809491; Wed, 13 May 2020 08:13:29 -0700 (PDT) Original-Received: from ?IPv6:2601:184:4180:66e7:4d17:b25e:8d9:2188? ([2601:184:4180:66e7:4d17:b25e:8d9:2188]) by smtp.googlemail.com with ESMTPSA id c41sm16342255qta.96.2020.05.13.08.13.28 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 13 May 2020 08:13:28 -0700 (PDT) In-Reply-To: Content-Language: en-GB 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:180160 Archived-At: This is a multi-part message in MIME format. --------------8E749A14CB345923344EA199 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 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. --------------8E749A14CB345923344EA199 Content-Type: text/x-patch; charset=UTF-8; name="0001-Store-frame-faces-in-hash-tables-instead-of-alists.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0001-Store-frame-faces-in-hash-tables-instead-of-alists.patc"; filename*1="h" >From 75d44050a9277c708c69afe09de334ab3c300da1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Pit-Claudel?= 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 --------------8E749A14CB345923344EA199--