[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#41200: Displaying a tooltip with x-show-tip gets very slow as more f
From: |
Eli Zaretskii |
Subject: |
bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined |
Date: |
Fri, 15 May 2020 18:17:53 +0300 |
> 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.
- bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined, (continued)
bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined, Eli Zaretskii, 2020/05/12
bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined, Eli Zaretskii, 2020/05/15
bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined, Clément Pit-Claudel, 2020/05/15
bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined,
Eli Zaretskii <=
bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined, Noam Postavsky, 2020/05/15
bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined, Clément Pit-Claudel, 2020/05/15
bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined, Eli Zaretskii, 2020/05/15
bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined, Clément Pit-Claudel, 2020/05/15
bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined, Eli Zaretskii, 2020/05/15
bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined, Clément Pit-Claudel, 2020/05/15
bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined, Eli Zaretskii, 2020/05/15
bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined, Clément Pit-Claudel, 2020/05/15
bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined, Juri Linkov, 2020/05/16
bug#41200: Displaying a tooltip with x-show-tip gets very slow as more faces are defined, Clément Pit-Claudel, 2020/05/16