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/