Re: Issue 5362: Generalize context searches (issue 344050043 by address@hidden)

[dak]

On 2018/07/01 14:09:28, Dan Eble wrote:
> (initial feedback—more later)

> https://codereview.appspot.com/344050043/diff/1/lily/global-context.cc
> File lily/global-context.cc (right):


https://codereview.appspot.com/344050043/diff/1/lily/global-context.cc#newcode143
> lily/global-context.cc:143: return score;
> On 2018/07/01 13:21:42, dak wrote:
> > So if there already is a Score context, it will be returned in lieu
of _any_
> > request for creating any kind of context?  That sounds weird.

> The missing information is that this method is called to create an
immediate
> child after acceptability has been verified, so it is only a Score.

Ugh.  The half-hardwired relation of Global and Score is sort of a
nuisance.  LilyPond does not really want to work with a different
setup but it's still spelled out in {engraver,performer}-init.ly as if
it could.  If someone messes with that, I'd prefer that LilyPond
breaks down in obvious places rather than all over the place.  I'll
readily admit that this isn't the case currently either, but it does
not exactly make review nicer

> I wouldn't call this any worse, but a comment would still be
> beneficial.

Yup.


https://codereview.appspot.com/344050043/diff/1/lily/include/context.hh
> File lily/include/context.hh (right):


https://codereview.appspot.com/344050043/diff/1/lily/include/context.hh#newcode61
> lily/include/context.hh:61: template <FindMode mode, Direction dir>
> On 2018/07/01 13:21:42, dak wrote:

> > Since the dir argument is actually being read from the command in
> > question rather than being fixed at compile time, you then need to
> > hardcode a farmout to the various variants since the actually
> > called variant cannot, after all, be determined at compile time in
> > general.

> You're focusing on the context-specced music iterator which has the
> runtime direction.  There are other callers using a hard-coded
> constant direction.

> This is something I vacillated about.  At first, I had 7 public
> methods (2 modes do something consistent but not really useful) with
> direction in the method name.  That has long been my preference over
> providing constant arguments.  Then I got to the context-specced
> music iterator and thought, maybe I should go the other way in this
> case, inline the switch(dir) and trust the compiler to optimize it
> in the cases that direction is known at compile time.

Premature optimization is the root of all evil.  I'm going to
pontificate now in the hope that you'll have some opportunity in your
work life to take karmatic revenge by passing this kind of "wisdom"
on.

C++ is a statically typed language.  It prides itself on allowing
basic types and data structures to be implemented in-language with an
efficiency near native implementations, to a good degree via its
template programming mechanisms which are totally alien to the
C language in syntax, to the degree that even the token >> requires
being viewed as two separate tokens in some situations.  The
associated cost to the human reader is horrific but there are
situations when this pays off.

Those situations are things like the Complex data type or the STL or
Boost++ libraries, stuff that the "ordinary" programmer never looks
into but "only" uses, stuff which is written once and used hundreds
and thousands of times all over in different systems.  One result is
that there _still_ isn't an efficient standard multidimensional array
type that numeric libraries would consistently use and that would
allow to painlessly write code with the same kind of aliasing
guarantees and possibilities of strength reduction that mathematicians
could work with using Fortran II in, what?, the 1950s?

So cranking out the template stuff is heavy-duty stuff.  There are
heavy-duty situations in the LilyPond code base where we use it,
either as mere users in the case of external libraries like STL, but
also ourselves and there mostly for marrying the static type system
and memory management of C++ to the dynamic type system and garbage
collection system of Guile.  This is stuff that is heavily used in
joints seminal to performance.  It's also very thoroughly used, to the
degree where we could swap out the type system for something different
and everything would follow.

It's unpleasantly dense code, but it keeps stuff in one place and
reasonably efficient.  That's a good tradeoff for using templates.
Trampolines are generated automagically using the template system:
that's a somewhat worse tradeoff but removes a solid bout of clutter,
clutter that would be easy to understand but still requires looking
at.

Now your use of templates possibly saves a few cycles, assuming that
branching out into 8 different versions does not cost more cycles due
to execution cache pollution than the savings are.  We are talking
about few cycles of savings in functions that for better or worse take
easily tens of thousands of cycles to execute.  That's pointless.  We
don't improve how the computer gets along with the code, so our focus
needs to be on how the human gets along with the code.

Template instantiation is a separate compiler pass with complex
semantics that a human needs to stay on top of.  Function arguments
are simple in comparison.

It's not really an equivalent choice.  In fact, the greatest
performance drain to be feared is having code that the average
programmer will be afraid to touch.  All other code will gradually be
refactored and improved over time and kept in a shape compatible with
the rest.  Scary code, in contrast, will not get improved.  The
associated one-time savings in the order of a few thousands of
percents (if at all) are offset against the cost of the code making
programmers less inclined to do further refactoring.  Which is
definite a larger percentage.

We have to keep what we think we are going to be able to achieve in
relation with how hard to maintain the code becomes to a programmer
being somewhat comparably versed in Scheme and C++ because only those
programmers will actually have the required view of the whole to work
effectively on the LilyPond code base.

That's a social metric of the code.  Keeping sight of that is a much
more fuzzy thing than figuring out whether some version runs 2 cycles
faster than another.  But even given some fuzziness, some comparisons
can be made.

As a rule of thumb you don't go wrong reducing complexity as long as
you do not increase _algorithmic_ complexity.  Reducing algorithmic
complexity does warrant refactoring, and one wants to do it in a
manner where the complexity is condensed into a comparatively small
and general and possibly separately maintainable piece of code, so
that the bulk of the code is again comparatively easy to understand.

That keeps the code in a shape where people can work effectively on it
and cause incremental changes that are not mere window dressing.

I mean, with regard to the LilyPond code base, this is reminiscent of
the joke of the guy taking a broiled chicken to the vet and asking
whether something can be done.  But we are not there yet.


https://codereview.appspot.com/344050043/