Issue 4365: Add ly_is_smob<T>(S) to check if S is a smob of type T.

Add ly_is_smob<T>(S) to check if S is a smob of type T.

Hide Smob_base<>::is_smob(S) to prevent the misleading situation of D::is_smob(S) returning true when S represents an ancestor of D rather than a D.  This issue was previously worked around by overriding is_smob() in several derived smob classes; that is no longer needed.

Hide Smob_base<>::smob_p() because there doesn't seem to be a need for it to be public.

[dak, 2015-05-02 08:25:09]

https://codereview.appspot.com/236850043/diff/1/lily/include/smobs.hh
File lily/include/smobs.hh (right):

https://codereview.appspot.com/236850043/diff/1/lily/include/smobs.hh#newcode235
lily/include/smobs.hh:235: template <class T>
Nope.  dynamic_cast is non-trivial operation that cannot be optimized away at
compile time for almost any use.  Calling it for every instance of the rather
ubiquitous is_smob query (ubiquitous enough that I made it an operation separate
from unsmob and checked that the generated code was nicer) is excessive.

And all that for avoiding less of a dozen instances of an open-coded
dynamic_cast with reasonably clear intent?  No.

Creating a separate function or template for that is one thing.  Forcing
everything else through it another.

[Dan Eble, 2015-05-02 12:54:18]

On 2015/05/02 08:25:09, dak wrote:

> Nope.  dynamic_cast is non-trivial operation that cannot be optimized away at
> compile time for almost any use.  Calling it for every instance of the rather
> ubiquitous is_smob query (ubiquitous enough that I made it an operation
separate
> from unsmob and checked that the generated code was nicer) is excessive.

When "Patchy the autobot says: passes tests" does that include any profiling?

Since you've been through the exercise of examining generated code, I'm curious
whether you found dynamic_cast<T*>(T*) (or any upcast) producing anything worse
than static_cast<T*>(T*) would have.  That would shock me; but if there is some
compiler version actually in use that is that oblivious, we should be able to
adapt a template metaprogramming trick from boost or C++11 that avoids the
dynamic_cast in those cases.

There might be some cases where ly_is_smob<Child>() is called where only
ly_is_smob<Parent>() is required (though this seems unlikely).  The right way to
improve performance in those cases is to ask the right question instead.

In the case of necessary down-casting, a test is required to prevent the
function from answering the wrong question.  There is evidence that this has
been recognized as a problem in some cases already: Engraver and other classes
have an overridden unsmob() that includes a dynamic cast and an overridden
is_smob() that depends on it.  Are there other cases waiting to be discovered? 
Is somebody going to contribute one next week that will be discovered months
from now?  This patch would make it possible to answer "No, ly_is_smob<T>(s)
returns true if and only if s is a T."

> And all that for avoiding less of a dozen instances of an open-coded
> dynamic_cast with reasonably clear intent?  No.

The intent of overriding Engraver::is_smob() etc. is clear only to one who
bothers to read it.  Maybe you undervalue avoiding future misuse because of
talent and familiarity.

[mail_philholmes.net, 2015-05-02 13:05:31]

----- Original Message ----- 
From: <nine.fierce.ballads@gmail.com>
To: <dak@gnu.org>
Cc: <reply@codereview-hr.appspotmail.com>; <lilypond-devel@gnu.org>
Sent: Saturday, May 02, 2015 1:54 PM
Subject: Re: Issue 4365: Add ly_is_smob<T>(S) to check if S is a smob of 
typeT. (issue 236850043 by nine.fierce.ballads@gmail.com)


> On 2015/05/02 08:25:09, dak wrote:
>
>> Nope.  dynamic_cast is non-trivial operation that cannot be optimized
> away at
>> compile time for almost any use.  Calling it for every instance of the
> rather
>> ubiquitous is_smob query (ubiquitous enough that I made it an
> operation separate
>> from unsmob and checked that the generated code was nicer) is
> excessive.
>
> When "Patchy the autobot says: passes tests" does that include any
> profiling?


No.  It tests that make and make doc complete successfully and that there 
are no regtest differences.

To test performance, you might like to run an unmodified version and this 
modified version against a reasonably complex lilypond source file.

--
Phil Holmes 

[dak, 2015-05-02 14:28:11]

On 2015/05/02 12:54:18, Dan Eble wrote:
> On 2015/05/02 08:25:09, dak wrote:
> 
> > Nope.  dynamic_cast is non-trivial operation that cannot be optimized away
at
> > compile time for almost any use.  Calling it for every instance of the
rather
> > ubiquitous is_smob query (ubiquitous enough that I made it an operation
> separate
> > from unsmob and checked that the generated code was nicer) is excessive.
> 
> When "Patchy the autobot says: passes tests" does that include any profiling?

No.  There is a handwavy bit of memory and property access profiling involved
but it is neither reproducible nor highly significant.  It has caught a few
blunders in the past.
 
> Since you've been through the exercise of examining generated code, I'm
curious
> whether you found dynamic_cast<T*>(T*) (or any upcast) producing anything
worse
> than static_cast<T*>(T*) would have.

Admittedly you are right regarding the performance implications of trivial
dynamic_cast calls: they should not exercise the polymorphic type mechanism and
should even compile on classes without virtual function table.

> There might be some cases where ly_is_smob<Child>() is called where only
> ly_is_smob<Parent>() is required (though this seems unlikely).  The right way
to
> improve performance in those cases is to ask the right question instead.

It becomes harder to ask the right question if we hide the difference between
right and wrong away.

> In the case of necessary down-casting, a test is required to prevent the
> function from answering the wrong question.  There is evidence that this has
> been recognized as a problem in some cases already: Engraver and other classes
> have an overridden unsmob() that includes a dynamic cast and an overridden
> is_smob() that depends on it.

Ah, the really funny thing here is that this has not been recognized as a
problem.  If you take a look at the overriden Engraver::unsmob history, it all
started in lily/engraver.cc as

+Engraver*
+unsmob_engraver (SCM eng)
+{
+  return dynamic_cast<Engraver*> (unsmob_translator (eng));
+}

in commit
commit ddd59edaae68e71d5d3ea2576b3d0d25807fb500
Author: Han-Wen Nienhuys <hanwen@xs4all.nl>
Date:   Thu Dec 31 02:49:04 2009 -0200

    Add basic scheme programmable engravers.
    
    * input/regression/scheme-engraver.ly shows a basic example.
    
    * extend \consists syntax to accept an alist of callables.
    
    * add Scheme_engraver which is the C++ glue to the Scheme callables.
    
    * Make get_listener_ in translator_listener_record also pass the
      listened class, so we can use generic infrastructure for hooking
      scheme functions to event listeners.
    
    * add scheme bindings:
    
      - ly:translator-context
      - ly:context-moment
      - ly:engraver-make-grob
    
    * Remove Translator::must_be_last_.  Use virtual method must_be_last
      () const instead.

No override involved here at all.  The seminal commit with which it became an
overriden static class member function is

commit 9066eeede909ace56324c905217c5b585ba42f90
Author: David Kastrup <dak@gnu.org>
Date:   Sun Jul 27 15:21:22 2014 +0200

    Replace remaining uses of unsmob_xxx with Xxx::unsmob
    
    This runs
    sed -i 's/unsmob_\(engraver\|global_context\|input\|item\|music\|paper_score
\|performance\|performer\|spanner\|stream_event\)\b/\u\1::unsmob/g' $(git grep -
l unsmob_ lily)
    
    as the mechanical part of the conversion.  A separate commit provides
    the required definitions.

So basically, this became "overriden" and part of the derivation chain as part
of a mostly unrelated mostly mechanical conversion.  And, uh, I did not realize
that dynamic_cast<T*>(T::unsmob(...)) was even required for downcasting before I
rewrote the listener stuff in issue 4357 to be type-safe and had the compiler
refuse to do a silent conversion here.  Could not have happened with
unsmob_engraver.

> Are there other cases waiting to be discovered? 
> Is somebody going to contribute one next week that will be discovered months
> from now?  This patch would make it possible to answer "No, ly_is_smob<T>(s)
> returns true if and only if s is a T."
> 
> > And all that for avoiding less of a dozen instances of an open-coded
> > dynamic_cast with reasonably clear intent?  No.
> 
> The intent of overriding Engraver::is_smob() etc. is clear only to one who
> bothers to read it.  Maybe you undervalue avoiding future misuse because of
> talent and familiarity.

So how is the intent of ly_is_smob clear to anyone not reading its definition? 
You do raise a number of valid points.  They just don't seem to make much of an
argument for ly_is_smob over the current solution.  I admit that the performance
angle is likely a non-issue for the trivial cases.

The question is whether one can throw in a mechanism for making LY_ASSERT_SMOB
deliver useful messages for such derived classes.  Even if it reverts to calling
the typeid unmangler from <cxxabi.h>.  I _think_ we should be able to rely on it
these days.  There is a test for it for the sake of the flower/yaffut testing
stuff and I seem to remember that the fallback via c++filt was never really
necessary.

Some side-channel advantage like that would go a lot towards motivating such a
change: the type testing for the callbacks in issue 4357 is pretty awkward.

[Dan Eble, 2015-05-02 16:11:20]

On 2015/05/02 14:28:11, dak wrote:
> On 2015/05/02 12:54:18, Dan Eble wrote:
> > There might be some cases where ly_is_smob<Child>() is called where only
> > ly_is_smob<Parent>() is required (though this seems unlikely).  The right
way
> to
> > improve performance in those cases is to ask the right question instead.
> 
> It becomes harder to ask the right question if we hide the difference between
> right and wrong away.

I see your point.  To make it obvious when dynamic_cast occurs and to reduce the
number of ways of doing things, what do you think of replacing both T::unsmob()
and T::is_smob() with these?

  ly_unsmob<T> (s) which compiles iff T is the base smob type
  ly_dynamic_unsmob<T> (s) which incorporates a dynamic_cast

Or if you prefer the very long, very obvious way, dynamic_cast<Derived *>
(ly_unsmob<T> (s)) instead of the latter.

> So how is the intent of ly_is_smob clear to anyone not reading its definition?


I don't think ly_is_smob<T> in context has any advantage over T::is_smob;
actually, T::is_smob looks nicer.  The problem is the surprises that arise when
inheritance is involved.  If T::is_smob() didn't require the intentional
overrides to avoid the surprises, it wouldn't be worth changing.

> The question is whether one can throw in a mechanism for making LY_ASSERT_SMOB
> deliver useful messages for such derived classes.
...
> Some side-channel advantage like that would go a lot towards motivating such a
> change: the type testing for the callbacks in issue 4357 is pretty awkward.

I disagree that fixing the inheritance problem with is_smob() needs any more
justification, but I will try to spend some time learning about this other need.
 Thanks for the feedback so far.

[Dan Eble, 2015-05-02 16:14:20]

On 2015/05/02 16:11:20, Dan Eble wrote:
>
>   ly_unsmob<T> (s) which compiles iff T is the base smob type

That sounds a little unclear.  I mean that T owns the smob tag; it is not
derived.

[Dan Eble, 2015-05-13 00:39:46]

further work in https://codereview.appspot.com/231680043/