GNU bug report logs -
#41200
Displaying a tooltip with x-show-tip gets very slow as more faces are defined
Previous Next
Reported by: Clément Pit-Claudel <cpitclaudel <at> gmail.com>
Date: Tue, 12 May 2020 04:31:01 UTC
Severity: normal
Tags: moreinfo, patch
Merged with 41267
Found in version 26.3
Fixed in version 28.1
Done: Lars Ingebrigtsen <larsi <at> gnus.org>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 41200 in the body.
You can then email your comments to 41200 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Tue, 12 May 2020 04:31:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Clément Pit-Claudel <cpitclaudel <at> gmail.com>:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org.
(Tue, 12 May 2020 04:31:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
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 <at> 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 (text/plain, attachment)]
[tip.personal-config.prof (text/plain, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Tue, 12 May 2020 06:43:02 GMT)
Full text and
rfc822 format available.
Message #8 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Tue, 12 May 2020 11:31:01 GMT)
Full text and
rfc822 format available.
Message #11 received at 41200 <at> debbugs.gnu.org (full text, mbox):
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)))
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Tue, 12 May 2020 15:14:02 GMT)
Full text and
rfc822 format available.
Message #14 received at 41200 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
>> 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 (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Tue, 12 May 2020 15:29:02 GMT)
Full text and
rfc822 format available.
Message #17 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> Resent-From: Clément Pit-Claudel <cpitclaudel <at> gmail.com>
> Original-Sender: "Debbugs-submit" <debbugs-submit-bounces <at> debbugs.gnu.org>
> Resent-CC: bug-gnu-emacs <at> gnu.org
> Resent-Sender: help-debbugs <at> gnu.org
> From: Clément Pit-Claudel <cpitclaudel <at> 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?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Tue, 12 May 2020 17:20:02 GMT)
Full text and
rfc822 format available.
Message #20 received at 41200 <at> debbugs.gnu.org (full text, mbox):
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?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Tue, 12 May 2020 17:43:02 GMT)
Full text and
rfc822 format available.
Message #23 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Tue, 12 May 2020 17:59:02 GMT)
Full text and
rfc822 format available.
Message #26 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> From: martin rudalics <rudalics <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Wed, 13 May 2020 02:42:02 GMT)
Full text and
rfc822 format available.
Message #29 received at 41200 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
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 <at> 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 (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Wed, 13 May 2020 14:59:02 GMT)
Full text and
rfc822 format available.
Message #32 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> No, please go ahead.
Installed meanwhile.
Thanks, martin
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Wed, 13 May 2020 14:59:02 GMT)
Full text and
rfc822 format available.
Message #35 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Wed, 13 May 2020 15:14:01 GMT)
Full text and
rfc822 format available.
Message #38 received at 41200 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
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 (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Wed, 13 May 2020 17:43:02 GMT)
Full text and
rfc822 format available.
Message #41 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> 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
Merged 41200 41267.
Request was from
Eli Zaretskii <eliz <at> gnu.org>
to
control <at> debbugs.gnu.org.
(Thu, 14 May 2020 18:59:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Fri, 15 May 2020 11:06:01 GMT)
Full text and
rfc822 format available.
Message #46 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> Cc: 41200 <at> debbugs.gnu.org
> From: Clément Pit-Claudel <cpitclaudel <at> 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?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Fri, 15 May 2020 14:05:01 GMT)
Full text and
rfc822 format available.
Message #49 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> 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).
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Fri, 15 May 2020 14:35:01 GMT)
Full text and
rfc822 format available.
Message #52 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 41200 <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Fri, 15 May 2020 15:00:01 GMT)
Full text and
rfc822 format available.
Message #55 received at 41200 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
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?)
[0001-Store-frame-faces-in-hash-tables-instead-of-alists.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Fri, 15 May 2020 15:19:02 GMT)
Full text and
rfc822 format available.
Message #58 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> Cc: 41200 <at> debbugs.gnu.org
> From: Clément Pit-Claudel <cpitclaudel <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Fri, 15 May 2020 15:34:01 GMT)
Full text and
rfc822 format available.
Message #61 received at 41200 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Fri, 15 May 2020 16:24:02 GMT)
Full text and
rfc822 format available.
Message #64 received at 41200 <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Fri, 15 May 2020 17:29:01 GMT)
Full text and
rfc822 format available.
Message #67 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> Cc: 41200 <at> debbugs.gnu.org, Anders Lindgren <andlind <at> gmail.com>
> From: Clément Pit-Claudel <cpitclaudel <at> 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?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Fri, 15 May 2020 18:51:01 GMT)
Full text and
rfc822 format available.
Message #70 received at 41200 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 15/05/2020 13.28, Eli Zaretskii wrote:
>> Cc: 41200 <at> debbugs.gnu.org, Anders Lindgren <andlind <at> gmail.com>
>> From: Clément Pit-Claudel <cpitclaudel <at> 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.
[0001-Store-frame-faces-in-hash-tables-instead-of-alists.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Fri, 15 May 2020 19:07:01 GMT)
Full text and
rfc822 format available.
Message #73 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> Cc: 41200 <at> debbugs.gnu.org, Juri Linkov <juri <at> linkov.net>
> From: Clément Pit-Claudel <cpitclaudel <at> 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?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Fri, 15 May 2020 19:11:01 GMT)
Full text and
rfc822 format available.
Message #76 received at 41200 <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Fri, 15 May 2020 19:24:02 GMT)
Full text and
rfc822 format available.
Message #79 received at 41200 <at> debbugs.gnu.org (full text, mbox):
On 15/05/2020 15.05, Eli Zaretskii wrote:
>> Cc: 41200 <at> debbugs.gnu.org, Juri Linkov <juri <at> linkov.net>
>> From: Clément Pit-Claudel <cpitclaudel <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Fri, 15 May 2020 19:39:02 GMT)
Full text and
rfc822 format available.
Message #82 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> Cc: 41200 <at> debbugs.gnu.org, juri <at> linkov.net
> From: Clément Pit-Claudel <cpitclaudel <at> 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?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Fri, 15 May 2020 19:53:01 GMT)
Full text and
rfc822 format available.
Message #85 received at 41200 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 15/05/2020 15.38, Eli Zaretskii wrote:
>> Cc: 41200 <at> debbugs.gnu.org, juri <at> linkov.net
>> From: Clément Pit-Claudel <cpitclaudel <at> 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.
[0001-Store-frame-faces-in-hash-tables-instead-of-alists.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Fri, 15 May 2020 21:24:02 GMT)
Full text and
rfc822 format available.
Message #88 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Sat, 16 May 2020 08:46:02 GMT)
Full text and
rfc822 format available.
Message #91 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Sat, 16 May 2020 23:25:01 GMT)
Full text and
rfc822 format available.
Message #94 received at 41200 <at> debbugs.gnu.org (full text, mbox):
>>>> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Sat, 16 May 2020 23:44:01 GMT)
Full text and
rfc822 format available.
Message #97 received at 41200 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
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.
[0001-Move-tab-bar-and-tab-line-to-faces.el-part-of-bug-41.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Sun, 17 May 2020 22:15:02 GMT)
Full text and
rfc822 format available.
Message #100 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> + :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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Mon, 18 May 2020 01:20:02 GMT)
Full text and
rfc822 format available.
Message #103 received at 41200 <at> debbugs.gnu.org (full text, mbox):
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?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Tue, 19 May 2020 22:26:03 GMT)
Full text and
rfc822 format available.
Message #106 received at 41200 <at> debbugs.gnu.org (full text, mbox):
>>> + :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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Sat, 23 May 2020 08:12:02 GMT)
Full text and
rfc822 format available.
Message #109 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> From: Juri Linkov <juri <at> linkov.net>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 41200 <at> debbugs.gnu.org
> Date: Wed, 20 May 2020 00:48:55 +0300
>
> >>> + :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.
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Sat, 23 May 2020 22:57:02 GMT)
Full text and
rfc822 format available.
Message #112 received at 41200 <at> debbugs.gnu.org (full text, mbox):
>> >> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Sun, 24 May 2020 02:34:02 GMT)
Full text and
rfc822 format available.
Message #115 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> From: Juri Linkov <juri <at> linkov.net>
> Cc: cpitclaudel <at> gmail.com, 41200 <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Sun, 24 May 2020 22:11:02 GMT)
Full text and
rfc822 format available.
Message #118 received at 41200 <at> debbugs.gnu.org (full text, mbox):
>> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Mon, 08 Jun 2020 00:24:02 GMT)
Full text and
rfc822 format available.
Message #121 received at 41200 <at> debbugs.gnu.org (full text, mbox):
>> >>> + :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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Sat, 20 Jun 2020 07:48:01 GMT)
Full text and
rfc822 format available.
Message #124 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> From: Juri Linkov <juri <at> linkov.net>
> Cc: cpitclaudel <at> gmail.com, 41200 <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Sat, 20 Jun 2020 16:56:02 GMT)
Full text and
rfc822 format available.
Message #127 received at 41200 <at> debbugs.gnu.org (full text, mbox):
On 20/06/2020 03.47, Eli Zaretskii wrote:
>> From: Juri Linkov <juri <at> linkov.net>
>> Cc: cpitclaudel <at> gmail.com, 41200 <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Sat, 04 Jul 2020 07:59:02 GMT)
Full text and
rfc822 format available.
Message #130 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> Cc: 41200 <at> debbugs.gnu.org
> From: Clément Pit-Claudel <cpitclaudel <at> 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!
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Sun, 13 Sep 2020 03:31:02 GMT)
Full text and
rfc822 format available.
Message #133 received at 41200 <at> debbugs.gnu.org (full text, mbox):
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 <at> gnu.org> writes:
>> Cc: 41200 <at> debbugs.gnu.org
>> From: Clément Pit-Claudel <cpitclaudel <at> 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!
Added tag(s) patch.
Request was from
Lars Ingebrigtsen <larsi <at> gnus.org>
to
control <at> debbugs.gnu.org.
(Sun, 13 Sep 2020 13:29:01 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Tue, 06 Apr 2021 10:25:02 GMT)
Full text and
rfc822 format available.
Message #138 received at 41200 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
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
[0001-Store-frame-faces-in-hash-tables-instead-of-alists-V9.patch (text/x-patch, attachment)]
[Message part 3 (application/pgp-signature, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Tue, 06 Apr 2021 12:31:02 GMT)
Full text and
rfc822 format available.
Message #141 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> Date: Tue, 06 Apr 2021 16:35:28 +1000
> From: Jashank Jeremy <jashank <at> rulingia.com.au>
> Cc: Clément Pit-Claudel <cpitclaudel <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Tue, 06 Apr 2021 15:08:02 GMT)
Full text and
rfc822 format available.
Message #144 received at 41200 <at> debbugs.gnu.org (full text, mbox):
On 4/6/21 8:30 AM, Eli Zaretskii wrote:
>> Date: Tue, 06 Apr 2021 16:35:28 +1000
>> From: Jashank Jeremy <jashank <at> rulingia.com.au>
>> Cc: Clément Pit-Claudel <cpitclaudel <at> 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 :)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Tue, 06 Apr 2021 15:51:02 GMT)
Full text and
rfc822 format available.
Message #147 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> Cc: 41200 <at> debbugs.gnu.org
> From: Clément Pit-Claudel <cpitclaudel <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Fri, 23 Apr 2021 03:57:01 GMT)
Full text and
rfc822 format available.
Message #150 received at 41200 <at> debbugs.gnu.org (full text, mbox):
> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Wed, 12 May 2021 20:31:02 GMT)
Full text and
rfc822 format available.
Message #153 received at 41200 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> 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
Added tag(s) moreinfo.
Request was from
Lars Ingebrigtsen <larsi <at> gnus.org>
to
control <at> debbugs.gnu.org.
(Wed, 12 May 2021 20:31:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Thu, 13 May 2021 06:53:01 GMT)
Full text and
rfc822 format available.
Message #158 received at 41200 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
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
[0001-Store-frame-faces-in-hash-tables-instead-of-alists-V10.patch (text/x-patch, attachment)]
[Message part 3 (application/pgp-signature, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Thu, 13 May 2021 09:16:01 GMT)
Full text and
rfc822 format available.
Message #161 received at 41200 <at> debbugs.gnu.org (full text, mbox):
Jashank Jeremy <jashank <at> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Thu, 13 May 2021 23:28:02 GMT)
Full text and
rfc822 format available.
Message #164 received at 41200 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
At 2021-05-13 11:15:29 +0200, Lars Ingebrigtsen wrote:
> Jashank Jeremy <jashank <at> 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
[Message part 2 (application/pgp-signature, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Sat, 12 Jun 2021 12:16:02 GMT)
Full text and
rfc822 format available.
Message #167 received at 41200 <at> debbugs.gnu.org (full text, mbox):
Jashank Jeremy <jashank <at> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Sun, 13 Jun 2021 03:20:02 GMT)
Full text and
rfc822 format available.
Message #170 received at 41200 <at> debbugs.gnu.org (full text, mbox):
[[[ 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)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Tue, 06 Jul 2021 12:43:02 GMT)
Full text and
rfc822 format available.
Message #173 received at 41200 <at> debbugs.gnu.org (full text, mbox):
Lars Ingebrigtsen <larsi <at> gnus.org> writes:
> Jashank Jeremy <jashank <at> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Wed, 21 Jul 2021 14:03:01 GMT)
Full text and
rfc822 format available.
Message #176 received at 41200 <at> debbugs.gnu.org (full text, mbox):
Jashank Jeremy <jashank <at> 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 marked as fixed in version 28.1, send any further explanations to
41200 <at> debbugs.gnu.org and Clément Pit-Claudel <cpitclaudel <at> gmail.com>
Request was from
Lars Ingebrigtsen <larsi <at> gnus.org>
to
control <at> debbugs.gnu.org.
(Wed, 21 Jul 2021 14:03:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Wed, 21 Jul 2021 14:29:01 GMT)
Full text and
rfc822 format available.
Message #181 received at 41200 <at> debbugs.gnu.org (full text, mbox):
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
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#41200; Package
emacs.
(Wed, 21 Jul 2021 14:34:01 GMT)
Full text and
rfc822 format available.
Message #184 received at 41200 <at> debbugs.gnu.org (full text, mbox):
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)
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Thu, 19 Aug 2021 11:24:04 GMT)
Full text and
rfc822 format available.
This bug report was last modified 2 years and 245 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.