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

classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Clément Pit-Claudel
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))

tip.emacs-q.prof (4K) Download Attachment
tip.personal-config.prof (50K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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




Reply | Threaded
Open this post in threaded view
|

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

martin rudalics
 >> 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

tooltip-hide.diff (464 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Clément Pit-Claudel
> Resent-From: Clément Pit-Claudel <[hidden email]>
> Original-Sender: "Debbugs-submit" <[hidden email]>
> Resent-CC: [hidden email]
> Resent-Sender: [hidden email]
> From: Clément Pit-Claudel <[hidden email]>
> 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?



Reply | Threaded
Open this post in threaded view
|

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

Clément Pit-Claudel
In reply to this post by martin rudalics
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?





Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: martin rudalics <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Clément Pit-Claudel
In reply to this post by Eli Zaretskii
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.

0001-Store-frame-faces-in-hash-tables-instead-of-alists.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

martin rudalics
In reply to this post by Eli Zaretskii
> No, please go ahead.

Installed meanwhile.

Thanks, martin





Reply | Threaded
Open this post in threaded view
|

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

martin rudalics
In reply to this post by Clément Pit-Claudel
 > 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




Reply | Threaded
Open this post in threaded view
|

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

Clément Pit-Claudel
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.



0001-Store-frame-faces-in-hash-tables-instead-of-alists.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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




Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Clément Pit-Claudel
> Cc: [hidden email]
> From: Clément Pit-Claudel <[hidden email]>
> 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?



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> Cc: [hidden email]
> From: Clément Pit-Claudel <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|

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 13.28, Eli Zaretskii wrote:

>> Cc: [hidden email], Anders Lindgren <[hidden email]>
>> From: Clément Pit-Claudel <[hidden email]>
>> 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.


0001-Store-frame-faces-in-hash-tables-instead-of-alists.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> Cc: [hidden email], Juri Linkov <[hidden email]>
> From: Clément Pit-Claudel <[hidden email]>
> 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?



Reply | Threaded
Open this post in threaded view
|

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: [hidden email], Juri Linkov <[hidden email]>
>> From: Clément Pit-Claudel <[hidden email]>
>> 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.



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> Cc: [hidden email], [hidden email]
> From: Clément Pit-Claudel <[hidden email]>
> 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?



Reply | Threaded
Open this post in threaded view
|

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.38, Eli Zaretskii wrote:

>> Cc: [hidden email], [hidden email]
>> From: Clément Pit-Claudel <[hidden email]>
>> 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.


0001-Store-frame-faces-in-hash-tables-instead-of-alists.patch (16K) Download Attachment
12