Re: Copy alist instead of deep copy on Grob clone (issue 561640045 by address@hidden)

[hanwenn]

On 2020/04/14 20:07:17, hahnjo wrote:
> On 2020/04/14 19:14:29, hanwenn wrote:
> > On 2020/04/14 10:22:01, hahnjo wrote:
> > > On 2020/04/14 09:39:18, hanwenn wrote:
> > > > it reads from the immutable_list_ if there is no override in the
> > > > mutable property list.
> > > 
> > > Ack, that's also what the two names suggested. The code in Grob's
copy
> > > constructor does a copy of mutable_property_alist_. As far as I
understand
> it
> > > contains those elements that have been changed for the Grob? So
I'm thinking
> > > about the following situation (not real code, just conceptually):
> > > Grob g1;
> > > g1.set_property("key", "value");
> > > Grob g2(g1);
> > > g2.set_property("key", "value2");
> > > 
> > > set_property will change mutable_property_alist_, and with this
change both
> > > Grobs will share some of it. So I can't help, but this looks like
the
> > > g1.get_property("key") will probably return "value2" at the end,
no?
> > 
> > The code now calls ly_alist_copy(), which constructs a new alist
with the same
> > keys and values,
> > so your scenario will continue to work correctly.
> > 
> > A deep copy protects against something like
> > 
> >  Grob g1;
> >  g1.set_property("key", scm_cons(1,2));
> >  Grob g2(g1);
> >  SCM v = g2.get_property("key");
> >  scm_set_cdr_x(v, 3)
> > 
> > but this protection only works if the set_property was called
beforehand. If
> you
> > do this for a something that comes out of the immutable list, it'll
be a
> > disaster.
> 
> My point is that there is some shared memory by two different Grobs.
This can
> potentially lead to problems and very surprising behavior. Given that
there's no
> measurable performance impact, I remain opposed to this change.

My point is that all Grobs already share memory, but you are right that
if there 
were no performance upside, it seems an unnecessary risk to take.

I did some better structured measurements, with interleaved runs on
MSDM:

$ python  f.py out.txt 
[55.57, 56.29, 55.43, 56.31, 55.44, 55.89, 55.21, 56.14]
1st
avg 55.4125
med 55.435
stddev 0.149
2nd
avg 56.1575
med 56.215
stddev 0.194
med diff 1.407053 %:

This is consistent with the profile data I have. There I see:

  1.58      2.49     0.13   115613     0.00     0.00 
ly_deep_copy(scm_unused_struct*)

(1.58% of runtime is this function.)


                            16394155            
ly_deep_copy(scm_unused_struct*) [79]
                0.00    0.00      40/115613     
Accidental_engraver::update_local_key_signature(scm_unused_struct*)
[422]
                0.00    0.00     123/115613     
set_context_property_on_children(Context*, scm_unused_struct*,
scm_unused_struct*) [425]
                0.13    0.00  115450/115613      Grob::Grob(Grob const&)
[81]
[79]     1.6    0.13    0.00  115613+16394155
ly_deep_copy(scm_unused_struct*) [79]
                             16394155            
ly_deep_copy(scm_unused_struct*) [79]


ie. virtually all non-recursive calls come from Grob::Grob.

https://codereview.appspot.com/561640045/