Re: Resolving standoffs (was: Naming question for get_property, set_property)

[Han-Wen Nienhuys]

On Tue, Apr 14, 2020 at 12:13 AM Carl Sorensen <address@hidden> wrote:
> 1) David has an excellent track record of making challenging things easier to 
> do by thinking carefully about the implications of potentially arbitrary 
> syntax.  For example, the parser has been made more robust, and the scheme 
> interaction with the LilyPond parser has become much easier.  Because David 
> has such an excellent track record in this regard, I am inclined to favor 
> allowing this patch.

This change doesn't concern the parser, though.

> 2) Han-Wen believes that this syntax change is potentially bad for at least 
> the following reasons:
>    a) The approach "seems laborious"
>    b)  The approach will make it hard to add new grob-properties at runtime 
> (David K. disagrees with this item -- in fact he believes it will force us to 
> provide a proper interface for registering user-added properties).
>    c)  Uses more memory -- lots of bytes per Grob.  David K. says it only 
> uses ~500 bytes per Grob type, not per Grob.
>    d) Interferes with Guile 2 adoption.  David K. says the current patch has 
> no effect on Scheme; further work on Scheme is in the future.
>    e) It's not necessary to make all these changes in the code base to do the 
> experiments that David K. wants to do.
>
> 3) Han-Wen has proposed that David's future development be based on a 
> separate branch, rather than master.  Once the benefits are shown, Han-Wen 
> has promised to help merge the separate development branch into the mainline. 
>  David states that working on stale code (diverged from master) for an 
> indefinite period of time is a significant hindrance to moving forward.
>
> 4) Although I'm recognized as a smart guy in my world, the events surrounding 
> this proposed change have demonstrated to me that the LilyPond project is 
> fortunate to have brilliant people working on it.  I feel like I'm not even 
> in the same area code as Han-Wen and David.  Thanks to both of you!
>
> I think for me, the bottom line is that David has proposed a reason why the 
> proposed new syntax is better than the existing syntax.  The reason is a 
> little bit vague to me, but I trust David in this kind of situation.  Han-Wen 
> has identified some potential warning issues about the new syntax, to which 
> David has responded.  The only issue that Han-Wen raises for which I'm not 
> satisfied an answer exists is item b) above.  If this patch breaks something 
> users currently can do (adding new grob-properties at runtime) without 
> providing another facility to do so, I think that's a legitimate concern.  On 
> the other hand, I think that David's proposed memoization is likely to be 
> very useful.
>
> David, do you agree that as it stands now, your patch precludes adding grob 
> properties at runtime?  If so, would it be reasonable to add the 
> infrastructure for adding grob properties at runtime to this patch?
>
> Han-Wen, do you agree that David has addressed your concerns about memory and 
> Guile2?  If so, and he could resolve your concerns about adding grob 
> properties at runtime, would you be willing to accept this syntax change?
>
> From  where I stand,  the only think keeping me from giving a LGTM is the 
> possible difficulty in adding new grob properties at runtime.  But I don’t 
> have a clear sense of how significant that problem is.

I think your assessment of the patch and my comments misses the mark.

Here is the situation as it stands:

Currently, if you run a profile of processing any large file,
Grob:internal_get_property is one of the top functions. In the current
master, it stands at number 3 for the MSDM score, with
3.5% of time spent in 27635947 calls in 0.29 seconds.  This number
also points to the challenge: individually, the function is actually
extremely cheap: 27635947 times in 0.29 seconds means it takes 10
nanoseconds on average, or about 26 CPU cycles to run. That is not a
lot of slack to optimize away.

My experiments from this weekend confirmed that it is extremely hard
to make that number budge. If the plans that David had for this
function made sense, some of my changes should have also shown some
improvement. David -on the contrary- claims that his ideas are totally
different, and that I don't understand them. You have pointed to
David's past, solid improvements to the parser, so I will point to
https://lists.gnu.org/archive/html/lilypond-devel/2020-01/msg00410.html.
David has been adamant for several years that LilyPond+GUILEv2
performs so terribly because UTF-8 transcoding across the GUILE API,
which makes no sense when you think through the argument.

David wants to rearrange get_property calls so it can emit different
code based on the receiver type. It would be strange if that worked,
because compile-time, you cannot distinguish between Grob* and its
subclasses Paper_column, Item, Spanner and System: the pointers are
often upcast from their origin and then passed to other functions
which call internal_get_property on the baseclass (Grob) rather than
the subclass. The same concern holds for LIlypond grob types, ie.
NoteHead, Stem, etc. So, you can only distinguish between Grob vs.
Context vs. Prob (ie. Music). By specializing for Grobs, you cut the
number of properties to consider from 800 to 400, but 400 is still too
large a vector to add to every grob. So you need a second level
lookup, but the second level cannot be memoized, because it depends on
data which cannot be resolved at compile time. So you're back to
runtime lookups, which is essentially what I tried out here,
https://github.com/hanwen/lilypond/tree/grob-dict.

In short, David is asking me to set aside my well-founded skepticism
on his plans, so get_property can decltype to get at the type of the
receiver, ie.

  #define get_property(receiver, name) \
     { something something (decltype)(*receiver_type) }

I am concerned that the logical next step will require including the
definitions of Music, Grob, Context etc. so the macro can actually do
something different based on its type. This would make all of LilyPond
depend on all of those headers, which will significantly slow down
recompiles when working on the source code. We'd be looking at another
global refactor to split out get_property back into
get_{music,context,grob}_property. Maybe it won't, but so far, David
hasn't shown what he intends to do, so it is hard to judge.

If David wants to experiment to make something that compiles against
current master, he could simply refactor the get_object/set_object()
calls. They work exactly the same, and because the macro is already
separate, there is no need to do a global refactoring. That wouldn't
work to demonstrate improvements in benchmarks, but it would
demonstrate the overall idea, so we know what we're talking about, and
we can verify that it at least works correctly. In addition, you could
run a profile to demonstrate that the get_object performance improves
rather than deteriorates. (according to the profile, get_object runs
in 3 nanoseconds, ie. 8 cycles on average.)

David has not mentioned two downsides to his refactoring:

1. it touches code all over the place making git-blame less useful.
2. all other pending changes become invalidated and need to be
reorganized, including historical changes.

In short, I see this as a poorly specified, ill-conceived plan, and to
add insult to injury, nobody has tried to evaluate my extensive
technical commentary on the plan at hand.

I also briefly wish to address Dan's point:

get_property is a macro for performance reasons, but to the caller the
distinction is invisible, and it would certainly be a member function
if there no performance considerations. This pattern is also common in
the GUILE source code, for example, scm_is_eq looks like a function
(lower case), but it is actually a macro for performance reasons.

For thinking about the readability of the source code at the caller
side, it is better that it remains as a method. We can do the
following to ensure it is documented in the classes appropriately:

#define memoized_get_property( .. )

class Grob {
..
#ifndef get_property
#define get_property(x) memoized_get_property(x)
#endif

}

or maybe:

class Grob {

#define __tmp get_property
#undef get_property

   // this is actually a macro for performance reasons.
   get_property(..)  { }

#define get_property __tmp
}

-- 
Han-Wen Nienhuys - address@hidden - http://www.xs4all.nl/~hanwen