Issue 12708048: Avoid Scheme-computed Skyline_pair values to get collected before use (Closed)
DescriptionAvoid 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.
Patch Set 1 #
Total comments: 4
MessagesTotal messages: 6
looks quite good to me, of course. 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.c... lily/side-position-interface.cc:216: Skyline my_dim; 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. If get_*_propertry does create a temporary copy of the Scheme object, unsmob() returns a pointer inside the object, but there is no lasting reference to the object itself. The function call on line 239 causes quite a lot of skylines to be looked up, and this function aligned_side() to be called many times, so it is not surprising if a temporary object is overwritten before the pointer is used on line 240.
Sign in to reply to this message.
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.c... 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.
Sign in to reply to this message.
On Sun, 11 Aug 2013 12:46:04 -0700, <dak@gnu.org> wrote: > 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.c... > 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. > 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. > That's it. The property in question is 'vertical-skylines' and it is generated by a callback. From the stacktrace we can determine that get_*_property() was called with pure==true to generate the object that is pointed to by the copy of 'skyp' that is associated with the crash. So get_*_property() used internal_get_pure_property() which returns the computed skylines as a SCM object /without/ storing it in the 'vertical-skylines' property (because it wants to leave a pointer to callback there for when we need the real, unpure, skylines).
Sign in to reply to this message.
On 2013/08/11 20:41:48, Keith wrote: > On Sun, 11 Aug 2013 12:46:04 -0700, <mailto:dak@gnu.org> wrote: > > > > 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.c... > > 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. > > > 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. > > > > That's it. The property in question is 'vertical-skylines' and it is generated > by a callback. From the stacktrace we can determine that get_*_property() was > called with pure==true to generate the object that is pointed to by the copy of > 'skyp' that is associated with the crash. > > So get_*_property() used internal_get_pure_property() which returns the computed > skylines as a SCM object /without/ storing it in the 'vertical-skylines' > property (because it wants to leave a pointer to callback there for when we need > the real, unpure, skylines). > Well, I wanted to point out that the continuing presence of the problem when compiling without optimization makes this problem scenario unlikely since the SCM variable containing the returned value can't possibly be optimized out of existence. But it turns out that the original code never put the returned SCM value into a variable in the first place but rather unsmobbed it immediately. So yes, I'd definitely like to hear back whether current master still exhibits the problem. It's conceivable that this change helped, though there don't seem to be a lot of opportunities for garbage collection between return of the SCM value and use of the Skyline_pair.
Sign in to reply to this message.
On 2013/08/11 21:07:03, dak wrote: > It's conceivable that this change helped, > though there don't seem to be a lot of opportunities > for garbage collection between return of the SCM > value and use of the Skyline_pair. There is ample opportunity for garbage generation and garbage collection. The call to maybe_pure_coordinate() estimates positions for everything in the score, which causes a lot of computation and memory use. Adding the two tracer printf()s below, I see a SCM object holding the skyline for the "Presto" is returned, then two hundred twenty three other skylines are created, side-positioned, and discarded, then the pointer into the skyline for the "Presto" is accessed. https://codereview.appspot.com/12708048/diff/1/lily/side-position-interface.cc File lily/side-position-interface.cc (right): https://codereview.appspot.com/12708048/diff/1/lily/side-position-interface.c... lily/side-position-interface.cc:216: Skyline my_dim; if (pure) printf("Storing pointer to skyline of a %s\n", me->name().c_str()); https://codereview.appspot.com/12708048/diff/1/lily/side-position-interface.c... lily/side-position-interface.cc:238: : me->get_parent (Y_AXIS)->maybe_pure_coordinate (common[Y_AXIS], Y_AXIS, pure, start, end); if (pure) printf("Accessing pointer to skyline of a %s\n", me->name().c_str());
Sign in to reply to this message.
On 2013/08/12 05:50:10, Keith wrote: > On 2013/08/11 21:07:03, dak wrote: > > > It's conceivable that this change helped, > > though there don't seem to be a lot of opportunities > > for garbage collection between return of the SCM > > value and use of the Skyline_pair. > > There is ample opportunity for garbage generation and garbage collection. The > call to maybe_pure_coordinate() estimates positions for everything in the score, > which causes a lot of computation and memory use. > > Adding the two tracer printf()s below, I see a SCM object holding the skyline > for the "Presto" is returned, then two hundred twenty three other skylines are > created, side-positioned, and discarded, Independent of this issue, this seems, uh, excessive? > then the pointer into the skyline for > the "Presto" is accessed. I think that the C++ memory allocator is independent of the Scheme allocator (ly:format uses scm_malloc so it may garbage collect when memory is tight, but that seems about the only use of scm_malloc). So as long as the skylines calculated in between are not smobbed (turned into Scheme objects), the Scheme garbage collector has no incentive to run. Some Scheme objects, however, are allocated whenever a symbol is encountered for the first time, so there are a lot of code paths that seem safe from triggering garbage collection _except_ when they are run for the first time. While most symbols used via get_property or similar are already somewhere present in permanent Scheme code (and thus the symbols themselves are already allocated), the memoizing construct used internally by get_property enters them into a weak hash table to make sure they have a reference. Maintenance of that weak hash table can still cause a Scheme allocation on the first run through. In short: it's just not a good idea to rely on no Scheme allocations in a non-trivial code passage. At any rate, the circumstances of this patch seem sufficient for retesting current master.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||
