Re: Avoid Scheme-computed Skyline_pair values to get collected before use (issue 12708048)

[dak]

Reviewers: Keith,


https://codereview.appspot.com/12708048/diff/1/lily/side-position-interface.cc
File lily/side-position-interface.cc (left):

https://codereview.appspot.com/12708048/diff/1/lily/side-position-interface.cc#oldcode216
lily/side-position-interface.cc:216: Skyline my_dim;
On 2013/08/11 19:30:46, Keith wrote:
> I am merely curious whether get_*_property() creates a temporary copy
of the
> Scheme object.  It seems to ultimately use Scheme's assq operation, to
return
> the property from the list and I don't know if assq creates a copy.

It does not create a temporary copy, so as long as a containing data
structure is not garbage-collected itself, one does not need to worry
too much.  Grobs are usually collected in a data structure on creation
(paper-score or something else) which is kept around until shipping, so
much of the code is rather lackadaisical about the life time of grobs.
And of course, the same holds for any data elements of a grob, and
again, the code is rather sloppy about keeping SCM references on stack
or elsewhere to protect against collection.

That is fine (though careless) as long as long as indeed the property in
question is a member of the grob, and that's always the case (I think)
for get_grob_data.  However, get_property can call callbacks, and then
the returned value is up for collection as soon as no SCM remains
referring to the returned value.

There is a _lot_ of code that will get problematic as soon as you use
callbacks for generating a property.

Description:
Avoid Scheme-computed Skyline_pair values to get collected before use

Retaining them as SCM values until they are no longer needed avoids
premature garbage collection.  This addresses circumstances in
connection with issue 3490.


Uploaded independently from the copy constructor patch since that
patch might hide the symptoms.

Please review this at https://codereview.appspot.com/12708048/

Affected files:
  M lily/side-position-interface.cc


Index: lily/side-position-interface.cc
diff --git a/lily/side-position-interface.cc  
b/lily/side-position-interface.cc
index  
dad906f51bf2cef85fcf9ac617d3cdc381a1b28d..8e23c2a03bb27734bfcd0b86afd9f42b43ca0025  
100644
--- a/lily/side-position-interface.cc
+++ b/lily/side-position-interface.cc
@@ -214,14 +214,13 @@ Side_position_interface::aligned_side (Grob *me, Axis  
a, bool pure, int start, i
     common[Y_AXIS] = staff_symbol->common_refpoint (common[Y_AXIS],  
Y_AXIS);

   Skyline my_dim;
-  Skyline_pair *skyp = Skyline_pair::unsmob (
-                         me->get_maybe_pure_property (a == X_AXIS
-                                                      ? "horizontal-skylines"
-                                                      : "vertical-skylines",
-                                                      pure,
-                                                      start,
-                                                      end));
-  if (skyp)
+  SCM skyp = me->get_maybe_pure_property (a == X_AXIS
+                                          ? "horizontal-skylines"
+                                          : "vertical-skylines",
+                                          pure,
+                                          start,
+                                          end);
+  if (Skyline_pair::unsmob (skyp))
     {
       // for spanner pure heights, we don't know horizontal spacing,
       // so a spanner can never have a meaningful x coordiante
@@ -237,7 +236,7 @@ Side_position_interface::aligned_side (Grob *me, Axis  
a, bool pure, int start, i
       Real yc = a == X_AXIS
                 ? me->pure_relative_y_coordinate (common[Y_AXIS], start,  
end)
                 : me->get_parent (Y_AXIS)->maybe_pure_coordinate  
(common[Y_AXIS], Y_AXIS, pure, start, end);
-      Skyline_pair copy = Skyline_pair (*skyp);
+      Skyline_pair copy = *Skyline_pair::unsmob (skyp);
       copy.shift (a == X_AXIS ? yc : xc);
       copy.raise (a == X_AXIS ? xc : yc);
       my_dim = copy[-dir];
@@ -280,16 +279,15 @@ Side_position_interface::aligned_side (Grob *me, Axis  
a, bool pure, int start, i
         {


-           Skyline_pair *sp = Skyline_pair::unsmob
-             (e->get_maybe_pure_property (a == X_AXIS
-                                          ? "horizontal-skylines"
-                                          : "vertical-skylines",
-                                          pure || cross_staff,
-                                          start,
-                                          end));
+           SCM sp = e->get_maybe_pure_property (a == X_AXIS
+                                                ? "horizontal-skylines"
+                                                : "vertical-skylines",
+                                                pure || cross_staff,
+                                                start,
+                                                end);

            aligns_to_cross_staff |= cross_staff;
-           if (sp)
+           if (Skyline_pair::unsmob (sp))
              {
                Real xc = pure && dynamic_cast<Spanner *> (e)
                          ? e->get_parent (X_AXIS)->relative_coordinate  
(common[X_AXIS], X_AXIS)
@@ -299,7 +297,7 @@ Side_position_interface::aligned_side (Grob *me, Axis  
a, bool pure, int start, i
                Real yc = a == X_AXIS
                          ? e->pure_relative_y_coordinate (common[Y_AXIS],  
start, end)
                          : e->maybe_pure_coordinate (common[Y_AXIS],  
Y_AXIS, pure, start, end);
-               Skyline_pair copy = Skyline_pair (*sp);
+               Skyline_pair copy = *Skyline_pair::unsmob (sp);
                if (a == Y_AXIS
                    && Stem::has_interface (e)
                    && to_boolean (me->get_maybe_pure_property  
("add-stem-support", pure, start, end)))