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

[Eli Zaretskii <eliz@gnu.org>]

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