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

[Eli Zaretskii]

> Cc: 41200@debbugs.gnu.org
> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Tue, 12 May 2020 22:41:24 -0400
> 
> > So I think if we want to support such large amounts of faces, we
> > should not store them in alists, but in a more efficient data
> > structure.
> 
> Indeed, you're completely right; thanks!  Replacing face_alist and 
> Vface_new_frame_defaults with hash tables makes the worst example about 10 
> times faster, and with that change tooltips now take 30 to 50ms to display 
> instead of 500-600ms in my real-life use case (my usual config).  I have 
> attached a patch.
> 
> I left a few questions in the code; I hope that's OK.  I have a few more 
> questions that are not part of the code:
> 
> * I removed the function frame-face-alist and changed the type of the 
> variable face-new-frame-defaults.  Both were documented as internal.  Should 
> I add an ELisp implementation of frame-face-alist for compatibility?  (It 
> wouldn't be a perfect shim, since modifying its return value wouldn't do the 
> same).  For face-new-frame-defaults it's a bit trickier, since the variable 
> now holds a hash table.  Should I change its name to make the change obvious, 
> at least? 

I'd like to keep the old face-new-frame-defaults and frame-face-alist
for compatibility, but mention in the doc strings that they no longer
return modifiable values, and perhaps deprecate them.

> * The name face_hash isn't ideal, since there's already a distinct notion of 
> face hashes (hash codes).  Can you think of a better name? 

face_hash_table?

> * I imagine that this change needs to be advertised somewhere, but I'm not 
> sure where; NEWS?

Let's think about this after we figured out what changes are needed in
the current functions and variables.

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

I don't think so, but if the behavior is now linear or sub-linear,
it's the best we can expect, since creating a new frame must walk over
all the faces.

> +  // QUESTION: is this where this should be initialized?

Yes, I think so.  But do we need to do anything when frame is deleted
as well?

> +  fset_face_hash
> +    (f, make_hash_table(hashtest_eq, DEFAULT_HASH_SIZE, DEFAULT_REHASH_SIZE,
                         ^^
Our C coding conventions are to leave one space between the function's
name and the opening parenthesis (here and elsewhere in the patch).

> -DEFUN ("frame-face-alist", Fframe_face_alist, Sframe_face_alist,
> +// QUESTION: Should I add an ELisp version of frame-face-hash?

You mean, frame-face-alist, right?  Yes, most definitely: I imagine a
lot of code out there uses that, and we wouldn't want to break that.

And I'm not sure we should have it only in Lisp: perhaps we should
maintain the alist as well, and add/remove to/from it when a face is
added or removed in the hash table.  Otherwise this change of
internals will have painful effect on packages that use the current
APIs.

> +       Lisp_Object lface = HASH_KEY(table, idx);
> +          Lisp_Object face_id = Fget (lface, Qface);
> +          // FIXME why is (get 'tab-line 'face) 0?

A bug, I guess.

> +          if (!FIXNATP (face_id))
> +            // FIXME: I'm not sure what to do in this case

I'm not sure I understand why do you need to look at the existing
face's 'face' property?  The original code didn't.

>    DEFVAR_LISP ("face-new-frame-defaults", Vface_new_frame_defaults,
>      doc: /* List of global face definitions (for internal use only.)  */);
> -  Vface_new_frame_defaults = Qnil;
> +  Vface_new_frame_defaults =
> +    make_hash_table (hashtest_eq, DEFAULT_HASH_SIZE, DEFAULT_REHASH_SIZE,
> +                     DEFAULT_REHASH_THRESHOLD, Qnil, Qnil);

Why do we need to start with a non-default hash-table?