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

[Stefan Monnier <monnier@iro.umontreal.ca>]

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