bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Clément Pit-Claudel]

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

Hi all,

I've recently noticed that opening a tooltip on my machine takes about 0.5s when x-gtk-use-system-tooltips is set to nil.
I bisected my config, and… nothing.  It's not one package: instead, it's an accumulation of small slowdowns.
Is seems that defining a face makes x-show-tip a tiny bit slower, but these effects stack.

Here is a repro:

  (defun my-def-many-faces (nfaces)
    (dotimes (i nfaces)
      (custom-declare-face
       (intern (format "my-face-%d" i))
       '((t)) "A face."
       :group 'basic-faces)))

  (defun my-bench-x-tip (nfaces)
    (setq x-gtk-use-system-tooltips nil)
    (my-def-many-faces nfaces)
    (benchmark-run 1 (x-show-tip "Test" (selected-frame) nil 5 nil nil)))

  (my-bench-x-tip 100) ;; ⇒ (0.035934318 1 0.015908304000000012)
  (my-bench-x-tip 200) ;; ⇒ (0.049593474 1 0.01508625500000002)
  (my-bench-x-tip 300) ;; ⇒ (0.094929297 2 0.03376510099999999)
  (my-bench-x-tip 400) ;; ⇒ (0.094900665 2 0.03254889999999999)
  (my-bench-x-tip 500) ;; ⇒ (0.118183442 2 0.03218763600000002)
  (my-bench-x-tip 600) ;; ⇒ (0.154759438 3 0.04923829399999996)
  (my-bench-x-tip 700) ;; ⇒ (0.183241646 3 0.04901039700000004)
  (my-bench-x-tip 800) ;; ⇒ (0.212218872 3 0.050182316999999976)
  (my-bench-x-tip 900) ;; ⇒ (0.248743542 3 0.04915146899999995)
  (my-bench-x-tip 1000) ;; ⇒ (0.29221963 3 0.04943874300000006)
  (my-bench-x-tip 1100) ;; ⇒ (0.334084605 3 0.05403986499999991)
  (my-bench-x-tip 1200) ;; ⇒ (0.397292289 4 0.06869684599999992)
  (my-bench-x-tip 1300) ;; ⇒ (0.442873256 4 0.06865671799999995)
  (my-bench-x-tip 1400) ;; ⇒ (0.492474982 4 0.06888139900000001)
  (my-bench-x-tip 1500) ;; ⇒ (0.579180262 5 0.08583425400000011)
  (my-bench-x-tip 1600) ;; ⇒ (0.63504114 5 0.08973981699999989)
  (my-bench-x-tip 1700) ;; ⇒ (0.723722857 5 0.09094433899999999)
  (my-bench-x-tip 1800) ;; ⇒ (0.791952279 5 0.08777533800000015)
  (my-bench-x-tip 1900) ;; ⇒ (0.902377982 6 0.10768666300000018)
  (my-bench-x-tip 2000) ;; ⇒ (0.998815784 6 0.11384837999999986)

Be sure to run it in emacs -q, not emacs -Q, because emacs -Q ignores X resources and hence skips the body of make-face-x-resource-internal, which contributes greatly to the issue.
For some reasons the effects are a bit worse in my config — roughly a factor 3 to 5 (I have 600 faces defined, and each tooltip takes .5s to display).  The profiles below suggest that face-spec-set-2 is called in my config, but not in my repro, which could explain part of the difference.

This is what the profile in emacs -q looks like:

- command-execute                                                1742  97%
 - call-interactively                                            1742  97%
  - funcall-interactively                                        1720  96%
   - eval-defun                                                  1711  95%
    - elisp--eval-defun                                          1711  95%
     - eval-region                                               1711  95%
      - let                                                      1711  95%
       - list                                                    1711  95%
        - let                                                    1711  95%
         - x-show-tip                                            1708  95%
          - face-set-after-frame-default                         1708  95%
           - face-spec-recalc                                    1654  92%
            - make-face-x-resource-internal                      1414  78%
             - set-face-attributes-from-resources                1413  78%
              - set-face-attribute-from-resource                 1394  77%
               - face-name                                       1353  75%
                - check-face                                     1348  75%
                   facep                                         1344  75%
            - face-spec-reset-face                                239  13%
             - apply                                              239  13%
                set-face-attribute                                234  13%

And this is what it looks like in my config:

- command-execute                                                1423  87%
 - call-interactively                                            1423  87%
  - apply                                                        1423  87%
   - call-interactively@ido-cr+-record-current-command               1423  87%
    - apply                                                      1423  87%
     - #<subr call-interactively>                                1423  87%
      - funcall-interactively                                    1423  87%
       - eval-defun                                              1345  83%
        - apply                                                  1345  83%
         - #<compiled 0x1fa5d1dc39debc9e>                        1345  83%
          - elisp--eval-defun                                    1345  83%
           - eval-region                                         1344  83%
            - apply                                              1344  83%
             - #<lambda -0x120930d847119138>                     1344  83%
              - endless/eval-overlay                             1344  83%
               - apply                                           1343  83%
                - #<subr eval-region>                            1343  83%
                 - my-bench-x-tip                                1343  83%
                  - let                                          1280  79%
                   - list                                        1280  79%
                    - let                                        1280  79%
                     - x-show-tip                                1277  78%
                      - face-set-after-frame-default               1277  78%
                       - face-spec-recalc                        1218  75%
                        - face-spec-set-2                         673  41%
                         - apply                                  672  41%
                          - set-face-attribute                    671  41%
                           - internal-set-lisp-face-attribute                669  41%
                            - frame-set-background-mode                651  40%
                             - face-spec-recalc                   411  25%
                              - make-face-x-resource-internal                352  21%
                               - set-face-attributes-from-resources                350  21%
                                - set-face-attribute-from-resource                343  21%
                                 - face-name                      312  19%
                                  - check-face                    309  19%
                                     facep                        308  19%
                              + face-spec-reset-face                 56   3%
                                face-spec-choose                    1   0%
                              + face-spec-set-2                     1   0%
                             - face-attr-match-p                  235  14%
                                face-attribute                    235  14%
                        - make-face-x-resource-internal                321  19%
                         - set-face-attributes-from-resources                320  19%
                          - set-face-attribute-from-resource                316  19%
                           - face-name                            296  18%
                            - check-face                          294  18%
                               facep                              293  18%
                        - face-spec-reset-face                    223  13%
                         - apply                                  223  13%
                            set-face-attribute                    219  13%
                  + my-def-many-faces                              63   3%
               + cider--make-result-overlay                         1   0%
           + end-of-defun                                           1   0%
       + smex                                                      78   4%
+ ...                                                             188  11%
+ timer-event-handler                                               4   0%
+ redisplay_internal (C function)                                   2   0%
+ flyspell-post-command-hook                                        1   0%


I've attached both profiles.

Clément.

Configured using:
 'configure -C'

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB NOTIFY
INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF
ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS LIBSYSTEMD
JSON PDUMPER LCMS2 GMP

Important settings:
  value of $LC_MONETARY: en_DK.UTF-8
  value of $LC_NUMERIC: en_DK.UTF-8
  value of $LC_TIME: en_DK.UTF-8
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Fundamental

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
format-spec rfc822 mml easymenu mml-sec password-cache epa derived epg
epg-config gnus-util rmail rmail-loaddefs text-property-search time-date
subr-x seq byte-opt gv bytecomp byte-compile cconv mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs
cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
timer select scroll-bar mouse jit-lock font-lock syntax facemenu
font-core term/tty-colors frame minibuffer cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote threads dbusbind
inotify lcms2 dynamic-setting system-font-setting font-render-setting
cairo move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 48162 5363)
 (symbols 48 6344 1)
 (strings 32 15896 1092)
 (string-bytes 1 517314)
 (vectors 16 10213)
 (vector-slots 8 140571 9444)
 (floats 8 19 41)
 (intervals 56 230 7)
 (buffers 992 11))

[-- Attachment #2: tip.emacs-q.prof --]
[-- Type: text/plain, Size: 3592 bytes --]


[profiler-profile "24.3" cpu #s(hash-table size 65 test equal rehash-size 1.5 rehash-threshold 0.8125 data ([redisplay sit-for execute-extended-command funcall-interactively call-interactively command-execute nil nil nil nil nil nil nil nil nil nil] 4 [sit-for execute-extended-command funcall-interactively call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil] 3 [let list let eval-region elisp--eval-defun eval-defun funcall-interactively call-interactively command-execute nil nil nil nil nil nil nil] 3 [facep check-face face-name set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal face-spec-recalc face-set-after-frame-default x-show-tip let list let eval-region elisp--eval-defun eval-defun funcall-interactively] 1344 [set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal face-spec-recalc face-set-after-frame-default x-show-tip let list let eval-region elisp--eval-defun eval-defun funcall-interactively call-interactively command-execute nil] 41 [face-name set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal face-spec-recalc face-set-after-frame-default x-show-tip let list let eval-region elisp--eval-defun eval-defun funcall-interactively call-interactively command-execute] 5 [set-face-attributes-from-resources make-face-x-resource-internal face-spec-recalc face-set-after-frame-default x-show-tip let list let eval-region elisp--eval-defun eval-defun funcall-interactively call-interactively command-execute nil nil] 19 [face-set-after-frame-default x-show-tip let list let eval-region elisp--eval-defun eval-defun funcall-interactively call-interactively command-execute nil nil nil nil nil] 54 [set-face-attribute apply face-spec-reset-face face-spec-recalc face-set-after-frame-default x-show-tip let list let eval-region elisp--eval-defun eval-defun funcall-interactively call-interactively command-execute nil] 234 [face-spec-recalc face-set-after-frame-default x-show-tip let list let eval-region elisp--eval-defun eval-defun funcall-interactively call-interactively command-execute nil nil nil nil] 1 [check-face face-name set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal face-spec-recalc face-set-after-frame-default x-show-tip let list let eval-region elisp--eval-defun eval-defun funcall-interactively call-interactively] 4 [make-face-x-resource-internal face-spec-recalc face-set-after-frame-default x-show-tip let list let eval-region elisp--eval-defun eval-defun funcall-interactively call-interactively command-execute nil nil nil] 1 [apply face-spec-reset-face face-spec-recalc face-set-after-frame-default x-show-tip let list let eval-region elisp--eval-defun eval-defun funcall-interactively call-interactively command-execute nil nil] 5 [nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 2 [tooltip-hide nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [completing-read-default completing-read read-extended-command byte-code call-interactively command-execute nil nil nil nil nil nil nil nil nil nil] 13 [read-from-minibuffer completing-read-default completing-read read-extended-command byte-code call-interactively command-execute nil nil nil nil nil nil nil nil nil] 9 [profiler-report funcall-interactively call-interactively command-execute execute-extended-command funcall-interactively call-interactively command-execute nil nil nil nil nil nil nil nil] 2 [Automatic\ GC] 47)) (24250 7797 63752 520000) nil]

[-- Attachment #3: tip.personal-config.prof --]
[-- Type: text/plain, Size: 37568 bytes --]


[profiler-profile "24.3" cpu #s(hash-table size 65 test equal rehash-size 1.5 rehash-threshold 0.8125 data ([set-face-attribute page-break-lines--update-display-table mapc page-break-lines--update-display-tables redisplay_internal\ \(C\ function\) nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 140 [ispell-send-string flyspell-word flyspell-post-command-hook nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [apply eldoc-minibuffer-message eldoc-message eldoc-print-current-symbol-info "#<compiled 0x16f6975adace3adf>" apply timer-event-handler nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 2 [syntax-ppss beginning-of-defun-raw end-of-defun elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [elisp--beginning-of-sexp elisp--fnsym-in-current-sexp elisp-eldoc-documentation-function run-hook-with-args-until-success eldoc-documentation-default eldoc-print-current-symbol-info "#<compiled 0x16f6975adace3adf>" apply timer-event-handler nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [intern custom-declare-face while let my-def-many-faces my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [make-face make-empty-face face-spec-set custom-declare-face while let my-def-many-faces my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 18 [set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal face-spec-recalc face-spec-set custom-declare-face while let my-def-many-faces my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 5 [custom-handle-keyword custom-handle-all-keywords custom-declare-face while let my-def-many-faces my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 13 [facep make-face make-empty-face face-spec-set custom-declare-face while let my-def-many-faces my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 15 [set-face-attributes-from-resources make-face-x-resource-internal make-face make-empty-face face-spec-set custom-declare-face while let my-def-many-faces my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [face-name set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal make-face make-empty-face face-spec-set custom-declare-face while let my-def-many-faces my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [apply face-spec-reset-face face-spec-recalc face-spec-set custom-declare-face while let my-def-many-faces my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [facep check-face face-name set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal make-face make-empty-face face-spec-set custom-declare-face while let my-def-many-faces my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [make-face-x-resource-internal make-face make-empty-face face-spec-set custom-declare-face while let my-def-many-faces my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 2 [set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal make-face make-empty-face face-spec-set custom-declare-face while let my-def-many-faces my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 3 [check-face face-name set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal make-face make-empty-face face-spec-set custom-declare-face while let my-def-many-faces my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [facemenu-add-new-face make-face make-empty-face face-spec-set custom-declare-face while let my-def-many-faces my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 3 [frame-set-background-mode internal-set-lisp-face-attribute set-face-attribute apply face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 5 [face-attribute face-attr-match-p frame-set-background-mode internal-set-lisp-face-attribute set-face-attribute apply face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 235 [internal-set-lisp-face-attribute set-face-attribute apply face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 18 [facep check-face face-name set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal face-spec-recalc frame-set-background-mode internal-set-lisp-face-attribute set-face-attribute apply face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 308 [set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal face-spec-recalc frame-set-background-mode internal-set-lisp-face-attribute set-face-attribute apply face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 31 [set-face-attribute apply face-spec-reset-face face-spec-recalc frame-set-background-mode internal-set-lisp-face-attribute set-face-attribute apply face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 53 [set-face-attributes-from-resources make-face-x-resource-internal face-spec-recalc frame-set-background-mode internal-set-lisp-face-attribute set-face-attribute apply face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 7 [make-face-x-resource-internal face-spec-recalc frame-set-background-mode internal-set-lisp-face-attribute set-face-attribute apply face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 2 [face-spec-recalc frame-set-background-mode internal-set-lisp-face-attribute set-face-attribute apply face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [check-face face-name set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal face-spec-recalc frame-set-background-mode internal-set-lisp-face-attribute set-face-attribute apply face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [apply face-spec-reset-face face-spec-recalc frame-set-background-mode internal-set-lisp-face-attribute set-face-attribute apply face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 3 [face-name set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal face-spec-recalc frame-set-background-mode internal-set-lisp-face-attribute set-face-attribute apply face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 3 [face-spec-choose face-spec-recalc frame-set-background-mode internal-set-lisp-face-attribute set-face-attribute apply face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [set-face-attribute apply face-spec-set-2 face-spec-recalc frame-set-background-mode internal-set-lisp-face-attribute set-face-attribute apply face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [set-face-attribute apply face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 2 [facep check-face face-name set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 293 [set-face-attribute apply face-spec-reset-face face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 219 [face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 59 [set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 20 [set-face-attributes-from-resources make-face-x-resource-internal face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 4 [face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [face-name set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 2 [apply face-spec-reset-face face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 4 [make-face-x-resource-internal face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [apply face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [check-face face-name set-face-attribute-from-resource set-face-attributes-from-resources make-face-x-resource-internal face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [face-spec-set-2 face-spec-recalc face-set-after-frame-default x-show-tip let list let my-bench-x-tip "#<subr eval-region>" apply endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [forward-sexp backward-sexp clojure-backward-logical-sexp cider--make-result-overlay endless/eval-overlay "#<lambda -0x120930d847119138>" apply eval-region elisp--eval-defun "#<compiled 0x1fa5d1dc39debc9e>" apply eval-defun funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [eval format-mode-line sml/fill-width-available sml/generate-minor-modes eval redisplay_internal\ \(C\ function\) nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 ["#<compiled 0xa721f0ff64839>" apply timer-event-handler nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 ["#<compiled 0xde148383988e753>" mapatoms smex-detect-new-commands smex funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 4 [mapatoms smex-detect-new-commands smex funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 ["#<compiled -0x3ac6ecab74afd64>" mapc ido-set-matches-1 ido-set-matches ido-read-internal "#<compiled 0xd9710a97778ae65>" apply ido-completing-read@ido-cr+-replace apply ido-completing-read smex-completing-read smex-read-and-run smex funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [ido-set-matches-1 ido-set-matches ido-read-internal "#<compiled 0xd9710a97778ae65>" apply ido-completing-read@ido-cr+-replace apply ido-completing-read smex-completing-read smex-read-and-run smex funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 ["#<compiled -0x3ac6ecab74afd64>" mapc ido-set-matches-1 ido-set-matches "#<compiled 0x166983c3be51b56a>" apply ido-exhibit read-from-minibuffer ido-read-internal "#<compiled 0xd9710a97778ae65>" apply ido-completing-read@ido-cr+-replace apply ido-completing-read smex-completing-read smex-read-and-run smex funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 16 [mapc ido-set-matches-1 ido-set-matches "#<compiled 0x166983c3be51b56a>" apply ido-exhibit read-from-minibuffer ido-read-internal "#<compiled 0xd9710a97778ae65>" apply ido-completing-read@ido-cr+-replace apply ido-completing-read smex-completing-read smex-read-and-run smex funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 5 [ido-completions "#<compiled 0x166983c3be51b56a>" apply ido-exhibit read-from-minibuffer ido-read-internal "#<compiled 0xd9710a97778ae65>" apply ido-completing-read@ido-cr+-replace apply ido-completing-read smex-completing-read smex-read-and-run smex funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [read-from-minibuffer ido-read-internal "#<compiled 0xd9710a97778ae65>" apply ido-completing-read@ido-cr+-replace apply ido-completing-read smex-completing-read smex-read-and-run smex funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 34 [ido-read-internal "#<compiled 0xd9710a97778ae65>" apply ido-completing-read@ido-cr+-replace apply ido-completing-read smex-completing-read smex-read-and-run smex funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 10 [mapcar ido-find-common-substring ido-set-common-completion "#<compiled 0x166983c3be51b56a>" apply ido-exhibit read-from-minibuffer ido-read-internal "#<compiled 0xd9710a97778ae65>" apply ido-completing-read@ido-cr+-replace apply ido-completing-read smex-completing-read smex-read-and-run smex funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [apply ido-tidy read-from-minibuffer ido-read-internal "#<compiled 0xd9710a97778ae65>" apply ido-completing-read@ido-cr+-replace apply ido-completing-read smex-completing-read smex-read-and-run smex funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [ido-word-matching-substring mapcar ido-find-common-substring ido-set-common-completion "#<compiled 0x166983c3be51b56a>" apply ido-exhibit read-from-minibuffer ido-read-internal "#<compiled 0xd9710a97778ae65>" apply ido-completing-read@ido-cr+-replace apply ido-completing-read smex-completing-read smex-read-and-run smex funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [self-insert-command funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute read-from-minibuffer ido-read-internal "#<compiled 0xd9710a97778ae65>" apply ido-completing-read@ido-cr+-replace apply ido-completing-read smex-completing-read smex-read-and-run smex funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [profiler-report funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute execute-extended-command smex-read-and-run smex funcall-interactively "#<subr call-interactively>" apply call-interactively@ido-cr+-record-current-command apply call-interactively command-execute nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil] 1 [Automatic\ GC] 188)) (24250 9524 179007 803000) nil]

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[martin rudalics]

 > Is seems that defining a face makes x-show-tip a tiny bit slower, but
 > these effects stack.

Please try with 'tooltip-reuse-hidden-frame' non-nil (there's nothing we
can do about a session's first tooltip appearance, though).

martin

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Clément Pit-Claudel]

On 12/05/2020 02.42, martin rudalics wrote:
>> Is seems that defining a face makes x-show-tip a tiny bit slower, but
>> these effects stack.
> 
> Please try with 'tooltip-reuse-hidden-frame' non-nil (there's nothing we
> can do about a session's first tooltip appearance, though).

I'm not seeing a difference here.  I used this code to test:


(defun my-def-many-faces (nfaces)
  (dotimes (i nfaces)
    (custom-declare-face
     (intern (format "my-face-%d" i))
     '((t)) "A face."
     :group 'basic-faces)))

(defun my-bench-x-tip (nfaces)
  (setq x-gtk-use-system-tooltips nil
        tooltip-reuse-hidden-frame t)
  (my-def-many-faces nfaces)
  (benchmark-run 1 (x-show-tip "Test" (selected-frame) nil 5 nil nil)))

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[martin rudalics]

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

 >> Please try with 'tooltip-reuse-hidden-frame' non-nil (there's nothing we
 >> can do about a session's first tooltip appearance, though).
 >
 > I'm not seeing a difference here.  I used this code to test:

Looks like a devastating bug in the GTK builds.  Can you try with the
attached patch?

Thanks, martin

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: tooltip-hide.diff --]
[-- Type: text/x-patch; name="tooltip-hide.diff", Size: 458 bytes --]

diff --git a/src/xfns.c b/src/xfns.c
index a5431aa890..a8cdd66b57 100644
--- a/src/xfns.c
+++ b/src/xfns.c
@@ -6748,7 +6748,8 @@ x_hide_tip (bool delete)

       /* Reset tip_last_frame, it will be reassigned when showing the
 	 next GTK+ system tooltip.  */
-      tip_last_frame = Qnil;
+      if (x_gtk_use_system_tooltips)
+	tip_last_frame = Qnil;

       /* Now look whether there's an Emacs tip around.  */
       if (FRAMEP (tip_frame))

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Clément Pit-Claudel]

On 12/05/2020 11.12, martin rudalics wrote:
>>> Please try with 'tooltip-reuse-hidden-frame' non-nil (there's nothing we
>>> can do about a session's first tooltip appearance, though).
>>
>> I'm not seeing a difference here.  I used this code to test:
> 
> Looks like a devastating bug in the GTK builds.  Can you try with the
> attached patch?
> 
> Thanks, martin

Indeed, with tooltip-reuse-hidden-frame t and your patch, creating a tooltip is instantaneous (after the first tooltip is created)
The docstring suggests that with this option, results won't always be correct.  Is there a chance that we could rebuild the frame when needed and then make that option the default?

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[martin rudalics]

 > Indeed, with tooltip-reuse-hidden-frame t and your patch, creating a
 >  tooltip is instantaneous (after the first tooltip is created)

Eli, any problems to fix this in Emacs 27?

 > The docstring suggests that with this option, results won't always be
 > correct.  Is there a chance that we could rebuild the frame when
 > needed and then make that option the default?

It depends on what "when needed" stands for.  Basically, the results may
be incorrect when some sort of face change happens.  But, as I recently
mentioned in another thread, the code not reusing a hidden frame already
fails picking up an internal border face specified via

   (set-face-background 'internal-border "red")

so such annoyances are already present in the default code.

In principle, you can always add or remove some non-position-specifying
alist entry and the tooltip frame will be recreated from scratch.  So if
we can identify the "when needed", it should be easy to add such an
entry and remove it as soon as the next tooltip frame has been created.

martin

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Eli Zaretskii]

> From: martin rudalics <rudalics@gmx.at>
> Date: Tue, 12 May 2020 19:42:52 +0200
> 
>  > Indeed, with tooltip-reuse-hidden-frame t and your patch, creating a
>  >  tooltip is instantaneous (after the first tooltip is created)
> 
> Eli, any problems to fix this in Emacs 27?

No, please go ahead.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[martin rudalics]

> No, please go ahead.

Installed meanwhile.

Thanks, martin

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Eli Zaretskii]

> Resent-From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Original-Sender: "Debbugs-submit" <debbugs-submit-bounces@debbugs.gnu.org>
> Resent-CC: bug-gnu-emacs@gnu.org
> Resent-Sender: help-debbugs@gnu.org
> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Tue, 12 May 2020 00:30:23 -0400
> 
> 
> [1:text/plain Hide]
> 
> Hi all,
> 
>   (defun my-def-many-faces (nfaces)
>     (dotimes (i nfaces)
>       (custom-declare-face
>        (intern (format "my-face-%d" i))
>        '((t)) "A face."
>        :group 'basic-faces)))
> 
>   (defun my-bench-x-tip (nfaces)
>     (setq x-gtk-use-system-tooltips nil)
>     (my-def-many-faces nfaces)
>     (benchmark-run 1 (x-show-tip "Test" (selected-frame) nil 5 nil nil)))
> 
>   (my-bench-x-tip 100) ;; ⇒ (0.035934318 1 0.015908304000000012)
>   (my-bench-x-tip 200) ;; ⇒ (0.049593474 1 0.01508625500000002)
>   (my-bench-x-tip 300) ;; ⇒ (0.094929297 2 0.03376510099999999)
>   (my-bench-x-tip 400) ;; ⇒ (0.094900665 2 0.03254889999999999)
>   (my-bench-x-tip 500) ;; ⇒ (0.118183442 2 0.03218763600000002)
>   (my-bench-x-tip 600) ;; ⇒ (0.154759438 3 0.04923829399999996)
>   (my-bench-x-tip 700) ;; ⇒ (0.183241646 3 0.04901039700000004)
>   (my-bench-x-tip 800) ;; ⇒ (0.212218872 3 0.050182316999999976)
>   (my-bench-x-tip 900) ;; ⇒ (0.248743542 3 0.04915146899999995)
>   (my-bench-x-tip 1000) ;; ⇒ (0.29221963 3 0.04943874300000006)
>   (my-bench-x-tip 1100) ;; ⇒ (0.334084605 3 0.05403986499999991)
>   (my-bench-x-tip 1200) ;; ⇒ (0.397292289 4 0.06869684599999992)
>   (my-bench-x-tip 1300) ;; ⇒ (0.442873256 4 0.06865671799999995)
>   (my-bench-x-tip 1400) ;; ⇒ (0.492474982 4 0.06888139900000001)
>   (my-bench-x-tip 1500) ;; ⇒ (0.579180262 5 0.08583425400000011)
>   (my-bench-x-tip 1600) ;; ⇒ (0.63504114 5 0.08973981699999989)
>   (my-bench-x-tip 1700) ;; ⇒ (0.723722857 5 0.09094433899999999)
>   (my-bench-x-tip 1800) ;; ⇒ (0.791952279 5 0.08777533800000015)
>   (my-bench-x-tip 1900) ;; ⇒ (0.902377982 6 0.10768666300000018)
>   (my-bench-x-tip 2000) ;; ⇒ (0.998815784 6 0.11384837999999986)
> 
> Be sure to run it in emacs -q, not emacs -Q, because emacs -Q ignores X resources and hence skips the body of make-face-x-resource-internal, which contributes greatly to the issue.
> For some reasons the effects are a bit worse in my config — roughly a factor 3 to 5 (I have 600 faces defined, and each tooltip takes .5s to display).  The profiles below suggest that face-spec-set-2 is called in my config, but not in my repro, which could explain part of the difference.
> 
> This is what the profile in emacs -q looks like:
> 
> - command-execute                                                1742  97%
>  - call-interactively                                            1742  97%
>   - funcall-interactively                                        1720  96%
>    - eval-defun                                                  1711  95%
>     - elisp--eval-defun                                          1711  95%
>      - eval-region                                               1711  95%
>       - let                                                      1711  95%
>        - list                                                    1711  95%
>         - let                                                    1711  95%
>          - x-show-tip                                            1708  95%
>           - face-set-after-frame-default                         1708  95%
>            - face-spec-recalc                                    1654  92%
>             - make-face-x-resource-internal                      1414  78%
>              - set-face-attributes-from-resources                1413  78%
>               - set-face-attribute-from-resource                 1394  77%
>                - face-name                                       1353  75%
>                 - check-face                                     1348  75%
>                    facep                                         1344  75%

If you look at internal-lisp-face-p, which is the workhorse of facep,
you will see that it does the moral equivalent of

  (assq FACE (frame-face-alist))

(The code is actually in a subroutine called by internal-lisp-face-p.)
Which means face-set-after-frame-default, which loops over all of the
faces, runs with O(n²) complexity in the number of faces.

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.

> Configured using:
>  'configure -C'
> 
> Configured features:
> XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB NOTIFY
> INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF
> ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS LIBSYSTEMD
> JSON PDUMPER LCMS2 GMP

No Emacs version information?

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Clément Pit-Claudel]

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

On 12/05/2020 11.27, Eli Zaretskii wrote:
> (The code is actually in a subroutine called by internal-lisp-face-p.)
> Which means face-set-after-frame-default, which loops over all of the
> faces, runs with O(n²) complexity in the number of faces.
> 
> 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? 
* 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? 
* I imagine that this change needs to be advertised somewhere, but I'm not sure where; NEWS?

Lastly, do the following new profiles suggest other opportunities for improvement?

- ...                                                             499  52%
   Automatic GC                                                   499  52%
- command-execute                                                 454  47%
 - call-interactively                                             454  47%
  - funcall-interactively                                         433  45%
   - eval-defun                                                   427  44%
    - elisp--eval-defun                                           427  44%
     - eval-region                                                426  44%
      - my-bench-x-tip                                            426  44%
       - let                                                      406  42%
        - list                                                    406  42%
         - let                                                    406  42%
          - x-show-tip                                            390  40%
           - face-set-after-frame-default                         387  40%
            - face-spec-recalc                                    374  39%
             - make-face-x-resource-internal                      296  30%
              - set-face-attributes-from-resources                273  28%
               - set-face-attribute-from-resource                 219  22%
                + face-name                                        65   6%
             + face-spec-reset-face                                62   6%
             + face-spec-set-2                                      6   0%
             + face-spec-choose                                     2   0%
            + face-list                                             2   0%
           + frame-windows-min-size                                 1   0%
       + my-def-many-faces                                         20   2%
     + end-of-defun                                                 1   0%
   + execute-extended-command                                       6   0%
  + byte-code                                                      21   2%
+ redisplay_internal (C function)                                   2   0%
  tooltip-hide                                                      1   0%

- command-execute                                                 768  80%
 - call-interactively                                             768  80%
  - apply                                                         768  80%
   - call-interactively@ido-cr+-record-current-command                768  80%
    - apply                                                       768  80%
     - #<subr call-interactively>                                 768  80%
      - funcall-interactively                                     768  80%
       - eval-defun                                               715  74%
        - apply                                                   713  74%
         - #<compiled 0x151e3cbebf653c17>                         712  74%
          - elisp--eval-defun                                     712  74%
           - eval-region                                          709  74%
            - apply                                               709  74%
             - #<lambda 0x32ad1cc4311e0c0>                        709  74%
              - endless/eval-overlay                              709  74%
               - apply                                            709  74%
                - #<subr eval-region>                             707  74%
                 - my-bench-x-tip                                 707  74%
                  - let                                           689  72%
                   - list                                         689  72%
                    - let                                         689  72%
                     - x-show-tip                                 676  70%
                      - face-set-after-frame-default                674  70%
                       - face-spec-recalc                         660  69%
                        - face-spec-set-2                         350  36%
                         - apply                                  348  36%
                          - set-face-attribute                    342  35%
                           - internal-set-lisp-face-attribute                342  35%
                            - frame-set-background-mode                331  34%
                             - face-spec-recalc                   284  29%
                              - make-face-x-resource-internal                235  24%
                               - set-face-attributes-from-resources                216  22%
                                - set-face-attribute-from-resource                174  18%
                                 - face-name                       36   3%
                                  + check-face                     21   2%
                              + face-spec-reset-face                 40   4%
                              + face-spec-set-2                     4   0%
                             + face-attr-match-p                   24   2%
                               face-spec-choose                     1   0%
                             + face-list                            1   0%
                        - make-face-x-resource-internal                248  25%
                         - set-face-attributes-from-resources                215  22%
                          - set-face-attribute-from-resource                169  17%
                           + face-name                             36   3%
                        + face-spec-reset-face                     54   5%
                        + face-spec-choose                          2   0%
                       + face-list                                  1   0%
                  + my-def-many-faces                              18   1%
           + beginning-of-defun                                     1   0%
             end-of-defun                                           1   0%
        + #<lambda 0x159f62efd027a>                                 2   0%
       + smex                                                      53   5%
- ...                                                             182  19%
   Automatic GC                                                   182  19%
+ redisplay_internal (C function)                                   3   0%

Also, since the GC seems to be a significant part, here's a memory profile:

- command-execute                                         305,314,261  99%
 - call-interactively                                     305,314,261  99%
  - funcall-interactively                                 305,262,257  99%
   - eval-defun                                           303,318,241  98%
    - elisp--eval-defun                                   303,317,185  98%
     - eval-region                                        303,296,332  98%
      - my-bench-x-tip                                    303,295,276  98%
       - let                                              273,049,377  89%
        - list                                            273,049,377  89%
         - let                                            273,049,377  89%
          - x-show-tip                                    177,538,262  57%
           - face-set-after-frame-default                 175,519,190  57%
            - face-spec-recalc                            174,935,046  57%
             - make-face-x-resource-internal              138,435,960  45%
              + set-face-attributes-from-resources        138,407,728  45%
             + face-spec-reset-face                        36,126,838  11%
             + face-spec-choose                                74,360   0%
             + face-spec-set-2                                 21,216   0%
            + face-list                                       554,400   0%
           + frame-windows-min-size                             7,676   0%
           + run-at-time                                        4,352   0%
            setq                                                4,224   0%
          + float-time                                          3,888   0%
       + my-def-many-faces                                 30,245,899   9%
      + internal-macroexpand-for-load                           1,056   0%
     + end-of-defun                                             4,160   0%
     + beginning-of-defun                                       2,112   0%
   + execute-extended-command                               1,944,016   0%
  + byte-code                                                  52,004   0%
+ redisplay_internal (C function)                           1,427,117   0%

> No Emacs version information?

Woops. Sorry! GNU Emacs 28.0.50 (build 10, x86_64-pc-linux-gnu, GTK+ Version 3.22.30, cairo version 1.15.10) of 2020-05-10

Thanks again for the pointers,
Clément.

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

From 6d3d5d94189ae31fdbc29b2db0707d6b13a5c362 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..4423965a01 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, Qnil));
+
   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..cccf0e4852 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, Qnil);
 
   DEFVAR_LISP ("face-default-stipple", Vface_default_stipple,
     doc: /* Default stipple pattern used on monochrome displays.
-- 
2.17.1

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[martin rudalics]

 > 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:

   CC       frame.o
In file included from ../../src/lisp.h:954,
                  from ../../src/frame.c:29:
../../src/frame.c: In function ‘make_frame’:
./globals.h:6365:14: error: incompatible type for argument 6 of ‘make_hash_table’
  #define Qnil builtin_lisp_symbol (0)
               ^~~~~~~~~~~~~~~~~~~~~~~
../../src/frame.c:952:57: note: in expansion of macro ‘Qnil’
                          DEFAULT_REHASH_THRESHOLD, Qnil, Qnil));
                                                          ^~~~
In file included from ../../src/conf_post.h:39,
                  from ./config.h:2270,
                  from ../../src/frame.c:20:
../../src/lisp.h:3661:43: note: expected ‘_Bool’ but argument is of type ‘Lisp_Object’ {aka ‘struct Lisp_Object’}
                               Lisp_Object, bool);
                                            ^~~~
make[1]: *** [Makefile:402: frame.o] Fehler 1
make[1]: *** Es wird auf noch nicht beendete Prozesse gewartet....
make[1]: Verzeichnis „/home/martin/emacs-git/release/obj-gtk/src“ wird verlassen
make: *** [Makefile:424: src] Fehler 2

martin

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Clément Pit-Claudel]

[-- 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

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[martin rudalics]

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

Thanks.  Builds now and I'm running with it.

martin

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Eli Zaretskii]

> 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?

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Clément Pit-Claudel]

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

Hi Eli,

Thanks a lot for the review.  I have attached an updated version of the patch.

On 15/05/2020 07.05, Eli Zaretskii wrote:
> 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?

>> * 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.

>> +  // 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).

>> -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.

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.

> 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?)

>> +	  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?  Should I make sure that it is? If so, where from?

> 
>> +          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.

> 
>>    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?)


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

From 86ea2f96e4e3a2161f68f13b251620b98489fd35 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/frame.el (frame-set-background-mode):
* lisp/faces.el (face-list): Update to work with hash
tables instead of alists.
(frame-face-alist): Reimplement using frame-face-map.

* src/frame.h (struct frame): Remove face_alist, add face_map.
(fset_face_alist): Remove.
(fset_face_map): New function.
* src/frame.c (make_frame): Initialize f->face_map.
(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_map): New function.
(Fframe_face_alist): Move to face.el.
(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_map;
change Vface_new_frame_defaults into a hash table.
---
 lisp/emacs-lisp/edebug.el    |  3 +-
 lisp/faces.el                | 13 +++++++-
 lisp/frame.el                |  2 +-
 lisp/progmodes/elisp-mode.el |  3 +-
 src/frame.c                  | 20 +++++++----
 src/frame.h                  |  8 ++---
 src/xfaces.c                 | 65 +++++++++++++++++++-----------------
 7 files changed, 67 insertions(+), 47 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..c45921bb48 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -176,10 +176,21 @@ face-font-registry-alternatives
 ;;; Creation, copying.
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
+(defun frame-face-alist (&optional frame)
+  "Return an alist of frame-local faces defined on FRAME.
+This alist is a copy of the contents of `frame-face-map'.
+For internal use only."
+  (declare (obsolete frame-face-map "28.1"))
+  (let ((faces))
+    (maphash (lambda (face spec) (push (cons face spec) faces))
+             (frame-face-map frame))
+    faces))
 
 (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)
+    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..14c9112aad 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-map))
                          (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..514b390910 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -946,6 +946,10 @@ make_frame (bool mini_p)
   rw->total_lines = mini_p ? 9 : 10;
   rw->pixel_height = rw->total_lines * FRAME_LINE_HEIGHT (f);
 
+  fset_face_map
+    (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 +1258,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 +1340,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_map (f, Fcopy_hash_table (sf->face_map));
+  /* Simple copy_hash_table isn't enough, because we need the contents of
+     the vectors which are the values in face_map 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_map);
+  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..f8915b3c1a 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_map;
 
   /* 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_map (struct frame *f, Lisp_Object val)
 {
-  f->face_alist = val;
+  f->face_map = val;
 }
 #if defined (HAVE_WINDOW_SYSTEM)
 INLINE void
diff --git a/src/xfaces.c b/src/xfaces.c
index 7d7aff95c1..89e9a10a0a 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_map, 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_map);
 	}
       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_map)) == 0)
     return;
 
   if (EQ (param, Qforeground_color))
@@ -4174,14 +4171,13 @@ 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,
+DEFUN ("frame-face-map", Fframe_face_map, Sframe_face_map,
        0, 1, 0,
-       doc: /* Return an alist of frame-local faces defined on FRAME.
+       doc: /* Return a hash table 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_map;
 }
 
 
@@ -6678,30 +6674,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 +6858,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_map);
   defsubr (&Sdisplay_supports_face_attributes_p);
   defsubr (&Scolor_distance);
   defsubr (&Sinternal_set_font_selection_order);
@@ -6881,7 +6884,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

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Eli Zaretskii]

> Cc: 41200@debbugs.gnu.org
> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> 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.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

>> 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.

You can use make_symbol_constant to make a variable read-only.  You
could also simulate it with a variable watcher that signals an error,
but there's no sense in doing that when make_symbol_constant exists.

Neither of these makes the list object itself read-only though.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Clément Pit-Claudel]

On 15/05/2020 11.17, Eli Zaretskii wrote:
>>>> * 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.

Got it.  Is face_table better? (that was Stefan's other suggestion)
 
>>> 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.

Would that be done in C, or in any place where frame-new-face-defaults is modified?  (for example, edebug changes face-new-frame-defaults in one place; if we keep the alist in addition to the hash table, would it modify both or would there be a C mechanism that mirrors hash-table modifications to the alist?)

> I'd really like to know that no one is using these before we make the
> final decision about this.

That's a fair point.  While I couldn't find uses of frame-face-alist, there are a few uses of face-new-frame-defaults-alist: https://github.com/pestctrl/emacs-config/commit/31d6bff97dacf60f71066902a23d37e59b4c1288 is from someone who uses that to speed up temporary frame creation(!), https://github.com/Lindydancer/face-explorer/blob/13bd4553bc4b09215a04d0267be1cb4ed834775c/test/face-explorer-test-faces.el is from Anders Lindgren (in CC; hi Andres! We're considering replacing face-new-frame-defaults-alist with a hash table, would that be an issue?), who uses it to remove a face, and https://github.com/Wilfred/elisp-fu/blob/92c491584f466ee729ea1b328234636e65e2c665/elisp-fu.el includes code that's the same as in eval-defun.

I have a variant of the patch that keeps the alist variable, but I fear that it's worse than removing it: since changes to the alist won't be propagated to the hash table, it might be better to error out with a compilation error?

Btw, I have one more question: some callers of face-list seems to rely on the order of faces added to it, so it would be better to preserve that order.  Since hash-tables are not necessarily ordered, should I sort faces by face-id before returning them? 

>> 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.

Ah, good catch.  Current there's a defface for tab-bar in lisp/tab-bar, and since that's preloaded it works, but the defface for tab-line is in lisp/tab-line.el and so isn't preloaded.
Should I move both to 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.

Sounds reasonable; but where would we store it?  Right now faces are just symbols, right?

> No, I was asking why not start with it as nil and actually make a
> hash-table when we first need it.

Oh, I thought it would be simpler to always have a hash table instead of having to sanity check every time.

Thanks a lot,
Clément.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Eli Zaretskii]

> Cc: 41200@debbugs.gnu.org, Anders Lindgren <andlind@gmail.com>
> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Fri, 15 May 2020 12:22:53 -0400
> 
> >>> 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.
> 
> Got it.  Is face_table better? (that was Stefan's other suggestion)

Is anything wrong with face_hash_table?

> > 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.
> 
> Would that be done in C, or in any place where frame-new-face-defaults is modified?  (for example, edebug changes face-new-frame-defaults in one place; if we keep the alist in addition to the hash table, would it modify both or would there be a C mechanism that mirrors hash-table modifications to the alist?)

The latter was what I had in mind.

> I have a variant of the patch that keeps the alist variable, but I fear that it's worse than removing it: since changes to the alist won't be propagated to the hash table, it might be better to error out with a compilation error?

Not sure yet, it depends on how this is used.

> Btw, I have one more question: some callers of face-list seems to rely on the order of faces added to it, so it would be better to preserve that order.  Since hash-tables are not necessarily ordered, should I sort faces by face-id before returning them? 

Yes, I think so.

> > I think the problem is that tab-line is declared a basic face, but its
> > defface form is not in faces.el.
> 
> Ah, good catch.  Current there's a defface for tab-bar in lisp/tab-bar, and since that's preloaded it works, but the defface for tab-line is in lisp/tab-line.el and so isn't preloaded.
> Should I move both to faces.el?

Yes, I think so.

> >>> 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.
> 
> Sounds reasonable; but where would we store it?  Right now faces are just symbols, right?

Don't hash-tables allow us to store more than one item in each slot?

> > No, I was asking why not start with it as nil and actually make a
> > hash-table when we first need it.
> 
> Oh, I thought it would be simpler to always have a hash table instead of having to sanity check every time.

What is the default size of a hash-table?

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Clément Pit-Claudel]

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

On 15/05/2020 13.28, Eli Zaretskii wrote:
>> Cc: 41200@debbugs.gnu.org, Anders Lindgren <andlind@gmail.com>
>> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
>> Date: Fri, 15 May 2020 12:22:53 -0400
>>
>>>>> 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.
>>
>> Got it.  Is face_table better? (that was Stefan's other suggestion)
> 
> Is anything wrong with face_hash_table?

Nope; I've attached a new patch.

>> Btw, I have one more question: some callers of face-list seems to rely on the order of faces added to it, so it would be better to preserve that order.  Since hash-tables are not necessarily ordered, should I sort faces by face-id before returning them? 

Done in the attached patch as well.

>>> I think the problem is that tab-line is declared a basic face, but its
>>> defface form is not in faces.el.
>>
>> Ah, good catch.  Current there's a defface for tab-bar in lisp/tab-bar, and since that's preloaded it works, but the defface for tab-line is in lisp/tab-line.el and so isn't preloaded.
>> Should I move both to faces.el?
>>
> Yes, I think so.

Thanks.  I will ask Juri to confirm before moving them, because I realize now that they have a custom group.
Juri (CC'd; hi Juri!), was there a reason to make tab-bar and tab-line basic faces?  I see they are both in their own files and groups, instead of being in faces.el.

>>>>> 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.
>>
>> Sounds reasonable; but where would we store it?  Right now faces are just symbols, right?
> 
> Don't hash-tables allow us to store more than one item in each slot?

They do, so I can certainly store (face-id . face-vector) in the hash table.  Done in the attached patch.  I only do this in Vface_new_frame_defaults, not in frame->face_hash_table.

>>> No, I was asking why not start with it as nil and actually make a
>>> hash-table when we first need it.
>>
>> Oh, I thought it would be simpler to always have a hash table instead of having to sanity check every time.
> 
> What is the default size of a hash-table?

65 entries, I think.


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

From be915d6d2ea7a739026dc4a53fab64f78efab2bd 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

* src/frame.h (struct frame): Add face_hash_table, remove face_alist.
(fset_face_hash_table): New function.
(fset_face_alist): Remove.
* src/frame.c (make_frame): Initialize f->face_hash_table.
(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.
(Fframe_face_hash_table): New function.
(Fframe_face_alist): Move to faces.el as frame-face-alist.
(syms_of_xfaces): Add frame_face_hash_table.

* lisp/emacs-lisp/edebug.el (edebug-eval-defun):
* lisp/progmodes/elisp-mode.el (elisp--eval-defun-1):
* lisp/frame.el (frame-set-background-mode): Update to work with hash
tables instead of alists.
* lisp/faces.el (face-new-frame-defaults): Mark obsolete.
(face-list): Update to use face--new-frame-defaults.
(frame-face-alist): Moved here from src/xfaces.c.
---
 lisp/custom.el               |  2 +-
 lisp/emacs-lisp/edebug.el    |  3 +-
 lisp/faces.el                | 23 ++++++++--
 lisp/frame.el                |  2 +-
 lisp/progmodes/elisp-mode.el |  3 +-
 src/frame.c                  | 20 ++++++---
 src/frame.h                  |  8 ++--
 src/xfaces.c                 | 82 ++++++++++++++++++------------------
 8 files changed, 83 insertions(+), 60 deletions(-)

diff --git a/lisp/custom.el b/lisp/custom.el
index 885c486c5e..3ac8bac40f 100644
--- a/lisp/custom.el
+++ b/lisp/custom.el
@@ -898,7 +898,7 @@ custom-push-theme
 	  ;; the value to a fake theme, `changed'.  If the theme is
 	  ;; later disabled, we use this to bring back the old value.
 	  ;;
-	  ;; For faces, we just use `face-new-frame-defaults' to
+	  ;; For faces, we just use `face--new-frame-defaults' to
 	  ;; recompute when the theme is disabled.
 	  (when (and (eq prop 'theme-value)
 		     (boundp symbol))
diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 78461185d3..76a75c04df 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..bb51797a38 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -176,10 +176,27 @@ face-font-registry-alternatives
 ;;; Creation, copying.
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
+(make-obsolete-variable
+ 'face-new-frame-defaults
+ "use `face--new-frame-defaults' or `face-alist' instead."
+ "28.1")
+
+(defun frame-face-alist (&optional frame)
+  "Return an alist of frame-local faces defined on FRAME.
+This alist is a copy of the contents of `frame--face-hash-table'.
+For internal use only."
+  (declare (obsolete frame--face-hash-table "28.1"))
+  (let ((faces))
+    (maphash (lambda (face spec) (push `(,(car spec) ,face . ,(cdr spec)) faces))
+             (frame--face-hash-table frame))
+    (mapcar #'cdr (sort faces (lambda (f1 f2) (< (car f1) (car f2)))))))
 
 (defun face-list ()
   "Return a list of all defined faces."
-  (mapcar #'car face-new-frame-defaults))
+  (let ((faces))
+    (maphash (lambda (face spec) (push `(,(car spec) . ,face) faces))
+             face--new-frame-defaults)
+    (mapcar #'cdr (sort faces (lambda (f1 f2) (< (car f1) (car f2)))))))
 
 (defun make-face (face)
   "Define a new face with name FACE, a symbol.
@@ -2097,7 +2114,7 @@ x-create-frame-with-faces
 (defun face-set-after-frame-default (frame &optional parameters)
   "Initialize the frame-local faces of FRAME.
 Calculate the face definitions using the face specs, custom theme
-settings, X resources, and `face-new-frame-defaults'.
+settings, X resources, and `face--new-frame-defaults'.
 Finally, apply any relevant face attributes found amongst the
 frame parameters in PARAMETERS."
   ;; The `reverse' is so that `default' goes first.
@@ -2106,7 +2123,7 @@ face-set-after-frame-default
 	(progn
 	  ;; Initialize faces from face spec and custom theme.
 	  (face-spec-recalc face frame)
-	  ;; Apply attributes specified by face-new-frame-defaults
+	  ;; Apply attributes specified by face--new-frame-defaults
 	  (internal-merge-in-global-face face frame))
       ;; Don't let invalid specs prevent frame creation.
       (error nil)))
diff --git a/lisp/frame.el b/lisp/frame.el
index 6c2f774709..66b5c6aa8d 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-table))
                          (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..75054b8818 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..0843d53559 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -946,6 +946,10 @@ make_frame (bool mini_p)
   rw->total_lines = mini_p ? 9 : 10;
   rw->pixel_height = rw->total_lines * FRAME_LINE_HEIGHT (f);
 
+  fset_face_hash_table
+    (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 +1258,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 +1340,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_table (f, Fcopy_hash_table (sf->face_hash_table));
+  /* Simple copy_hash_table isn't enough, because we need the contents of
+     the vectors which are the values in face_hash_table 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_table);
+  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..9761253085 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_table;
 
   /* 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_table (struct frame *f, Lisp_Object val)
 {
-  f->face_alist = val;
+  f->face_hash_table = val;
 }
 #if defined (HAVE_WINDOW_SYSTEM)
 INLINE void
diff --git a/src/xfaces.c b/src/xfaces.c
index 7d7aff95c1..eae991d1c2 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -95,9 +95,10 @@ Copyright (C) 1993-1994, 1998-2020 Free Software Foundation, Inc.
    with the symbol `face' in slot 0, and a slot for each of the face
    attributes mentioned above.
 
-   There is also a global face alist `Vface_new_frame_defaults'.  Face
-   definitions from this list are used to initialize faces of newly
-   created frames.
+   There is also a global face map `Vface_new_frame_defaults',
+   containing conses of (FACE_ID . FACE_DEFINITION).  Face definitions
+   from this table are used to initialize faces of newly created
+   frames.
 
    A face doesn't have to specify all attributes.  Those not specified
    have a value of `unspecified'.  Faces specifying all attributes but
@@ -1843,13 +1844,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_table, Qnil);
   else
-    lface = assq_no_quit (face_name, Vface_new_frame_defaults);
+    lface = CDR (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);
@@ -2734,11 +2733,6 @@ DEFUN ("internal-make-lisp-face", Finternal_make_lisp_face,
   /* Add a global definition if there is none.  */
   if (NILP (global_lface))
     {
-      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);
-
       /* 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.
 	 The mapping from Lisp face to Lisp face id is given by the
@@ -2748,9 +2742,14 @@ DEFUN ("internal-make-lisp-face", Finternal_make_lisp_face,
 	  xpalloc (lface_id_to_name, &lface_id_to_name_size, 1, MAX_FACE_ID,
 		   sizeof *lface_id_to_name);
 
+      Lisp_Object face_id = make_fixnum (next_lface_id);
       lface_id_to_name[next_lface_id] = face;
-      Fput (face, Qface, make_fixnum (next_lface_id));
+      Fput (face, Qface, face_id);
       ++next_lface_id;
+
+      global_lface = make_vector (LFACE_VECTOR_SIZE, Qunspecified);
+      ASET (global_lface, 0, Qface);
+      Fputhash (face, Fcons (face_id, global_lface), Vface_new_frame_defaults);
     }
   else if (f == NULL)
     for (i = 1; i < LFACE_VECTOR_SIZE; ++i)
@@ -2763,7 +2762,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_table);
 	}
       else
 	for (i = 1; i < LFACE_VECTOR_SIZE; ++i)
@@ -2924,7 +2923,7 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
       f = NULL;
       lface = lface_from_face_name (NULL, face, true);
 
-      /* When updating face-new-frame-defaults, we put :ignore-defface
+      /* When updating face--new-frame-defaults, we put :ignore-defface
 	 where the caller wants `unspecified'.  This forces the frame
 	 defaults to ignore the defface value.  Otherwise, the defface
 	 will take effect, which is generally not what is intended.
@@ -3508,7 +3507,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_table)) == 0)
     return;
 
   if (EQ (param, Qforeground_color))
@@ -4174,14 +4173,13 @@ 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,
+DEFUN ("frame--face-hash-table", Fframe_face_hash_table, Sframe_face_hash_table,
        0, 1, 0,
-       doc: /* Return an alist of frame-local faces defined on FRAME.
+       doc: /* Return a hash table 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_table;
 }
 
 
@@ -6678,30 +6676,32 @@ 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 = CAR (HASH_VALUE (table, idx));
+          if (FIXNATP (face_id)) {
+              int id = XFIXNAT (face_id);
+              eassert (id >= 0);
+              lface_id_to_name[id] = lface;
+            }
 	}
     }
   face_attr_sym[0] = Qface;
@@ -6855,7 +6855,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_table);
   defsubr (&Sdisplay_supports_face_attributes_p);
   defsubr (&Scolor_distance);
   defsubr (&Sinternal_set_font_selection_order);
@@ -6879,9 +6879,11 @@ syms_of_xfaces (void)
 the "specifity" of a face specification and should be let-bound
 only for this purpose.  */);
 
-  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;
+  DEFVAR_LISP ("face--new-frame-defaults", Vface_new_frame_defaults,
+    doc: /* Hash table of global face definitions (for internal use only.)  */);
+  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

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Eli Zaretskii]

> Cc: 41200@debbugs.gnu.org, Juri Linkov <juri@linkov.net>
> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Fri, 15 May 2020 14:50:17 -0400
> 
> >>> No, I was asking why not start with it as nil and actually make a
> >>> hash-table when we first need it.
> >>
> >> Oh, I thought it would be simpler to always have a hash table instead of having to sanity check every time.
> > 
> > What is the default size of a hash-table?
> 
> 65 entries, I think.

The number of basic faces is just 18.  Is 65 a good initial size for
that?

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Clément Pit-Claudel]

On 15/05/2020 15.05, Eli Zaretskii wrote:
>> Cc: 41200@debbugs.gnu.org, Juri Linkov <juri@linkov.net>
>> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
>> Date: Fri, 15 May 2020 14:50:17 -0400
>>
>>>>> No, I was asking why not start with it as nil and actually make a
>>>>> hash-table when we first need it.
>>>>
>>>> Oh, I thought it would be simpler to always have a hash table instead of having to sanity check every time.
>>>
>>> What is the default size of a hash-table?
>>
>> 65 entries, I think.
> 
> The number of basic faces is just 18.  Is 65 a good initial size for
> that?

I think that map stores more than just the basic faces: in fact, $ emacs -Q --batch --eval '(print (length (frame-face-alist)))' prints 100, so maybe I should even make the map larger by default?

Clément.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Eli Zaretskii]

> Cc: 41200@debbugs.gnu.org, juri@linkov.net
> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Fri, 15 May 2020 15:23:00 -0400
> 
> >>> What is the default size of a hash-table?
> >>
> >> 65 entries, I think.
> > 
> > The number of basic faces is just 18.  Is 65 a good initial size for
> > that?
> 
> I think that map stores more than just the basic faces: in fact, $ emacs -Q --batch --eval '(print (length (frame-face-alist)))' prints 100, so maybe I should even make the map larger by default?

We are miscommunicating.  I was talking about the size before loadup,
i.e. in temacs when it starts up.  Later, when we load preloaded
packages, the size should grow, but the hash-table takes care of that
by itself, doesn't it?

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Clément Pit-Claudel]

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

On 15/05/2020 15.38, Eli Zaretskii wrote:
>> Cc: 41200@debbugs.gnu.org, juri@linkov.net
>> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
>> Date: Fri, 15 May 2020 15:23:00 -0400
>>
>>>>> What is the default size of a hash-table?
>>>>
>>>> 65 entries, I think.
>>>
>>> The number of basic faces is just 18.  Is 65 a good initial size for
>>> that?
>>
>> I think that map stores more than just the basic faces: in fact, $ emacs -Q --batch --eval '(print (length (frame-face-alist)))' prints 100, so maybe I should even make the map larger by default?
> 
> We are miscommunicating.  I was talking about the size before loadup,
> i.e. in temacs when it starts up.

Ah, yes.  I think I mas mixing up the face_hash_table of each frame and Vface_new_frame_defaults.  Attached is a patch that makes Vface_new_frame_defaults 33-entries large.  Should I also change the default size for face_hash_table in struct frame?

> Later, when we load preloaded packages, the size should grow, but the
> hash-table takes care of that by itself, doesn't it?
Yes, definitely.


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

From 0d28f687046c721c4734aa57d8ae1ba5f23dd5b3 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

* src/frame.h (struct frame): Add face_hash_table, remove face_alist.
(fset_face_hash_table): New function.
(fset_face_alist): Remove.
* src/frame.c (make_frame): Initialize f->face_hash_table.
(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.
(Fframe_face_hash_table): New function.
(Fframe_face_alist): Move to faces.el as frame-face-alist.
(syms_of_xfaces): Add frame_face_hash_table.

* lisp/emacs-lisp/edebug.el (edebug-eval-defun):
* lisp/progmodes/elisp-mode.el (elisp--eval-defun-1):
* lisp/frame.el (frame-set-background-mode): Update to work with hash
tables instead of alists.
* lisp/faces.el (face-new-frame-defaults): Mark obsolete.
(face-list): Update to use face--new-frame-defaults.
(frame-face-alist): Moved here from src/xfaces.c.
---
 lisp/custom.el               |  2 +-
 lisp/emacs-lisp/edebug.el    |  3 +-
 lisp/faces.el                | 23 ++++++++--
 lisp/frame.el                |  2 +-
 lisp/progmodes/elisp-mode.el |  3 +-
 src/frame.c                  | 20 ++++++---
 src/frame.h                  |  8 ++--
 src/xfaces.c                 | 83 +++++++++++++++++++-----------------
 8 files changed, 84 insertions(+), 60 deletions(-)

diff --git a/lisp/custom.el b/lisp/custom.el
index 885c486c5e..3ac8bac40f 100644
--- a/lisp/custom.el
+++ b/lisp/custom.el
@@ -898,7 +898,7 @@ custom-push-theme
 	  ;; the value to a fake theme, `changed'.  If the theme is
 	  ;; later disabled, we use this to bring back the old value.
 	  ;;
-	  ;; For faces, we just use `face-new-frame-defaults' to
+	  ;; For faces, we just use `face--new-frame-defaults' to
 	  ;; recompute when the theme is disabled.
 	  (when (and (eq prop 'theme-value)
 		     (boundp symbol))
diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 78461185d3..76a75c04df 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..bb51797a38 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -176,10 +176,27 @@ face-font-registry-alternatives
 ;;; Creation, copying.
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
+(make-obsolete-variable
+ 'face-new-frame-defaults
+ "use `face--new-frame-defaults' or `face-alist' instead."
+ "28.1")
+
+(defun frame-face-alist (&optional frame)
+  "Return an alist of frame-local faces defined on FRAME.
+This alist is a copy of the contents of `frame--face-hash-table'.
+For internal use only."
+  (declare (obsolete frame--face-hash-table "28.1"))
+  (let ((faces))
+    (maphash (lambda (face spec) (push `(,(car spec) ,face . ,(cdr spec)) faces))
+             (frame--face-hash-table frame))
+    (mapcar #'cdr (sort faces (lambda (f1 f2) (< (car f1) (car f2)))))))
 
 (defun face-list ()
   "Return a list of all defined faces."
-  (mapcar #'car face-new-frame-defaults))
+  (let ((faces))
+    (maphash (lambda (face spec) (push `(,(car spec) . ,face) faces))
+             face--new-frame-defaults)
+    (mapcar #'cdr (sort faces (lambda (f1 f2) (< (car f1) (car f2)))))))
 
 (defun make-face (face)
   "Define a new face with name FACE, a symbol.
@@ -2097,7 +2114,7 @@ x-create-frame-with-faces
 (defun face-set-after-frame-default (frame &optional parameters)
   "Initialize the frame-local faces of FRAME.
 Calculate the face definitions using the face specs, custom theme
-settings, X resources, and `face-new-frame-defaults'.
+settings, X resources, and `face--new-frame-defaults'.
 Finally, apply any relevant face attributes found amongst the
 frame parameters in PARAMETERS."
   ;; The `reverse' is so that `default' goes first.
@@ -2106,7 +2123,7 @@ face-set-after-frame-default
 	(progn
 	  ;; Initialize faces from face spec and custom theme.
 	  (face-spec-recalc face frame)
-	  ;; Apply attributes specified by face-new-frame-defaults
+	  ;; Apply attributes specified by face--new-frame-defaults
 	  (internal-merge-in-global-face face frame))
       ;; Don't let invalid specs prevent frame creation.
       (error nil)))
diff --git a/lisp/frame.el b/lisp/frame.el
index 6c2f774709..66b5c6aa8d 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-table))
                          (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..75054b8818 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..0843d53559 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -946,6 +946,10 @@ make_frame (bool mini_p)
   rw->total_lines = mini_p ? 9 : 10;
   rw->pixel_height = rw->total_lines * FRAME_LINE_HEIGHT (f);
 
+  fset_face_hash_table
+    (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 +1258,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 +1340,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_table (f, Fcopy_hash_table (sf->face_hash_table));
+  /* Simple copy_hash_table isn't enough, because we need the contents of
+     the vectors which are the values in face_hash_table 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_table);
+  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..9761253085 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_table;
 
   /* 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_table (struct frame *f, Lisp_Object val)
 {
-  f->face_alist = val;
+  f->face_hash_table = val;
 }
 #if defined (HAVE_WINDOW_SYSTEM)
 INLINE void
diff --git a/src/xfaces.c b/src/xfaces.c
index 7d7aff95c1..3344558a85 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -95,9 +95,10 @@ Copyright (C) 1993-1994, 1998-2020 Free Software Foundation, Inc.
    with the symbol `face' in slot 0, and a slot for each of the face
    attributes mentioned above.
 
-   There is also a global face alist `Vface_new_frame_defaults'.  Face
-   definitions from this list are used to initialize faces of newly
-   created frames.
+   There is also a global face map `Vface_new_frame_defaults',
+   containing conses of (FACE_ID . FACE_DEFINITION).  Face definitions
+   from this table are used to initialize faces of newly created
+   frames.
 
    A face doesn't have to specify all attributes.  Those not specified
    have a value of `unspecified'.  Faces specifying all attributes but
@@ -1843,13 +1844,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_table, Qnil);
   else
-    lface = assq_no_quit (face_name, Vface_new_frame_defaults);
+    lface = CDR (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);
@@ -2734,11 +2733,6 @@ DEFUN ("internal-make-lisp-face", Finternal_make_lisp_face,
   /* Add a global definition if there is none.  */
   if (NILP (global_lface))
     {
-      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);
-
       /* 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.
 	 The mapping from Lisp face to Lisp face id is given by the
@@ -2748,9 +2742,14 @@ DEFUN ("internal-make-lisp-face", Finternal_make_lisp_face,
 	  xpalloc (lface_id_to_name, &lface_id_to_name_size, 1, MAX_FACE_ID,
 		   sizeof *lface_id_to_name);
 
+      Lisp_Object face_id = make_fixnum (next_lface_id);
       lface_id_to_name[next_lface_id] = face;
-      Fput (face, Qface, make_fixnum (next_lface_id));
+      Fput (face, Qface, face_id);
       ++next_lface_id;
+
+      global_lface = make_vector (LFACE_VECTOR_SIZE, Qunspecified);
+      ASET (global_lface, 0, Qface);
+      Fputhash (face, Fcons (face_id, global_lface), Vface_new_frame_defaults);
     }
   else if (f == NULL)
     for (i = 1; i < LFACE_VECTOR_SIZE; ++i)
@@ -2763,7 +2762,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_table);
 	}
       else
 	for (i = 1; i < LFACE_VECTOR_SIZE; ++i)
@@ -2924,7 +2923,7 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
       f = NULL;
       lface = lface_from_face_name (NULL, face, true);
 
-      /* When updating face-new-frame-defaults, we put :ignore-defface
+      /* When updating face--new-frame-defaults, we put :ignore-defface
 	 where the caller wants `unspecified'.  This forces the frame
 	 defaults to ignore the defface value.  Otherwise, the defface
 	 will take effect, which is generally not what is intended.
@@ -3508,7 +3507,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_table)) == 0)
     return;
 
   if (EQ (param, Qforeground_color))
@@ -4174,14 +4173,13 @@ 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,
+DEFUN ("frame--face-hash-table", Fframe_face_hash_table, Sframe_face_hash_table,
        0, 1, 0,
-       doc: /* Return an alist of frame-local faces defined on FRAME.
+       doc: /* Return a hash table 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_table;
 }
 
 
@@ -6678,30 +6676,32 @@ 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 = CAR (HASH_VALUE (table, idx));
+          if (FIXNATP (face_id)) {
+              int id = XFIXNAT (face_id);
+              eassert (id >= 0);
+              lface_id_to_name[id] = lface;
+            }
 	}
     }
   face_attr_sym[0] = Qface;
@@ -6855,7 +6855,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_table);
   defsubr (&Sdisplay_supports_face_attributes_p);
   defsubr (&Scolor_distance);
   defsubr (&Sinternal_set_font_selection_order);
@@ -6879,9 +6879,12 @@ syms_of_xfaces (void)
 the "specifity" of a face specification and should be let-bound
 only for this purpose.  */);
 
-  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;
+  DEFVAR_LISP ("face--new-frame-defaults", Vface_new_frame_defaults,
+    doc: /* Hash table of global face definitions (for internal use only.)  */);
+  Vface_new_frame_defaults =
+    /* 33 entries is enough to fit all basic faces */
+    make_hash_table (hashtest_eq, 33, 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

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Juri Linkov]

>>>> I think the problem is that tab-line is declared a basic face, but its
>>>> defface form is not in faces.el.
>>>
>>> Ah, good catch.  Current there's a defface for tab-bar in lisp/tab-bar, and since that's preloaded it works, but the defface for tab-line is in lisp/tab-line.el and so isn't preloaded.
>>> Should I move both to faces.el?
>>>
>> Yes, I think so.
>
> Thanks.  I will ask Juri to confirm before moving them, because I realize now that they have a custom group.
> Juri (CC'd; hi Juri!), was there a reason to make tab-bar and tab-line basic faces?  I see they are both in their own files and groups, instead of being in faces.el.

Actually, no reason other than consistency of faces belonging to the
same file where they are used.  But if it will fix the technical problem,
please move them to faces.el, especially given the fact that their
counterpart tool-bar face is already defined in faces.el.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Clément Pit-Claudel]

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

On 16/05/2020 19.03, Juri Linkov wrote:
>>>>> I think the problem is that tab-line is declared a basic face, but its
>>>>> defface form is not in faces.el.
>>>>
>>>> Ah, good catch.  Current there's a defface for tab-bar in lisp/tab-bar, and since that's preloaded it works, but the defface for tab-line is in lisp/tab-line.el and so isn't preloaded.
>>>> Should I move both to faces.el?
>>>>
>>> Yes, I think so.
>>
>> Thanks.  I will ask Juri to confirm before moving them, because I realize now that they have a custom group.
>> Juri (CC'd; hi Juri!), was there a reason to make tab-bar and tab-line basic faces?  I see they are both in their own files and groups, instead of being in faces.el.
> 
> Actually, no reason other than consistency of faces belonging to the
> same file where they are used.  But if it will fix the technical problem,
> please move them to faces.el, especially given the fact that their
> counterpart tool-bar face is already defined in faces.el.

Thanks a lot.  The attached patch does that.

[-- Attachment #2: 0001-Move-tab-bar-and-tab-line-to-faces.el-part-of-bug-41.patch --]
[-- Type: text/x-patch, Size: 2639 bytes --]

From 4d3349d83791a57cdc01374c82792fff6e1b8a94 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Pit-Claudel?= <clement.pitclaudel@live.com>
Date: Sat, 16 May 2020 19:36:43 -0400
Subject: [PATCH] Move tab-bar and tab-line to faces.el (part of bug#41200)

These are basic faces, so they need to be defined in
faces.el (otherwise (get 'tab-line 'face) returns 0).

* lisp/tab-bar.el (tab-bar):
* lisp/tab-line.el (tab-line): Move from here...
* lisp/faces.el (menu): ...to here.
---
 lisp/faces.el    | 27 +++++++++++++++++++++++++++
 lisp/tab-bar.el  | 13 -------------
 lisp/tab-line.el | 14 --------------
 3 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/lisp/faces.el b/lisp/faces.el
index bb51797a38..b495fb6e87 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -2755,6 +2755,33 @@ tool-bar
   :version "21.1"
   :group 'basic-faces)
 
+(defface tab-line
+  '((((class color) (min-colors 88))
+     :inherit variable-pitch
+     :height 0.9
+     :background "grey85"
+     :foreground "black")
+    (((class mono))
+     :background "grey")
+    (t
+     :inverse-video t))
+  "Tab line face."
+  :version "27.1"
+  :group 'basic-faces)
+
+(defface tab-bar
+  '((((class color) (min-colors 88))
+     :inherit variable-pitch
+     :background "grey85"
+     :foreground "black")
+    (((class mono))
+     :background "grey")
+    (t
+     :inverse-video t))
+  "Tab bar face."
+  :version "27.1"
+  :group 'basic-faces)
+
 (defface menu
   '((((type tty))
      :inverse-video t)
diff --git a/lisp/tab-bar.el b/lisp/tab-bar.el
index ce6d8c33dd..689481b28b 100644
--- a/lisp/tab-bar.el
+++ b/lisp/tab-bar.el
@@ -50,19 +50,6 @@ tab-bar-faces
   :group 'faces
   :version "27.1")
 
-(defface tab-bar
-  '((((class color) (min-colors 88))
-     :inherit variable-pitch
-     :background "grey85"
-     :foreground "black")
-    (((class mono))
-     :background "grey")
-    (t
-     :inverse-video t))
-  "Tab bar face."
-  :version "27.1"
-  :group 'tab-bar-faces)
-
 (defface tab-bar-tab
   '((default
       :inherit tab-bar)
diff --git a/lisp/tab-line.el b/lisp/tab-line.el
index 7a2bdc0b72..50ec40bacf 100644
--- a/lisp/tab-line.el
+++ b/lisp/tab-line.el
@@ -41,20 +41,6 @@ tab-line-faces
   :group 'faces
   :version "27.1")
 
-(defface tab-line
-  '((((class color) (min-colors 88))
-     :inherit variable-pitch
-     :height 0.9
-     :background "grey85"
-     :foreground "black")
-    (((class mono))
-     :background "grey")
-    (t
-     :inverse-video t))
-  "Tab line face."
-  :version "27.1"
-  :group 'tab-line-faces)
-
 (defface tab-line-tab
   '((default
       :inherit tab-line)
-- 
2.17.1

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Juri Linkov]

> +  :group 'basic-faces)
> +
> +  :group 'basic-faces)
>
> -  :group 'tab-bar-faces)
> -
> -  :group 'tab-line-faces)

Could these faces belong to both groups?

  :group 'basic-faces
  :group 'tab-bar-faces

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Clément Pit-Claudel]

On 17/05/2020 17.59, Juri Linkov wrote:
>> +  :group 'basic-faces)
>> +
>> +  :group 'basic-faces)
>>
>> -  :group 'tab-bar-faces)
>> -
>> -  :group 'tab-line-faces)
> 
> Could these faces belong to both groups?
> 
>   :group 'basic-faces
>   :group 'tab-bar-faces

Yup, but that won't work well if the face is customized before the group is defined, right? Maybe the trick would be to add the group to the face when tab-bar.el and tab-bar-line.el are loaded?

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Juri Linkov]

>>> +  :group 'basic-faces)
>>> +
>>> +  :group 'basic-faces)
>>>
>>> -  :group 'tab-bar-faces)
>>> -
>>> -  :group 'tab-line-faces)
>> 
>> Could these faces belong to both groups?
>> 
>>   :group 'basic-faces
>>   :group 'tab-bar-faces
>
> Yup, but that won't work well if the face is customized before the
> group is defined, right?  Maybe the trick would be to add the group to
> the face when tab-bar.el and tab-line.el are loaded?

Actually since tab-bar.el is pre-loaded this problem exists only for
tab-line.el.

Then adding something like

  (nconc (get 'tab-line-faces 'custom-group) '((tab-line custom-face) ...))

to tab-line.el will add faces when tab-line.el are loaded.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Juri Linkov]

>> >> Could these faces belong to both groups?
>> >> 
>> >>   :group 'basic-faces
>> >>   :group 'tab-bar-faces
>> >
>> > Yup, but that won't work well if the face is customized before the
>> > group is defined, right?  Maybe the trick would be to add the group to
>> > the face when tab-bar.el and tab-line.el are loaded?
>> 
>> Actually since tab-bar.el is pre-loaded this problem exists only for
>> tab-line.el.
>> 
>> Then adding something like
>> 
>>   (nconc (get 'tab-line-faces 'custom-group) '((tab-line custom-face) ...))
>> 
>> to tab-line.el will add faces when tab-line.el are loaded.
>
> Could you guys please finalize these issues?  I'd like to install
> these changes on master, for which I'd need a patch that covers both
> the changes in the face storage and the fix for the tab-line face.
>
> Actually, the problem with the tab-line face will probably need to be
> fixed on emacs-27.

I don't recommend using the trick with nconc in emacs-27.  It would be
less risky if Clément will just move tab faces to faces.el in emacs-27.

Or may I suggest to preload tab-line.el in emacs-27?
That will solve the problem as well.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: cpitclaudel@gmail.com,  41200@debbugs.gnu.org
> Date: Sun, 24 May 2020 01:47:28 +0300
> 
> > Actually, the problem with the tab-line face will probably need to be
> > fixed on emacs-27.
> 
> I don't recommend using the trick with nconc in emacs-27.  It would be
> less risky if Clément will just move tab faces to faces.el in emacs-27.

Agreed.

> Or may I suggest to preload tab-line.el in emacs-27?

Doesn't sound justified to me.  I don't expect this feature to be so
popular as to always have it available.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Juri Linkov]

>> Or may I suggest to preload tab-line.el in emacs-27?
>
> Doesn't sound justified to me.  I don't expect this feature to be so
> popular as to always have it available.

Agreed, better to move deffaces to faces.el.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Juri Linkov]

>> >>> +  :group 'basic-faces)
>> >>> +  :group 'basic-faces)
>> >>>
>> >>> -  :group 'tab-bar-faces)
>> >>> -  :group 'tab-line-faces)
>> >>
>> >> Could these faces belong to both groups?
>> >>
>> >>   :group 'basic-faces
>> >>   :group 'tab-bar-faces
>> >
>> > Yup, but that won't work well if the face is customized before the
>> > group is defined, right?  Maybe the trick would be to add the group to
>> > the face when tab-bar.el and tab-line.el are loaded?
>>
>> Actually since tab-bar.el is pre-loaded this problem exists only for
>> tab-line.el.
>>
>> Then adding something like
>>
>>   (nconc (get 'tab-line-faces 'custom-group) '((tab-line custom-face) ...))
>>
>> to tab-line.el will add faces when tab-line.el are loaded.
>
> Could you guys please finalize these issues?  I'd like to install
> these changes on master, for which I'd need a patch that covers both
> the changes in the face storage and the fix for the tab-line face.
>
> Actually, the problem with the tab-line face will probably need to be
> fixed on emacs-27.

I never used the second argument MEMBERS of 'defgroup', but it's exactly
what is needed.  Now the problem with the tab-line face is fixed on emacs-27
in commit 6eb18a950d.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: cpitclaudel@gmail.com,  41200@debbugs.gnu.org
> Date: Mon, 08 Jun 2020 03:21:58 +0300
> 
> > Could you guys please finalize these issues?  I'd like to install
> > these changes on master, for which I'd need a patch that covers both
> > the changes in the face storage and the fix for the tab-line face.
> >
> > Actually, the problem with the tab-line face will probably need to be
> > fixed on emacs-27.
> 
> I never used the second argument MEMBERS of 'defgroup', but it's exactly
> what is needed.  Now the problem with the tab-line face is fixed on emacs-27
> in commit 6eb18a950d.

Thanks.

Clément, would you like to rebase your patch on the current master and
resubmit?  I think we are ready for installing it.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Clément Pit-Claudel]

On 20/06/2020 03.47, Eli Zaretskii wrote:
>> From: Juri Linkov <juri@linkov.net>
>> Cc: cpitclaudel@gmail.com,  41200@debbugs.gnu.org
>> Date: Mon, 08 Jun 2020 03:21:58 +0300
>>
>>> Could you guys please finalize these issues?  I'd like to install
>>> these changes on master, for which I'd need a patch that covers both
>>> the changes in the face storage and the fix for the tab-line face.
>>>
>>> Actually, the problem with the tab-line face will probably need to be
>>> fixed on emacs-27.
>>
>> I never used the second argument MEMBERS of 'defgroup', but it's exactly
>> what is needed.  Now the problem with the tab-line face is fixed on emacs-27
>> in commit 6eb18a950d.
> 
> Thanks.
> 
> Clément, would you like to rebase your patch on the current master and
> resubmit?  I think we are ready for installing it.

Yes, sorry for the delay.  I will try to do this today.  Thanks for your patience.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Eli Zaretskii]

> Cc: 41200@debbugs.gnu.org
> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Sat, 20 Jun 2020 12:55:18 -0400
> 
> > Clément, would you like to rebase your patch on the current master and
> > resubmit?  I think we are ready for installing it.
> 
> Yes, sorry for the delay.  I will try to do this today.  Thanks for your patience.

Ping!

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Benson Chu]

Hey all,

I've been using this patch for 3 months now, and it makes my Emacs much
snappier! However, every time I want to update master, I have to rebase
this patch up. Is there anything preventing this changeset from landing
on master?

Is there anything I can do to help?

Also, I think there's a bug in the latest patch on line 1308 of
elisp-mode.el. The arguments for the call to remhash are reversed.

Thanks!
Benson

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 41200@debbugs.gnu.org
>> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
>> Date: Sat, 20 Jun 2020 12:55:18 -0400
>> 
>> > Clément, would you like to rebase your patch on the current master and
>> > resubmit?  I think we are ready for installing it.
>> 
>> Yes, sorry for the delay.  I will try to do this today.  Thanks for your patience.
>
> Ping!

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Stefan Monnier]

> 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.

Oh, this is great, makes a very noticeable difference.

I'm hoping it can make it into Emacs-27.1, tho it is probably rather
late for such a significant change.

> * 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?

The variable's name did not say "alist", so I don't see a need to change
it from that point of view.  But I think it deserves a "--" since it's
supposed to be internal.

A quick grep revealed:

    elpa/packages/context-coloring/fixtures/benchmark/faces.el:  (mapcar #'car face-new-frame-defaults))

That doesn't seem very serious, but I haven't grep'd the MELPA packages.
I saw no such comparable use of `frame-face-alist` in the wild.

> * 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?

Yes: it's not a hash, it's a table.

I think the better names refer to the "conceptual" type rather than the
specific implementation type, so I prefer "map" or "table" to "hash",
"alist", "obarray", and whatnot.

> - command-execute                                                 454  47%
[...]
>            - face-set-after-frame-default                         387  40%
>             - face-spec-recalc                                    374  39%
>              - make-face-x-resource-internal                      296  30%
>               - set-face-attributes-from-resources                273  28%
>                - set-face-attribute-from-resource                 219  22%

[...]

> - command-execute                                                 768  80%
[...]
>                       - face-set-after-frame-default              674  70%
>                        - face-spec-recalc                         660  69%
>                         - face-spec-set-2                         350  36%
>                          - apply                                  348  36%
>                           - set-face-attribute                    342  35%
>                            - internal-set-lisp-face-attribute     342  35%
>                             - frame-set-background-mode           331  34%
>                              - face-spec-recalc                   284  29%
>                               - make-face-x-resource-internal     235  24%

Both of those profiles suggest that most of the time is still spent in
`face-spec-recalc`, so it would be worth trying harder to avoid calling
it or to speed it up somehow (presumably with some better memozing/caching).

The first profile also suggests that we spend too much time in
`make-face-x-resource-internal`.

What caught my eye in the second profile is the fact that we call
`face-spec-recalc` recursively.  I suspect that recursion is not desired.


        Stefan


PS: Maybe another way to speed this up is to do the `face-spec-recalc`
lazily (i.e. only when we encounter the face during redisplay), but
that's probably harder to do and not as good (since it will still be
slow in the case where the frame happens to display hundreds of faces).

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>, 41200@debbugs.gnu.org
> Date: Fri, 15 May 2020 10:03:44 -0400
> 
> I'm hoping it can make it into Emacs-27.1, tho it is probably rather
> late for such a significant change.

It's too late.  Such a significant change on such a low level is got
to cause some issues, especially as parts of it are
backward-incompatible.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Clément Pit-Claudel]

On 15/05/2020 10.03, Stefan Monnier 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.
> 
> Oh, this is great, makes a very noticeable difference.

Thanks for testing!

> The variable's name did not say "alist", so I don't see a need to change
> it from that point of view.  But I think it deserves a "--" since it's
> supposed to be internal.

Ah, that's a good point.  At least, renaming it will make it clear that something changed and make it easy to support older and newer emacsen.

> A quick grep revealed:
> 
>     elpa/packages/context-coloring/fixtures/benchmark/faces.el:  (mapcar #'car face-new-frame-defaults))

AFAICT, this is just a copy of faces.el, used to benchmark syntax highlighting (that is, this code is not run).

>> - command-execute                                                 454  47%
> [...]
>>            - face-set-after-frame-default                         387  40%
>>             - face-spec-recalc                                    374  39%
>>              - make-face-x-resource-internal                      296  30%
>>               - set-face-attributes-from-resources                273  28%
>>                - set-face-attribute-from-resource                 219  22%
> 
> [...]
> 
>> - command-execute                                                 768  80%
> [...]
>>                       - face-set-after-frame-default              674  70%
>>                        - face-spec-recalc                         660  69%
>>                         - face-spec-set-2                         350  36%
>>                          - apply                                  348  36%
>>                           - set-face-attribute                    342  35%
>>                            - internal-set-lisp-face-attribute     342  35%
>>                             - frame-set-background-mode           331  34%
>>                              - face-spec-recalc                   284  29%
>>                               - make-face-x-resource-internal     235  24%
> 
> Both of those profiles suggest that most of the time is still spent in
> `face-spec-recalc`, so it would be worth trying harder to avoid calling
> it or to speed it up somehow (presumably with some better memozing/caching).

Interesting.  I also wonder whether we could fast-track the case where the face spec is a vector full of 'undefined, since that seems to be the common case.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Stefan Monnier]

> Interesting.  I also wonder whether we could fast-track the case where the
> face spec is a vector full of 'undefined, since that seems to be the
> common case.

I was thinking also of the case where no face specs have changed and the
frame is "normal" (same frame properties as others, basically) so we
could re-use the faces from another frame.

[ After all, my Emacs sessions typically have dozens of frames, and only
  one of them (a minibuffer-only frame) has faces setup differently
  because it's configured to use a slightly larger font.  ]


        Stefan

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[martin rudalics]

 > I was thinking also of the case where no face specs have changed and the
 > frame is "normal" (same frame properties as others, basically) so we
 > could re-use the faces from another frame.

That's my hope as well.  For years now I'm making frames temporarily
invisible in order to avoid setting up faces for a new frame.

 > [ After all, my Emacs sessions typically have dozens of frames, and only
 >    one of them (a minibuffer-only frame) has faces setup differently
 >    because it's configured to use a slightly larger font.  ]

In my setup the minibuffer-only frame doesn't even do that.

martin

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Jashank Jeremy]

[-- Attachment #1.1: Type: text/plain, Size: 520 bytes --]

Hello,

I have updated this patch to make it compile and behave on Emacs master
at 1d93540371aadec8f877bd781267d38d411c40a0 (current a few hours ago).
The updated patch is attached.

I queried Clément, who has ok'd my shepherding this patch along.

CPU profile data I've gathered suggests this change has almost entirely
stamped out the face-related hot-spots I'd seen previously.  Hooray!

Cheers,
    ~jashank

-- 
Jashank Jeremy
WWW	jashankj.space
pgp	FE4D6AA8 3736591C FAD30D97 DCE96CF5 8D931E04


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

From 79586b3ea1100a0f8ca50be8b4dd62684558408e 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
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/frame.h (struct frame): Add face_hash_table, remove face_alist.
(fset_face_hash_table): New function.
(fset_face_alist): Remove.
* src/frame.c (make_frame): Initialize f->face_hash_table.
(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.
(Fframe_face_hash_table): New function.
(Fframe_face_alist): Move to faces.el as frame-face-alist.
(syms_of_xfaces): Add frame_face_hash_table.

* lisp/progmodes/elisp-mode.el (elisp--eval-defun-1):
* lisp/frame.el (frame-set-background-mode): Update to work with hash
tables instead of alists.
* lisp/faces.el (face-new-frame-defaults): Mark obsolete.
(face-list): Update to use face--new-frame-defaults.
(frame-face-alist): Moved here from src/xfaces.c.
(x-create-frame-with-faces): Update to handle subtle semantic change
to how frame faces propagate, which otherwise breaks frame creation
with reverse video enabled.

;; Patch history:
;;
;; V1 (2020-05-12 22:41:24 -0400), (Bug#41200) message 29.
;;   Authored-by: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
;;
;; V2 (2020-05-13 11:13:27 -0400), (Bug#41200) message 38.
;;   Fixes compilation error.
;;   Reported-by: martin rudalics <rudalics <at> gmx.at>
;;   Authored-by: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
;;
;; V3 (2020-05-15 10:59:36 -0400), (Bug#41200) message 55.
;;   Amendments to resolve issues raised or resolved by EliZ.
;;   * Code-style cleanup.
;;   * Rename `face_hash' to `face_map'.
;;   Suggested-by: Eli Zaretskii <eliz <at> gnu.org>
;;   Authored-by: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
;;
;; V4 (2020-05-15 14:50:17 -0400), (Bug#41200) message 70.
;;   * Settle on name `face_hash_table'.
;;   * Rename `face-new-frame-defaults' to `face--new-frame-defaults'.
;;   * Allow multiple variants of a face to exist.
;;   Suggested-by: Eli Zaretskii <eliz <at> gnu.org>
;;   Authored-by: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
;;
;; V5 (2020-05-15 14:50:17 -0400), (Bug#41200) message 85.
;;   Lower the size of `face--new-frame-defaults'.
;;   Suggested-by: Eli Zaretskii <eliz <at> gnu.org>
;;   Authored-by: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
;;
;; V6 (2021-04-05 17:10:55 +1000).
;;   Rebase onto master; move hunks around.
;;   * Changes to `edebug.el' dropped.
;;   Authored-by: Jashank Jeremy <jashank <at> rulingia.com.au>
;;
;; V7 (2021-04-06 12:31:55 +1000).
;;   Fix `frame-face-alist' to cope with changes from V4.
;;   Authored-by: Jashank Jeremy <jashank <at> rulingia.com.au>
;;
;; V8 (2021-04-06 13:13:33 +1000).
;;   There is a subtle change to face propagation semantics at face
;;   creation time, which stops face frame creation working at all
;;   only if reverse video is enabled.
;;   Authored-by: Jashank Jeremy <jashank <at> rulingia.com.au>
;;
;; V9 (2021-04-06 16:17:53 +1000).
;;   Correct argument order of `remhash' in `elisp-mode.el'.
;;   Reported-by: Benson Chu <bensonchu457 <at> gmail.com>
;;   Authored-by: Jashank Jeremy <jashank <at> rulingia.com.au>
---
 lisp/custom.el               |  2 +-
 lisp/faces.el                | 27 ++++++++++--
 lisp/frame.el                |  2 +-
 lisp/progmodes/elisp-mode.el |  3 +-
 src/frame.c                  | 20 ++++++---
 src/frame.h                  |  8 ++--
 src/xfaces.c                 | 83 +++++++++++++++++++-----------------
 7 files changed, 86 insertions(+), 59 deletions(-)

diff --git a/lisp/custom.el b/lisp/custom.el
index 85e5d65ffb..5b071ef311 100644
--- a/lisp/custom.el
+++ b/lisp/custom.el
@@ -906,7 +906,7 @@ custom-push-theme
 	  ;; the value to a fake theme, `changed'.  If the theme is
 	  ;; later disabled, we use this to bring back the old value.
 	  ;;
-	  ;; For faces, we just use `face-new-frame-defaults' to
+	  ;; For faces, we just use `face--new-frame-defaults' to
 	  ;; recompute when the theme is disabled.
 	  (when (and (eq prop 'theme-value)
 		     (boundp symbol))
diff --git a/lisp/faces.el b/lisp/faces.el
index 42f4cddfb1..2127b90c86 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -176,10 +176,29 @@ face-font-registry-alternatives
 ;;; Creation, copying.
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
+(make-obsolete-variable
+ 'face-new-frame-defaults
+ "use `face--new-frame-defaults' or `face-alist' instead."
+ "28.1")
+
+(defun frame-face-alist (&optional frame)
+  "Return an alist of frame-local faces defined on FRAME.
+This alist is a copy of the contents of `frame--face-hash-table'.
+For internal use only."
+  (declare (obsolete frame--face-hash-table "28.1"))
+  (let ((faces))
+    (maphash (lambda (face spec)
+               (let ((face-id  (car (gethash face face--new-frame-defaults))))
+                 (push `(,face-id ,face . ,spec) faces)))
+             (frame--face-hash-table frame))
+    (mapcar #'cdr (sort faces (lambda (f1 f2) (< (car f1) (car f2)))))))
 
 (defun face-list ()
   "Return a list of all defined faces."
-  (mapcar #'car face-new-frame-defaults))
+  (let ((faces))
+    (maphash (lambda (face spec) (push `(,(car spec) . ,face) faces))
+             face--new-frame-defaults)
+    (mapcar #'cdr (sort faces (lambda (f1 f2) (< (car f1) (car f2)))))))
 
 (defun make-face (face)
   "Define a new face with name FACE, a symbol.
@@ -2115,9 +2134,9 @@ x-create-frame-with-faces
     (unwind-protect
 	(progn
 	  (x-setup-function-keys frame)
+	  (face-set-after-frame-default frame parameters)
 	  (x-handle-reverse-video frame parameters)
 	  (frame-set-background-mode frame t)
-	  (face-set-after-frame-default frame parameters)
 	  (if (null visibility-spec)
 	      (make-frame-visible frame)
 	    (modify-frame-parameters frame (list visibility-spec)))
@@ -2129,7 +2148,7 @@ x-create-frame-with-faces
 (defun face-set-after-frame-default (frame &optional parameters)
   "Initialize the frame-local faces of FRAME.
 Calculate the face definitions using the face specs, custom theme
-settings, X resources, and `face-new-frame-defaults'.
+settings, X resources, and `face--new-frame-defaults'.
 Finally, apply any relevant face attributes found amongst the
 frame parameters in PARAMETERS."
   ;; The `reverse' is so that `default' goes first.
@@ -2138,7 +2157,7 @@ face-set-after-frame-default
 	(progn
 	  ;; Initialize faces from face spec and custom theme.
 	  (face-spec-recalc face frame)
-	  ;; Apply attributes specified by face-new-frame-defaults
+	  ;; Apply attributes specified by face--new-frame-defaults
 	  (internal-merge-in-global-face face frame))
       ;; Don't let invalid specs prevent frame creation.
       (error nil)))
diff --git a/lisp/frame.el b/lisp/frame.el
index 2b6e4a60b8..0d78e2fb39 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -1247,7 +1247,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 frame))
+                         (gethash face (frame--face-hash-table))
                          (face-spec-match-p face
                                             (face-user-default-spec face)
                                             frame)))
diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index 8ade718640..aa5801a7ca 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -1309,8 +1309,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-symbol face--new-frame-defaults)
 	   (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 bbdc3b5599..c8f341b829 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -946,6 +946,10 @@ make_frame (bool mini_p)
   rw->total_lines = FRAME_LINES (f) - (mini_p ? 1 : 0);
   rw->pixel_height = rw->total_lines * FRAME_LINE_HEIGHT (f);
 
+  fset_face_hash_table
+    (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 +1258,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 +1340,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_table (f, Fcopy_hash_table (sf->face_hash_table));
+  /* Simple copy_hash_table isn't enough, because we need the contents of
+     the vectors which are the values in face_hash_table 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_table);
+  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 0fd95e4dd3..1c297921f7 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_table;
 
   /* 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.
@@ -665,9 +665,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_table (struct frame *f, Lisp_Object val)
 {
-  f->face_alist = val;
+  f->face_hash_table = val;
 }
 #if defined (HAVE_WINDOW_SYSTEM)
 INLINE void
diff --git a/src/xfaces.c b/src/xfaces.c
index ab4440f46a..d86adef62c 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -95,9 +95,10 @@ Copyright (C) 1993-1994, 1998-2021 Free Software Foundation, Inc.
    with the symbol `face' in slot 0, and a slot for each of the face
    attributes mentioned above.
 
-   There is also a global face alist `Vface_new_frame_defaults'.  Face
-   definitions from this list are used to initialize faces of newly
-   created frames.
+   There is also a global face map `Vface_new_frame_defaults',
+   containing conses of (FACE_ID . FACE_DEFINITION).  Face definitions
+   from this table are used to initialize faces of newly created
+   frames.
 
    A face doesn't have to specify all attributes.  Those not specified
    have a value of `unspecified'.  Faces specifying all attributes but
@@ -1962,13 +1963,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_table, Qnil);
   else
-    lface = assq_no_quit (face_name, Vface_new_frame_defaults);
+    lface = CDR (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);
@@ -2867,11 +2866,6 @@ DEFUN ("internal-make-lisp-face", Finternal_make_lisp_face,
   /* Add a global definition if there is none.  */
   if (NILP (global_lface))
     {
-      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);
-
       /* 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.
 	 The mapping from Lisp face to Lisp face id is given by the
@@ -2881,9 +2875,14 @@ DEFUN ("internal-make-lisp-face", Finternal_make_lisp_face,
 	  xpalloc (lface_id_to_name, &lface_id_to_name_size, 1, MAX_FACE_ID,
 		   sizeof *lface_id_to_name);
 
+      Lisp_Object face_id = make_fixnum (next_lface_id);
       lface_id_to_name[next_lface_id] = face;
-      Fput (face, Qface, make_fixnum (next_lface_id));
+      Fput (face, Qface, face_id);
       ++next_lface_id;
+
+      global_lface = make_vector (LFACE_VECTOR_SIZE, Qunspecified);
+      ASET (global_lface, 0, Qface);
+      Fputhash (face, Fcons (face_id, global_lface), Vface_new_frame_defaults);
     }
   else if (f == NULL)
     for (i = 1; i < LFACE_VECTOR_SIZE; ++i)
@@ -2896,7 +2895,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_table);
 	}
       else
 	for (i = 1; i < LFACE_VECTOR_SIZE; ++i)
@@ -3057,7 +3056,7 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
       f = NULL;
       lface = lface_from_face_name (NULL, face, true);
 
-      /* When updating face-new-frame-defaults, we put :ignore-defface
+      /* When updating face--new-frame-defaults, we put :ignore-defface
 	 where the caller wants `unspecified'.  This forces the frame
 	 defaults to ignore the defface value.  Otherwise, the defface
 	 will take effect, which is generally not what is intended.
@@ -3642,7 +3641,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_table)) == 0)
     return;
 
   if (EQ (param, Qforeground_color))
@@ -4308,14 +4307,13 @@ 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,
+DEFUN ("frame--face-hash-table", Fframe_face_hash_table, Sframe_face_hash_table,
        0, 1, 0,
-       doc: /* Return an alist of frame-local faces defined on FRAME.
+       doc: /* Return a hash table 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_table;
 }
 
 
@@ -6832,30 +6830,32 @@ 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 = CAR (HASH_VALUE (table, idx));
+          if (FIXNATP (face_id)) {
+              int id = XFIXNAT (face_id);
+              eassert (id >= 0);
+              lface_id_to_name[id] = lface;
+            }
 	}
     }
   face_attr_sym[0] = Qface;
@@ -7011,7 +7011,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_table);
   defsubr (&Sdisplay_supports_face_attributes_p);
   defsubr (&Scolor_distance);
   defsubr (&Sinternal_set_font_selection_order);
@@ -7035,9 +7035,12 @@ syms_of_xfaces (void)
 the "specifity" of a face specification and should be let-bound
 only for this purpose.  */);
 
-  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;
+  DEFVAR_LISP ("face--new-frame-defaults", Vface_new_frame_defaults,
+    doc: /* Hash table of global face definitions (for internal use only.)  */);
+  Vface_new_frame_defaults =
+    /* 33 entries is enough to fit all basic faces */
+    make_hash_table (hashtest_eq, 33, 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.31.1


[-- Attachment #2: OpenPGP Digital Signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Eli Zaretskii]

> Date: Tue, 06 Apr 2021 16:35:28 +1000
> From: Jashank Jeremy <jashank@rulingia.com.au>
> Cc: Clément Pit-Claudel <cpitclaudel@gmail.com>
> 
> I have updated this patch to make it compile and behave on Emacs master
> at 1d93540371aadec8f877bd781267d38d411c40a0 (current a few hours ago).
> The updated patch is attached.
> 
> I queried Clément, who has ok'd my shepherding this patch along.
> 
> CPU profile data I've gathered suggests this change has almost entirely
> stamped out the face-related hot-spots I'd seen previously.  Hooray!

Yeah, it's a pity Clément dropped the ball on this one.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Clément Pit-Claudel]

On 4/6/21 8:30 AM, Eli Zaretskii wrote:
>> Date: Tue, 06 Apr 2021 16:35:28 +1000
>> From: Jashank Jeremy <jashank@rulingia.com.au>
>> Cc: Clément Pit-Claudel <cpitclaudel@gmail.com>
>>
>> I have updated this patch to make it compile and behave on Emacs master
>> at 1d93540371aadec8f877bd781267d38d411c40a0 (current a few hours ago).
>> The updated patch is attached.
>>
>> I queried Clément, who has ok'd my shepherding this patch along.
>>
>> CPU profile data I've gathered suggests this change has almost entirely
>> stamped out the face-related hot-spots I'd seen previously.  Hooray!
> 
> Yeah, it's a pity Clément dropped the ball on this one.

Sorry Eli, life got in the way.  I'm glad that Jashank is taking care of it :)

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Eli Zaretskii]

> Cc: 41200@debbugs.gnu.org
> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Tue, 6 Apr 2021 11:07:00 -0400
> 
> Sorry Eli, life got in the way.  I'm glad that Jashank is taking care of it :)

No need to apologize, we all have that problem sometimes.

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Stefan Monnier]

> I have updated this patch to make it compile and behave on Emacs master
> at 1d93540371aadec8f877bd781267d38d411c40a0 (current a few hours ago).
> The updated patch is attached.

I just tried it on Emacs's `master` but for me it results in

    src/emacs -Q

opening up with most faces that don't use colors: the *scratch* buffer's
comments are displayed in black+bold+italic, the mode-line has
a black background.  A call of the form `(error "foo")` has foo display
in black+italic and the `error symbol is displayed in reverse video
(black background).

OTOH, the `M-x` that is displayed when I hit `M-x` seems to have the
usual blueish color.


        Stefan

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I just tried it on Emacs's `master` but for me it results in
>
>     src/emacs -Q
>
> opening up with most faces that don't use colors: the *scratch* buffer's
> comments are displayed in black+bold+italic, the mode-line has
> a black background.  A call of the form `(error "foo")` has foo display
> in black+italic and the `error symbol is displayed in reverse video
> (black background).

I tried the patch, too, and I'm seeing the same -- most faces are
different than they used to be, which is surely not what's supposed to
happen.

Jashank, have to continued working on this patch?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Jashank Jeremy]

[-- Attachment #1.1: Type: text/plain, Size: 1382 bytes --]

At 2021-04-22 23:56:20 -0400, Stefan Monnier wrote:
> [...] most faces that don't use colors: the *scratch* buffer's
> comments are displayed in black+bold+italic, the mode-line has a black
> background.  A call of the form `(error "foo")` has foo display in
> black+italic and the `error symbol is displayed in reverse video
> (black background).

At 2021-05-12 22:29:47 +0200, Lars Ingebrigtsen wrote:
> I tried the patch, too, and I'm seeing the same -- most faces are
> different than they used to be, which is surely not what's supposed to
> happen.

But another experiment you could try: create a new frame.  And another.

OK, I'll spoil the surprise --- this misbehaviour occurs *only* on the
initial frame.  I don't precisely know _why_ that happens, but I do know
how it broke: I tried (and thought I had succeeded) fixing a bug that
occurred only when reverse video was enabled, which would stop creation
of new frames entirely.

> Jashank, have to continued working on this patch?

Yes --- I have a patched patch which fixes that behaviour.

The 10th revision of that patch is attached: I have been running it for
a few weeks atop 7c901d90e620b4d3651b86c13faf1e81eeb3db10 (master at the
time), so I can tell you it also works with native-compile.

I have rebased onto ec574a72f7198d9793b466f33382fff397ac4ce1 (master as
of now) and will test that.

Cheers,
    ~jashank


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

From 04b8f2d131824fa9dc2bece19f4a719f977e4f4c 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
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/frame.h (struct frame): Add face_hash_table, remove face_alist.
(fset_face_hash_table): New function.
(fset_face_alist): Remove.
* src/frame.c (make_frame): Initialize f->face_hash_table.
(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.
(Fframe_face_hash_table): New function.
(Fframe_face_alist): Move to faces.el as frame-face-alist.
(syms_of_xfaces): Add frame_face_hash_table.

* lisp/progmodes/elisp-mode.el (elisp--eval-defun-1):
* lisp/frame.el (frame-set-background-mode): Update to work with hash
tables instead of alists.
* lisp/faces.el (face-new-frame-defaults): Mark obsolete.
(face-list): Update to use face--new-frame-defaults.
(frame-face-alist): Moved here from src/xfaces.c.
(x-create-frame-with-faces): Update to handle subtle semantic change
to how frame faces propagate, which otherwise breaks frame creation
with reverse video enabled.

;; Patch history:
;;
;; V1 (2020-05-12 22:41:24 -0400), (Bug#41200) message 29.
;;   Authored-by: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
;;
;; V2 (2020-05-13 11:13:27 -0400), (Bug#41200) message 38.
;;   Fixes compilation error.
;;   Reported-by: martin rudalics <rudalics <at> gmx.at>
;;   Authored-by: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
;;
;; V3 (2020-05-15 10:59:36 -0400), (Bug#41200) message 55.
;;   Amendments to resolve issues raised or resolved by EliZ.
;;   * Code-style cleanup.
;;   * Rename `face_hash' to `face_map'.
;;   Suggested-by: Eli Zaretskii <eliz <at> gnu.org>
;;   Authored-by: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
;;
;; V4 (2020-05-15 14:50:17 -0400), (Bug#41200) message 70.
;;   * Settle on name `face_hash_table'.
;;   * Rename `face-new-frame-defaults' to `face--new-frame-defaults'.
;;   * Allow multiple variants of a face to exist.
;;   Suggested-by: Eli Zaretskii <eliz <at> gnu.org>
;;   Authored-by: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
;;
;; V5 (2020-05-15 14:50:17 -0400), (Bug#41200) message 85.
;;   Lower the size of `face--new-frame-defaults'.
;;   Suggested-by: Eli Zaretskii <eliz <at> gnu.org>
;;   Authored-by: Clément Pit-Claudel <clement.pitclaudel <at> live.com>
;;
;; V6 (2021-04-05 17:10:55 +1000).
;;   Rebase onto master; move hunks around.
;;   * Changes to `edebug.el' dropped.
;;   Authored-by: Jashank Jeremy <jashank <at> rulingia.com.au>
;;
;; V7 (2021-04-06 12:31:55 +1000).
;;   Fix `frame-face-alist' to cope with changes from V4.
;;   Authored-by: Jashank Jeremy <jashank <at> rulingia.com.au>
;;
;; V8 (2021-04-06 13:13:33 +1000).
;;   There is a subtle change to face propagation semantics at face
;;   creation time, which stops face frame creation working at all
;;   only if reverse video is enabled.
;;   Authored-by: Jashank Jeremy <jashank <at> rulingia.com.au>
;;
;; V9 (2021-04-06 16:17:53 +1000).
;;   Correct argument order of `remhash' in `elisp-mode.el'.
;;   Reported-by: Benson Chu <bensonchu457 <at> gmail.com>
;;   Authored-by: Jashank Jeremy <jashank <at> rulingia.com.au>
;;
;; V10 (2021-04-30 12:11:19 +1200).
;;   Fix broken faces on initial frame (from prior reverse video fix).
;;   Reported-by: Stefan Monnier <monnier <at> iro.umontreal.ca>
;;   Authored-by: Jashank Jeremy <jashank <at> rulingia.com.au>
---
 lisp/custom.el               |  2 +-
 lisp/faces.el                | 27 ++++++++++--
 lisp/frame.el                |  2 +-
 lisp/progmodes/elisp-mode.el |  3 +-
 src/frame.c                  | 20 ++++++---
 src/frame.h                  |  8 ++--
 src/xfaces.c                 | 83 +++++++++++++++++++-----------------
 7 files changed, 87 insertions(+), 58 deletions(-)

diff --git a/lisp/custom.el b/lisp/custom.el
index 078e3a8cf8..feac99d11b 100644
--- a/lisp/custom.el
+++ b/lisp/custom.el
@@ -926,7 +926,7 @@ custom-push-theme
 	  ;; the value to a fake theme, `changed'.  If the theme is
 	  ;; later disabled, we use this to bring back the old value.
 	  ;;
-	  ;; For faces, we just use `face-new-frame-defaults' to
+	  ;; For faces, we just use `face--new-frame-defaults' to
 	  ;; recompute when the theme is disabled.
 	  (when (and (eq prop 'theme-value)
 		     (boundp symbol))
diff --git a/lisp/faces.el b/lisp/faces.el
index 9969140f0c..bf24a1d7da 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -176,10 +176,29 @@ face-font-registry-alternatives
 ;;; Creation, copying.
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
+(make-obsolete-variable
+ 'face-new-frame-defaults
+ "use `face--new-frame-defaults' or `face-alist' instead."
+ "28.1")
+
+(defun frame-face-alist (&optional frame)
+  "Return an alist of frame-local faces defined on FRAME.
+This alist is a copy of the contents of `frame--face-hash-table'.
+For internal use only."
+  (declare (obsolete frame--face-hash-table "28.1"))
+  (let ((faces))
+    (maphash (lambda (face spec)
+               (let ((face-id  (car (gethash face face--new-frame-defaults))))
+                 (push `(,face-id ,face . ,spec) faces)))
+             (frame--face-hash-table frame))
+    (mapcar #'cdr (sort faces (lambda (f1 f2) (< (car f1) (car f2)))))))
 
 (defun face-list ()
   "Return a list of all defined faces."
-  (mapcar #'car face-new-frame-defaults))
+  (let ((faces))
+    (maphash (lambda (face spec) (push `(,(car spec) . ,face) faces))
+             face--new-frame-defaults)
+    (mapcar #'cdr (sort faces (lambda (f1 f2) (< (car f1) (car f2)))))))
 
 (defun make-face (face)
   "Define a new face with name FACE, a symbol.
@@ -2115,6 +2134,8 @@ x-create-frame-with-faces
     (unwind-protect
 	(progn
 	  (x-setup-function-keys frame)
+	  (dolist (face (nreverse (face-list)))
+	    (face-spec-recalc face frame))
 	  (x-handle-reverse-video frame parameters)
 	  (frame-set-background-mode frame t)
 	  (face-set-after-frame-default frame parameters)
@@ -2145,7 +2166,7 @@ x-create-frame-with-faces
 (defun face-set-after-frame-default (frame &optional parameters)
   "Initialize the frame-local faces of FRAME.
 Calculate the face definitions using the face specs, custom theme
-settings, X resources, and `face-new-frame-defaults'.
+settings, X resources, and `face--new-frame-defaults'.
 Finally, apply any relevant face attributes found amongst the
 frame parameters in PARAMETERS."
   ;; The `reverse' is so that `default' goes first.
@@ -2154,7 +2175,7 @@ face-set-after-frame-default
 	(progn
 	  ;; Initialize faces from face spec and custom theme.
 	  (face-spec-recalc face frame)
-	  ;; Apply attributes specified by face-new-frame-defaults
+	  ;; Apply attributes specified by face--new-frame-defaults
 	  (internal-merge-in-global-face face frame))
       ;; Don't let invalid specs prevent frame creation.
       (error nil)))
diff --git a/lisp/frame.el b/lisp/frame.el
index aff1d479ec..94e0cf3c6a 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -1231,7 +1231,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 frame))
+                         (gethash face (frame--face-hash-table))
                          (face-spec-match-p face
                                             (face-user-default-spec face)
                                             frame)))
diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index a56c7093e7..7ed2d3d08c 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -1325,8 +1325,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-symbol face--new-frame-defaults)
 	   (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 e3d65dd28f..1b7a712518 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -1017,6 +1017,10 @@ make_frame (bool mini_p)
   rw->total_lines = FRAME_LINES (f) - (mini_p ? 1 : 0);
   rw->pixel_height = rw->total_lines * FRAME_LINE_HEIGHT (f);
 
+  fset_face_hash_table
+    (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;
@@ -1325,7 +1329,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
@@ -1407,14 +1411,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_table (f, Fcopy_hash_table (sf->face_hash_table));
+  /* Simple copy_hash_table isn't enough, because we need the contents of
+     the vectors which are the values in face_hash_table 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_table);
+  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 75a0b184c1..e70f418dc9 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_table;
 
   /* 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.
@@ -667,9 +667,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_table (struct frame *f, Lisp_Object val)
 {
-  f->face_alist = val;
+  f->face_hash_table = val;
 }
 #if defined (HAVE_WINDOW_SYSTEM)
 INLINE void
diff --git a/src/xfaces.c b/src/xfaces.c
index ab4440f46a..d86adef62c 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -95,9 +95,10 @@ Copyright (C) 1993-1994, 1998-2021 Free Software Foundation, Inc.
    with the symbol `face' in slot 0, and a slot for each of the face
    attributes mentioned above.
 
-   There is also a global face alist `Vface_new_frame_defaults'.  Face
-   definitions from this list are used to initialize faces of newly
-   created frames.
+   There is also a global face map `Vface_new_frame_defaults',
+   containing conses of (FACE_ID . FACE_DEFINITION).  Face definitions
+   from this table are used to initialize faces of newly created
+   frames.
 
    A face doesn't have to specify all attributes.  Those not specified
    have a value of `unspecified'.  Faces specifying all attributes but
@@ -1962,13 +1963,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_table, Qnil);
   else
-    lface = assq_no_quit (face_name, Vface_new_frame_defaults);
+    lface = CDR (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);
@@ -2867,11 +2866,6 @@ DEFUN ("internal-make-lisp-face", Finternal_make_lisp_face,
   /* Add a global definition if there is none.  */
   if (NILP (global_lface))
     {
-      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);
-
       /* 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.
 	 The mapping from Lisp face to Lisp face id is given by the
@@ -2881,9 +2875,14 @@ DEFUN ("internal-make-lisp-face", Finternal_make_lisp_face,
 	  xpalloc (lface_id_to_name, &lface_id_to_name_size, 1, MAX_FACE_ID,
 		   sizeof *lface_id_to_name);
 
+      Lisp_Object face_id = make_fixnum (next_lface_id);
       lface_id_to_name[next_lface_id] = face;
-      Fput (face, Qface, make_fixnum (next_lface_id));
+      Fput (face, Qface, face_id);
       ++next_lface_id;
+
+      global_lface = make_vector (LFACE_VECTOR_SIZE, Qunspecified);
+      ASET (global_lface, 0, Qface);
+      Fputhash (face, Fcons (face_id, global_lface), Vface_new_frame_defaults);
     }
   else if (f == NULL)
     for (i = 1; i < LFACE_VECTOR_SIZE; ++i)
@@ -2896,7 +2895,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_table);
 	}
       else
 	for (i = 1; i < LFACE_VECTOR_SIZE; ++i)
@@ -3057,7 +3056,7 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
       f = NULL;
       lface = lface_from_face_name (NULL, face, true);
 
-      /* When updating face-new-frame-defaults, we put :ignore-defface
+      /* When updating face--new-frame-defaults, we put :ignore-defface
 	 where the caller wants `unspecified'.  This forces the frame
 	 defaults to ignore the defface value.  Otherwise, the defface
 	 will take effect, which is generally not what is intended.
@@ -3642,7 +3641,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_table)) == 0)
     return;
 
   if (EQ (param, Qforeground_color))
@@ -4308,14 +4307,13 @@ 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,
+DEFUN ("frame--face-hash-table", Fframe_face_hash_table, Sframe_face_hash_table,
        0, 1, 0,
-       doc: /* Return an alist of frame-local faces defined on FRAME.
+       doc: /* Return a hash table 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_table;
 }
 
 
@@ -6832,30 +6830,32 @@ 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 = CAR (HASH_VALUE (table, idx));
+          if (FIXNATP (face_id)) {
+              int id = XFIXNAT (face_id);
+              eassert (id >= 0);
+              lface_id_to_name[id] = lface;
+            }
 	}
     }
   face_attr_sym[0] = Qface;
@@ -7011,7 +7011,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_table);
   defsubr (&Sdisplay_supports_face_attributes_p);
   defsubr (&Scolor_distance);
   defsubr (&Sinternal_set_font_selection_order);
@@ -7035,9 +7035,12 @@ syms_of_xfaces (void)
 the "specifity" of a face specification and should be let-bound
 only for this purpose.  */);
 
-  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;
+  DEFVAR_LISP ("face--new-frame-defaults", Vface_new_frame_defaults,
+    doc: /* Hash table of global face definitions (for internal use only.)  */);
+  Vface_new_frame_defaults =
+    /* 33 entries is enough to fit all basic faces */
+    make_hash_table (hashtest_eq, 33, 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.31.1


[-- Attachment #2: OpenPGP Digital Signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Lars Ingebrigtsen]

Jashank Jeremy <jashank@rulingia.com.au> writes:

> The 10th revision of that patch is attached: I have been running it for
> a few weeks atop 7c901d90e620b4d3651b86c13faf1e81eeb3db10 (master at the
> time), so I can tell you it also works with native-compile.

I can confirm that this version fixes the face problems observed with
the previous version.

I don't see your copyright assignment papers on file -- is that in the
pipeline, or have you yet to start the assignment process?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Jashank Jeremy]

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

At 2021-05-13 11:15:29 +0200, Lars Ingebrigtsen wrote:
> Jashank Jeremy <jashank@rulingia.com.au> writes:
>> The 10th revision of that patch is attached: I have been running it for
>> a few weeks atop 7c901d90e620b4d3651b86c13faf1e81eeb3db10 (master at the
>> time), so I can tell you it also works with native-compile.
>
> I can confirm that this version fixes the face problems observed with
> the previous version.

Excellent, thanks.

> I don't see your copyright assignment papers on file -- is that in the
> pipeline, or have you yet to start the assignment process?

Started ... but presently stalled due to the employer pipeline hazard.

    ~jashank

[-- Attachment #2: OpenPGP Digital Signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Lars Ingebrigtsen]

Jashank Jeremy <jashank@rulingia.com.au> writes:

>> I don't see your copyright assignment papers on file -- is that in the
>> pipeline, or have you yet to start the assignment process?
>
> Started ... but presently stalled due to the employer pipeline hazard.

This was a month ago -- has there been any further progress here?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > >> I don't see your copyright assignment papers on file -- is that in the
  > >> pipeline, or have you yet to start the assignment process?
  > >
  > > Started ... but presently stalled due to the employer pipeline hazard.

  > This was a month ago -- has there been any further progress here?

Talking with people at the employer, and with the FSF staff, and
encouraging them to talk with each other, could enable them to unblock
it.  Some companies' lawyers look at the issue in a very narrow way,
and the FSF staff could help them see a solution.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Aaron Jensen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Jashank Jeremy <jashank@rulingia.com.au> writes:
>
>>> I don't see your copyright assignment papers on file -- is that in the
>>> pipeline, or have you yet to start the assignment process?
>>
>> Started ... but presently stalled due to the employer pipeline hazard.
>
>This was a month ago -- has there been any further progress here?

Is there any chance that Clément could pick this up from his original
patch and avoid waiting for copyright assignment of the
corrected/up-to-date patch? I'd do it mysel if I were more familiar with
the code, but I'm not.

Aaron

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Lars Ingebrigtsen]

Jashank Jeremy <jashank@rulingia.com.au> writes:

> Yes --- I have a patched patch which fixes that behaviour.
>
> The 10th revision of that patch is attached: I have been running it for
> a few weeks atop 7c901d90e620b4d3651b86c13faf1e81eeb3db10 (master at the
> time), so I can tell you it also works with native-compile.
>
> I have rebased onto ec574a72f7198d9793b466f33382fff397ac4ce1 (master as
> of now) and will test that.

I see that the copyright assignment paperwork was finished a couple of
weeks ago, so I've now re-tested your patch, and I can't see any
glitches, so I've pushed it to Emacs 28.

Thanks!

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen [2021-07-21 16:02:12] wrote:
> I see that the copyright assignment paperwork was finished a couple of
> weeks ago, so I've now re-tested your patch, and I can't see any
> glitches, so I've pushed it to Emacs 28.

Yay!


        Stefan

bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined

[Clément Pit-Claudel]

On 7/21/21 10:28 AM, Stefan Monnier wrote:
> Lars Ingebrigtsen [2021-07-21 16:02:12] wrote:
>> I see that the copyright assignment paperwork was finished a couple of
>> weeks ago, so I've now re-tested your patch, and I can't see any
>> glitches, so I've pushed it to Emacs 28.

Thanks everyone!

Now to figure why tooltips are blinking so much lately ^^ (it's from before this patch)