Re: Question about display engine

[Ergus]

On Mon, Sep 09, 2019 at 09:08:51PM +0300, Eli Zaretskii wrote:
> > Date: Mon, 9 Sep 2019 19:08:58 +0200
> > From: Ergus <address@hidden>
> > Cc: martin rudalics <address@hidden>, address@hidden
> >
> > 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.