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