bug#59347: 29.0.50; `:family` face setting ignored

[Po Lu]

Gregory Heytings <gregory@heytings.org> writes:

> My measurements indicate that the "improved" code is ~20 slower in an
> optimized build, and I care about performance.  That's a lot to handle
> a variable that is, in fact, a near-constant.

And how many microseconds is ``~20x slower''? Do you actually see the
drop in performance?

You remind me of the people who painstakingly profile GL, and get angry
over a drop in performance from ``26450fps'' to ``9300fps'', when the
end result is still more than sufficient to match the display refresh.

With the following instrumentation:

diff --git a/src/xfaces.c b/src/xfaces.c
index 88d3a79f8c0..0eced7b7be5 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -6089,8 +6089,15 @@ realize_gui_face (struct face_cache *cache, Lisp_Object 
attrs[LFACE_VECTOR_SIZE]
        }
       if (! FONT_OBJECT_P (attrs[LFACE_FONT_INDEX]))
        {
+         unsigned long long t1, t2;
+         struct timespec timespec;
+
          spec = copy_font_spec (attrs[LFACE_FONT_INDEX]);
 
+         clock_gettime (CLOCK_THREAD_CPUTIME_ID, &timespec);
+         t1 = (timespec.tv_sec * 1000000
+               + timespec.tv_nsec / 1000);
+
          /* Unset several values in SPEC, usually the width, slant,
             and weight.  The best possible values for these
             attributes is determined in font_find_for_lface, called
@@ -6117,6 +6124,11 @@ realize_gui_face (struct face_cache *cache, Lisp_Object 
attrs[LFACE_VECTOR_SIZE]
          font_unset_attribute (spec, FONT_SPACING_INDEX, QCspacing);
          font_unset_attribute (spec, FONT_AVGWIDTH_INDEX, QCavgwidth);
 
+         clock_gettime (CLOCK_THREAD_CPUTIME_ID, &timespec);
+         t2 = (timespec.tv_sec * 1000000
+               + timespec.tv_nsec / 1000);
+         fprintf (stderr, "that was: %ull us\n", t2 - t1);
+
          attrs[LFACE_FONT_INDEX] = font_load_for_lface (f, attrs, spec);
        }
       if (FONT_OBJECT_P (attrs[LFACE_FONT_INDEX]))

all that code takes somewhere between 2 to 4 microseconds to complete
for me, in a build with optimizations enabled.  I don't know how many
faces you think people have to realize, or what kind of 1950s computer
you have in mind, but on any reasonably modern computer it will take
somewhere around 500,000 to 250,000 faces for this extra code to make a
single seconds' difference.

> Once again, that variable isn't supposed to be changed by users.  It
> is there for one purpose: if a user reports a bug about font
> selection, it will be possible to ask them to set that variable to
> this-and-that value (most probably 0 and most-positive-fixnum) to
> check whether Emacs behaves better.
>
> Of course, brave souls can also try to change it, if they want.

No matter whether or not a variable is supposed to be changed by the
user, it must not be a bitmask!  And in general, it must not be
confusing to users.  Users are supposed to debug Emacs.

> It is not "searching for a symbol through a list with three elements",
> it is 12 function calls, each of which traverses a list of three
> elements.

And how many microseconds is that?  How is it significant, compared to
the time it takes to send the glyphs in the resulting font off to the X
server and wait for a reply?  Or to allocate all the colors in the face?
Or to simply search through the color cache for all those colors?

> The post to which you are replying explains in every possible detail
> why that statement is wrong.  That you don't have the patience to read
> a 300 posts long bug thread is one thing, that you don't have the
> patience to read a 100 lines long explanation, and bluntly declare
> that it is "nonsense", is another.

Then feel free to write a better doc string, instead of outright
reverting the change.  The only requirement is that it must describe the
variable in terms that users can understand, even if it is somewhat
vague.

> And the purpose of that variable is precisely that: making it possible
> to dynamically control an implementation detail.  It is _not_ supposed
> to be set by users who do not know about the inner workings of the
> face code.

If users are not supposed to set the variable, then delete it
completely.  Otherwise, users will set it, and documentation they cannot
understand is just asking for trouble.