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

["Clément Pit-Claudel" <cpitclaudel@gmail.com>]

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.