Re: Issue 4365: Add ly_is_smob<T>(S) to check if S is a smob of type T. (issue 236850043 by address@hidden)

[dak]

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 <address@hidden>
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 <address@hidden>
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.

https://codereview.appspot.com/236850043/