Issue 5336: Remove downcasting methods from Grob_array and Grob_info

https://sourceforge.net/p/testlilyissues/issues/5336/

Presenting dynamic casts as simple getters was hiding something that
is better left in the open.

[Carl, 2018-06-11 22:45:12]

I realize this issue has already been pushed and closed.  But I have a question.

What is the harm of having the downcasting in the getter function?  An advantage
is that we can change the downcasting in one place if it is part of the member
functions for the class.  If not, we have to change it throughout the code base.
 Maintainability would seem to be a reason to keep it as it was.

I'm not asking for a revert.  But I would like to understand the reason it was
worth it for Dan to make this change.  Mostly for my own understanding of good
programming practices.

Thanks,

Carl

[Dan Eble, 2018-06-12 00:38:35]

My intent was to make it clear to the person using these objects what
is going on.

One thing that was hidden was the extra cost of getting an Item* over
getting a Grob*.  Consider this excerpt from the old version of
Dynamic_align_engraver:

    create_line_spanner (info.grob ());
    if (has_interface<Spanner> (info.grob ()))
      {
        started_.push_back (info.spanner ());
        current_dynamic_spanner_ = info.spanner ();
      }

Perhaps the person who wrote this code understood but didn't care that
he asked three times (at least: I haven't looked into
create_line_spanner) for a runtime check that info refers to a
Spanner.  I think it is more likely that he did not see or had
forgotten about the cast.  If he hadn't had info.spanner () available
to him, and had had to type out dynamic_cast<Spanner*> (info.grob ()),
he probably would have structured the code to do it once, not three
times.

To be clear, the cost of dynamic casting in lilypond is not huge; but
it is noticeable and it can be improved.  Last week I profiled one of
the rest regression tests that always shows differences and found that
1% of cycles are spent on dynamic casting.  I think that's 1% of score
processing, but it might be 1% of the total; I'm not sure because it
was my first time using the tool and I chose to move on to another
task before verifying which.

Point two:

    Ligature_bracket_engraver::acknowledge_note_column (Grob_info info)
    {
      if (ligature_)
        {
          Tuplet_bracket::add_column (ligature_,
                                      info.item ());
          add_bound_item (ligature_, info.item ());
        }
    }

If info refers to a Spanner, then this code passes a null pointer to
add_column and add_bound_item.  I'm not sure if that would be bad, but
let's assume it would.  If the person who coded this had had to write,

    Item *item = dynamic_cast<Info*> (info.grob ());
    ... (item);
    ... (item);

he would be more likely to notice that the pointer might be null and
handle it.

But now let's say that we know that it is impossible for
acknowledge_note_column to be passed a Grob_info referring to anything
but an Item.  (That might actually be the case, but I'm not sure.)
Well, that's checkmate for Grob_info::item (), because all that is
required to handle _that_ is static_cast, and _it_ doesn't have the
overhead of a function call and a dynamic_cast.

Along the same lines are things like the following.  I hope I can
describe this as "loose" without offending any of my worthy comrades
here:

    int
    Paper_column::get_rank (Grob const *me)
    {
      return dynamic_cast<Paper_column const *> (me)->rank_;
    }


If "me" is a valid Grob that is not a Paper_column, this dereferences
a null pointer, which is not something that any function should be
doing, especially not one with an interface that accepts a Grob.  A
function that will not work with anything but a Paper_column should
have Paper_column in its signature.

Well, my kids have come home!  I have to go, but I think that's about
all I have to write anyway.  I'll add more later if anything comes to
mind.

[Dan Eble, 2018-06-12 00:59:42]

To summarize, the principle I advocate is to establish early what kind of object
you are dealing with, and once you have paid for that information, retain it as
long as you can.  In LilyPond it seems I often see the opposite: casts in the
lowest depths of the code.

Regards,
Dan

[Dan Eble, 2018-06-12 21:37:55]

I remembered another thing about dynamic casting which is not specifically
relevant to the changes in this review, and which should be pretty well known,
but I'll record it here for the sake of completeness.

Wherever there is alternative processing predicated on type, e.g.

    if (A *a = dynamic_cast<A *> (info.grob ())) { ... }
    else if (B *b = dynamic_cast<B *> (info.grob ()) { ... }
    else if (C *c = dynamic_cast<C *> (info.grob ()) { ... }

the reader should stop and consider that perhaps defining a virtual method would
be better.  It could be the case that there is only one block that does
something and the others are omitted.

Again, this pattern is more easily recognized with the casts out in the open
than if they were obscured as, info.item(), info.spanner(), etc.

[Carl, 2018-06-12 22:18:41]

Dan,

Thanks for the feedback.  I appreciate it.

I'm still not convinced that pulling the dynamic casts out of the getter/setter
pair is better.

You talk about performance penalties of dynamic casting.  But your profiling
shows that dynamic casting took about 1% of the processing time.  Even if you
could reduce dynamic casting by 90% (which seems unlikely), you'd only reduce
execution time by 0.9%, which seems pretty insignificant.

The tradeoff of having people know about dynamic casting and using it properly
needs to be matched with people not needing to know about dynamic casting and
being able to ignore it.  It seems to me that unless there is a significant
mistake that is made by code that doesn't know about the dynamic cast, it's
better off to hide it.  In fact, it seems to me that it would be possible (and
maybe preferable) to put necessary error checking once in the setter/getter,
rather than having to recreate it multiple times throughout the code base.

If dynamic casting were taking up 50% of the process time, or errors in using
dynamic casting were responsible for large numbers of bugs, I might feel
different.  But to me, the benefits I understand from your explanation seem to
be not worth the cost of having dynamic casts show up in 21 different files.

As I said before, I'm not asking for a reversion.  I think I just have a
different tradeoff value model than you have.

Thanks for your explanation,

Carl

[dan_faithful.be, 2018-06-13 03:24:02]

On Jun 12, 2018, at 18:18, carl.d.sorensen@gmail.com wrote:
> 
> The tradeoff of having people know about dynamic casting and using it
> properly needs to be matched with people not needing to know about
> dynamic casting and being able to ignore it.

Carl, I appreciate hearing your perspective and I’d like to continue this
discussion to the point of reaching a consensus on the design.

I perceive that we understand each other’s points and simply disagree.  There is
nothing new I want to counter with.  I will just state that if a contributor
were made uncomfortable by dynamic_cast, my two-pronged solution would be (1)
gently encourage him to educate himself on this fundamental feature of C++, and
(2) over time, rework the software to require fewer casts by preserving more
type information in the internal interfaces and pushing the casts outward toward
the interface with Scheme.

> As I said before, I'm not asking for a reversion.  I think I just have a
> different tradeoff value model than you have.

Clearly, and I’m sure David can contribute a third perspective, and I hope he
will choose to.

This attempted clean-up and some other work in progress has been about as
engaging as pounding sand down a rat hole, and I have only persevered in it
because I considered it a step in the direction of a cleaner system in the long
run.  If it is valuable, then I would be willing to continue, but if that is not
the general perception, then I would rather take one step back, restore the code
to an acceptable state (probably not quite a straight reversion, though), and
never touch it again.

Thence, back to consensus: From what I’ve seen, the existing approach in many
(most?) cases is that any kind of grob can be passed to any function and if it’s
the right kind of grob, you get what you asked for, and if it’s the wrong kind
of grob, you get something else.  I can see that this is necessary at the
interface between Scheme and C++, but it doesn’t have to be carried through to
all the C++ internals, with each step working to determine the specific type
information it requires but neglecting to propagate it to the next step.  That
loose approach could be continued, but it doesn’t have to be.  Should it be
continued?

Regards,
— 
Dan

[dak, 2018-06-13 08:12:44]

On 2018/06/13 03:24:02, dan_faithful.be wrote:
> On Jun 12, 2018, at 18:18, mailto:carl.d.sorensen@gmail.com wrote:
> > 
> > The tradeoff of having people know about dynamic casting and using it
> > properly needs to be matched with people not needing to know about
> > dynamic casting and being able to ignore it.
> 
> Carl, I appreciate hearing your perspective and I’d like to continue this
> discussion to the point of reaching a consensus on the design.
> 
> I perceive that we understand each other’s points and simply disagree.  There
is
> nothing new I want to counter with.  I will just state that if a contributor
> were made uncomfortable by dynamic_cast, my two-pronged solution would be (1)
> gently encourage him to educate himself on this fundamental feature of C++,
and
> (2) over time, rework the software to require fewer casts by preserving more
> type information in the internal interfaces and pushing the casts outward
toward
> the interface with Scheme.
> 
> > As I said before, I'm not asking for a reversion.  I think I just have a
> > different tradeoff value model than you have.
> 
> Clearly, and I’m sure David can contribute a third perspective, and I hope he
> will choose to.

I've been hoping to avoid that.

> This attempted clean-up and some other work in progress has been about as
> engaging as pounding sand down a rat hole,

The only thing I've found to work yet are cats and poison.  Cats scale down the
visibility and scale of the problem.  Poison at least manages to keep the
problem in the house at bay because there you got enough of a rationale to
revert to second-generation poisons in-house when the damn critters have
literally consumed several kg of first-generation bait (which is reasonably
harmless regarding the effects on rat corpse feeders) with diminishing effect.

Either require a human bothered about dealing with the problem to a sufficient
degree to keep working on it.  Such engagement may start with pounding sand down
a rat hole.

> and I have only persevered in it
> because I considered it a step in the direction of a cleaner system in the
long
> run.  If it is valuable, then I would be willing to continue, but if that is
not
> the general perception,

The "general perception" is irrelevant as long as the general is not working on
the code.  However, this work includes reviews.  I don't see a future for
LilyPond when nobody is working on it, but the future without people bothering
to review it is not an inviting prospect, either.  It's too big for that to
work.

Dan's work here is trying to lower the amount of code lookups a typical C++
programmer and LilyPond newbie will have to deal with.  It hardcodes a LilyPond
code concept as a dynamic cast and little else makes sense (apart from possibly
adding an assertion).  I can follow his rationale that we'd want to stick in the
proper type domain in the first place and want to get there eventually.  At the
same time, it's a distinction completely irrelevant at the Scheme level where
things are dynamically typed.

I readily agree that the overall performance impact is small.  At the same point
of time, you have to declare your types in C++ all the time and overly generic
declarations, while individually causing only very little performance drain
mostly irrelevant to what the dynamic Scheme type system has to go through all
of the time, cause programmers wasting time on figuring out what the code is
talking about here.  It's bad C++ Feng Shui, distracting from more important
work.

I'll readily admit the effects on inner architecture for making a house
welcoming but am utterly untalented in that regard.  Thus I am very much content
with those who actually are bothered by details enough to propose changes and
argue for them to start clean up.  This change here is not actually changing
things as much as a bit of sorting allowing for throwing some things out later. 
So it's a bit pointless to argue for or against it on its own merits and/or make
a molehill of the sand to be pounded into a rat hole.  I sure hope that we'll
get to the rats themselves eventually when we have reduced the number of holes
they keep running out of.

> then I would rather take one step back, restore the code
> to an acceptable state (probably not quite a straight reversion, though), and
> never touch it again.

If the code were in acceptable state, we'd have more programmers and fewer
discussions.

> I can see that this is necessary at the
> interface between Scheme and C++, but it doesn’t have to be carried through to
> all the C++ internals, with each step working to determine the specific type
> information it requires but neglecting to propagate it to the next step.  That
> loose approach could be continued, but it doesn’t have to be.  Should it be
> continued?

I'd prefer to have code enjoyable to work with.  What this means is different at
the C++ and the Scheme level.  If you have to declare your types in a static
type lattice, it makes sense declaring them in the manner conveying their
meaning.  So I am fine with your longer perspective.  At the same time, this
review is about code changes in preparation of making the longer perspective a
more appealing and defined task to work on and so discussing its respective
merits is somewhat pointless and wasting time and energy we'd rather want spent
on actual improvements.

It's good that we have comfortable review structures in place suitable for
dealing with large-scale changes.  I am just afraid that current development
speed tends to makke for unproportional response.  Sort of an overblown
autoimmune response, a bit self-defeating.

Summary: I'd want Dan to go ahead.  Not as much based on the individual goals
achieved with this particular patch but his overall goal of making the interior
design of LilyPond more hospitable to visiting C++ programmers.  It's not just
the code but also the culture that wants adjustment in that respect.  This patch
and review engaged more energy than called for and that's a thing that wants
fixing as badly as the code.

[Carl, 2018-06-13 16:04:53]

I am convinced by these arguments.  

Thank you for your patience with me.

Hopefully we can get some rats taken care of.

Carl

[Carl, 2018-06-13 16:15:07]

On 2018/06/13 03:24:02, dan_faithful.be wrote:

> I perceive that we understand each other’s points and simply disagree.  There
is
> nothing new I want to counter with.  I will just state that if a contributor
> were made uncomfortable by dynamic_cast, my two-pronged solution would be (1)
> gently encourage him to educate himself on this fundamental feature of C++,
and
> (2) over time, rework the software to require fewer casts by preserving more
> type information in the internal interfaces and pushing the casts outward
toward
> the interface with Scheme.
> 

I now understand more about the overhead that is involved in the encapsulation
that I thought was desirable.  Rather than an execution overhead, there is a
coding overhead.  For every type of dynamic cast I may want to use, I need to
provide a getter method.  And this just covers up a dynamic cast; there's not
any reasonable error handling involved in the getter method.  That's not very
smart, I see now.

I wholeheartedly agree with your changes.  Thanks for running with this issue.

Carl