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

[jonas . hahnfeld]

On 2020/04/14 07:24:10, hanwenn wrote:
> On 2020/04/14 07:00:56, hahnjo wrote:
> > On 2020/04/13 21:01:11, hanwenn wrote:
> > > it's hard to say if this makes a measurable difference:
> > > 
> > > [...]
> > 
> > So then ... why do it at all? From a code perspective, a deep copy
looks saner
> > in terms of "encapsulation" (one principle of object-oriented
programming). If
> > there is no provable gain, I'm for leaving the current code alone
until there
> is
> > good reason to change - "premature optimization is the root of all
evil"
> 
> As I described in the commit message, this code doesn't protect
anything,
> because get_property can also return values from the immutable list.

get_property uses data backed by mutable_property_alist_, right? So if
we do a deep copy there, a copied grob can return different data than
the original one. Whereas a copy of the list returns the same references
which can be modified from both instances of the object. At least that's
the way it works in C++, is there a difference with SCM values?

> Due to timing variability, we can't quantify performance changes below
1% of
> impact, but if we add 10 of them in a row, we do get measurable
impact. 
> 
> For the Mutable_properties change, we saw seemingly consistent
performance
> differences, which we could tease apart from deep copy change by
separating out
> the commit.

We did see a measurable difference with the Mutable_properties change,
so this cannot be the reason. I never said it was, it was just a guess.

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