Re: Grow heap aggressively during music interpretation (issue 561390043 by address@hidden)

[jonas . hahnfeld]

The approach sounds good to me. I can confirm that this works on my
system and the runtime is pretty good. Please readd the autoconf logic
to detect bdw-gc.

FWIW I tried to research on the internals of GC. I found the following
statement that decides whether to collect or just expand the heap:
https://github.com/ivmai/bdwgc/blob/v8.0.4/alloc.c#L1435 We are hit by
the case
(GC_fo_entries > (last_fo_entries + 500) && (last_bytes_finalized |
GC_bytes_finalized) != 0)
Removing this case gets me to a state very close to this patch, both in
terms of performance and memory usage.

If I understand the approach correctly, GC_fo_entries counts the number
of outstanding finalizations. It seems like Guile is making good use of
this functionality, so GC tries to reclaim that memory first instead of
just expanding the heap. Please note that the number 500 is hard-coded,
so we have no means to influence it via environment variables or APIs. I
guess something like that could be added, but it would again take some
time to bubble through all distributions that we care about.

TL;DR: For now it's probably easiest to scale the heap as this patch
does.


https://codereview.appspot.com/561390043/diff/567180043/lily/include/smobs.hh
File lily/include/smobs.hh (right):

https://codereview.appspot.com/561390043/diff/567180043/lily/include/smobs.hh#newcode312
lily/include/smobs.hh:312: static size_t count;
On 2020/02/09 03:29:26, Dan Eble wrote:
> It seems that this is initialized to zero because it is static, but if
it simply
> had an "= 0", I wouldn't have had to go refresh my memory with a web
search.
> 
> Is it correct that there are no static Smobs anywhere in the program? 
If there
> were, it would be responsible to spend some time checking whether this
could be
> affected by the "static initialization order fiasco."  (Maybe you
already have.)

IIRC static objects without initialization are put into .bss which is
initialized to 0 when loading the binary. Adding "= 0" puts the variable
in data, but it'll still be initialized at load time. "static
initialization" is only a problem when the constructor is doing work,
which is not the case for an integer.

Anyway I think adding a comment here and in smobs.cc how this is
initialized (refer to static lifetime in C/C++) would likely clarify the
intent.

https://codereview.appspot.com/561390043/diff/567180043/lily/smobs.cc
File lily/smobs.cc (right):

https://codereview.appspot.com/561390043/diff/567180043/lily/smobs.cc#newcode23
lily/smobs.cc:23: #include <gc/gc.h>
You probably want this to be guarded by GUILEV2?

https://codereview.appspot.com/561390043/diff/567180043/lily/smobs.cc#newcode46
lily/smobs.cc:46: printf ("bytes per obj %d\n", size / count);
Please remove before committing.

https://codereview.appspot.com/561390043/