Re: Question about display engine

[Ergus <spacibba@aol.com>]

On Mon, Sep 09, 2019 at 09:08:51PM +0300, Eli Zaretskii wrote:
>> Date: Mon, 9 Sep 2019 19:08:58 +0200
>> From: Ergus <spacibba@aol.com>
>> Cc: martin rudalics <rudalics@gmx.at>, emacs-devel@gnu.org
>>
>> But after working on that parts I recognize that the actual
>> algorithm for merge are extremely complicated and some parts are
>> redundant. All the code is full of conditions and branch divergences
>> and the design of faces infrastructure could be more efficient.
>
>I don't think I agree.  Given the complexity of the face-related
>functionalities -- face remapping, the need to recompute all the faces
>when some basic face changes, the various sources of face-related
>information for each buffer/string position, etc. -- I find the design
>and implementation of face code quite elegant and easy to understand,
>maintain and change.
>
I am talking about efficiency.

>> Code that is used very often like "PRODUCE_GLYPHS" checks 3 if conditions
>> every time (we should unify at least the if/else to bypass directly to
>> the pointer, asserting that it is always initialized in terminal or gui).
>
>I don't think I see the optimization opportunity you are talking about
>here.  If you want to make a produce_glyphs method available as a TTY
>hook, then why do you think it will make any tangible change of code
>efficiency?  It's just a dereference and a call through a function
>pointer vs a test and a direct call.
>
This won't be an important optimization.

It is just not standard. If there is no difference why are there
different methods to select with if/else? The macro simply looks like a
workaround.

In any case, if the it/frame/rif structs are standard; then they should
be always initialized. the tests will go away. And we could add an assert
to test only in debug code. 

>> face_at_buffer_position and merge_face_ref are actually full of nested
>> and nested conditional calls and even (direct and indirect) recursive
>> calls. Some functions that could call merge_named_face I think they call
>> merge_face_ref instead (with the extra checks and divergences). While
>> other "low level" calls at the end also call merge_face_ref (probably
>> for safety) ex get_lface_attributes, merge_face_vectors and so on.
>
>All of this is necessary to support the immense flexibility of face-related
>functionality we have, starting from the half a dozen different ways
>Lisp can specify a face.  There's nothing redundant in that code,
>AFAIK, and unlike you, I don't find it too complicated at all.
>
To add one field to the faces it required many modifications. Most of
them with very similar changes here and there; else if and similar
changes... every time I see an if-else-if with more than 3-4 conditions
I start wondering.

>> I understand that all this is probably negligible... but all that
>> inhibits many optimizations very important these days like code
>> inlining, branch prediction and vectorization.
>
>What can I say? code should be correct first, and fast only after
>that.  If the functionality we want to support disables some
>optimizations, so be it.  And I wouldn't worry about this particular
>part of display code anyway, because the most expensive parts of
>redisplay are elsewhere.  It should be clear from a simple
>consideration: the number of face changes in a typical window is an
>order of magnitude or more smaller than the number of characters and
>other display elements in that window.  So if we want to optimize
>redisplay, we should look at the 90% part of the code, not at the 10%.
>
Agree. But then why when the GC fails the lagging is so intense? I
thought that the main part of the display engine related with GC was the
faces part.

>> But also a big part of the code is needed just because the
>> divergence between the tui and the gui code...
>
>I don't think I follow.  There's no difference between TTY and GUI
>frames wrt face handling, except where TTY color translation is
>involved.
>
>> if we unify the interfaces; then an important part of the
>> code will be simplified and reduced and easier to maintain and modify.
>
>Which interfaces would you like to unify?  I don't think I understand.
>
Most of the code conditioned with: if (FRAME_WINDOW_P (f)) could be
simplified. But I understand that this could be a lot of work and I
don't know enough about this.