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

[jonas . hahnfeld]

On 2020/04/15 22:15:04, hanwenn wrote:
> 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 %:

A bit of explanation which number is which version would have helped.
But I think I can confirm the improvement, on my system from 41.7s to
40.3s; if you think that there's no drawback from omitting the deep
copy, I'm fine with this patch.

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