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

[dak]

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.


https://codereview.appspot.com/12708048/