GC Warning related to large mem block allocation - Help needed

GC Warning related to large mem block allocation - Help needed

[David Pirotte]

[-- Attachment #1: Type: text/plain, Size: 3127 bytes --]

Hello,

Currently, using guile-cv upon large images triggers these warnings,
using guile guile 2.2.3:

	GC Warning: Repeated allocation of very large block (appr. size 775237632):
        May lead to memory leak and poor performance.

	... then 1 '=' above msg per block allocation, tens to hundreds, sometimes
	even thousands of these, depending on the image processing being done ...

These are totally unnecessary (I know I'm allocating these blocks), renders the
repl 'output' unreadable/unusable and actually scare users (well, I only have one
user for now).

So I am trying to understand and solve this problem. Below a (naive) attempt to
patch guile so it uses GC_malloc_ignore_off_page for objects > 100Kb, but that
did not even work: I guess I do miss most if not all of the puzzle pieces here...

While I wrote this tiny patch,  2 quiz also raised in my mind, here there are,
together with the naive patch (below):

1-    should this function return volatile ? (and how)

In bwd-gc README file you can read:

	https://github.com/ivmai/bdwgc

GC_malloc_ignore_off_page(bytes)

    Identical to GC_malloc, but the client promises to keep a pointer to the
    somewhere within the first 256 bytes of the object while it is live. (This
    pointer should normally be declared volatile to prevent interference from
    compiler optimizations.) This is the recommended way to allocate anything that
    is likely to be larger than 100 Kbytes or so. (GC_malloc may result in failure
    to reclaim such objects.)

and if yes, how would I achieve this since this function has multiple return
calls (with different pointer type) ?

2-    I don't understand the last return 'case'

    ...
      return scm_inline_gc_alloc
        (&thread->freelists[idx], idx, SCM_INLINE_GC_KIND_NORMAL);
    ...

How should that be patched so it also use GC_malloc_ignore_off_page(bytes)
when bytes > 100Kb?

Thanks,
David

;;;
;;; Below the patch that does not work :)
;;;

From 0ae4f18e478b2e390de72f560ff8428edfeda860 Mon Sep 17 00:00:00 2001
From: David PIROTTE <david@altosw.be>
Date: Fri, 29 Dec 2017 22:40:52 -0200
Subject: [PATCH] Use GC_malloc_ignore_off_page for objects > 100Kb

* libguile/gc-inline.h (scm_inline_gc_malloc): Use
  GC_malloc_ignore_off_page for objects > 100Kb.
---
 libguile/gc-inline.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/libguile/gc-inline.h b/libguile/gc-inline.h
index fcbe5a54e..7d02e67cc 100644
--- a/libguile/gc-inline.h
+++ b/libguile/gc-inline.h
@@ -119,8 +119,12 @@ scm_inline_gc_malloc (scm_i_thread *thread, size_t bytes)
 {
   size_t idx = scm_inline_gc_bytes_to_freelist_index (bytes);
 
-  if (SCM_UNLIKELY (idx >= SCM_INLINE_GC_FREELIST_COUNT))
-    return GC_malloc (bytes);
+  if (SCM_UNLIKELY (idx >= SCM_INLINE_GC_FREELIST_COUNT)) {
+    if (bytes > 100000)
+      return  GC_malloc_ignore_off_page (bytes);
+    else
+      return GC_malloc (bytes);
+  }
 
   return scm_inline_gc_alloc
     (&thread->freelists[idx], idx, SCM_INLINE_GC_KIND_NORMAL);
-- 
2.15.0

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: GC Warning related to large mem block allocation - Help needed

[David Pirotte]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

Hi again,

> Currently, using guile-cv upon large images triggers these warnings,
> using guile guile 2.2.3:
> 
> 	GC Warning: Repeated allocation of very large block (appr. size 775237632):
>         May lead to memory leak and poor performance.

Of course you don't need guile-cv to reproduce the problem; if you have enough mem,
you can try (or try reducing the mem block, raising the iota arg ...):

scheme@(guile-user)> ,use (srfi srfi-4)
scheme@(guile-user)> (for-each (lambda (i) (make-f32vector 100000000)) (iota 100))
GC Warning: Repeated allocation of very large block (appr. size 400003072):
        May lead to memory leak and poor performance.
GC Warning: Repeated allocation of very large block (appr. size 400003072):
        May lead to memory leak and poor performance.
GC Warning: Repeated allocation of very large block (appr. size 400003072):
	May lead to memory leak and poor performance.
...

David

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: GC Warning related to large mem block allocation - Help needed

[David Pirotte]

[-- Attachment #1: Type: text/plain, Size: 1155 bytes --]


> So I am trying to understand and solve this problem. Below a (naive) attempt to
> patch guile so it uses GC_malloc_ignore_off_page for objects > 100Kb, but that
> did not even work: I guess I do miss most if not all of the puzzle pieces here...

Obviously (missing all the pieces of the puzzle): I see now that srfi-4 uses srfi-4.c
(thanks ijp), which calls scm_i_make_typed_bytevector, defined in bytevectors.c,
which calls make_bytevector, which calls  scm_gc_malloc_pointerless, which is
defined as #define SCM_GC_MALLOC_POINTERLESS(size) GC_MALLOC_ATOMIC (size), which I
presume calls BDW-GC GC_malloc_atomic(nbytes)

So, (a) the patch below is totally useless for any srfi-4 ops, and (b) it would
probably be more interesting to address this GC Warning reports for repeated 
large block of mem with a 'global' guile approach ... which I unfortunately can't
do: I just don't have the knowledge and the experience.

I can still try to patch srfi-4.c, for fun and locally, but even that is not easy
for me, because I read and write basic C (far from expert level), and I'm quickly
lost even just reading libguile code ...

David

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: GC Warning related to large mem block allocation - Help needed

[Mike Gran]

On Sat, Dec 30, 2017 at 02:05:21AM -0200, David Pirotte wrote:
> 
> > So I am trying to understand and solve this problem. Below a (naive) attempt to
> > patch guile so it uses GC_malloc_ignore_off_page for objects > 100Kb, but that
> > did not even work: I guess I do miss most if not all of the puzzle pieces here...

If all you are doing is trying to get Guile not to issue warnings about big
allocations, I think all you need to do is put -DGC_IGNORE_WARN in the
CFLAGS when you build Guile.

If you don't or can't rebuild Guile, you might get away with including the
following somewhere early in your library.  Or it may not work. Dunno.

  GC_set_warn_proc(GC_ignore_warn_proc);

> 
> Obviously (missing all the pieces of the puzzle): I see now that srfi-4 uses srfi-4.c
> (thanks ijp), which calls scm_i_make_typed_bytevector, defined in bytevectors.c,
> which calls make_bytevector, which calls  scm_gc_malloc_pointerless, which is
> defined as #define SCM_GC_MALLOC_POINTERLESS(size) GC_MALLOC_ATOMIC (size), which I
> presume calls BDW-GC GC_malloc_atomic(nbytes)
> 
> So, (a) the patch below is totally useless for any srfi-4 ops, and (b) it would
> probably be more interesting to address this GC Warning reports for repeated 
> large block of mem with a 'global' guile approach ... which I unfortunately can't
> do: I just don't have the knowledge and the experience.
> 
> I can still try to patch srfi-4.c, for fun and locally, but even that is not easy
> for me, because I read and write basic C (far from expert level), and I'm quickly
> lost even just reading libguile code ...
> 
> David

Re: GC Warning related to large mem block allocation - Help needed

[David Pirotte]

[-- Attachment #1: Type: text/plain, Size: 2550 bytes --]

Hi Mike,

> If all you are doing is trying to get Guile not to issue warnings about big
> allocations, I think all you need to do is put -DGC_IGNORE_WARN in the
> CFLAGS when you build Guile.

Thanks for the suggestion, but it does not work. If I trust guile (gcc) to use an
exported CFLAGS env variable:

	export CFLAGS="-O3 -DGC_IGNORE_WARN"
	[ which is properly listed in the config.log after configure...

and if this flag is 'correct', it appeared to work at first glance:

	scheme@(guile-user)> ,use (srfi srfi-4)
	scheme@(guile-user)>  (for-each (lambda (i) (make-f32vector 100000000)) (iota 100))
	[ no warnings here

but a more expensive task triggers these warnings still

scheme@(guile-user)> ,time (quasep caso-9 '1mm)
 init...  GC Warning: Repeated allocation of very large block (appr. size 775012352):
	May lead to memory leak and poor performance.
kernels... GC Warning: Repeated allocation of very large block (appr. size 775237632):
	May lead to memory leak and poor performance.
small pellets... GC Warning: Repeated allocation of very large block (appr. size 775012352):
	May lead to memory leak and poor performance.
grip layer... GC Warning: Repeated allocation of very large block (appr. size 775012352):
	May lead to memory leak and poor performance.
GC Warning: Repeated allocation of very large block (appr. size 775012352):
	May lead to memory leak and poor performance.
micro pellets... GC Warning: Repeated allocation of very large block (appr. size 775012352):
	May lead to memory leak and poor performance.
GC Warning: Repeated allocation of very large block (appr. size 775012352):
	May lead to memory leak and poor performance.
gripped kernels... GC Warning: Repeated allocation of very large block (appr. size 775012352):
	May lead to memory leak and poor performance.
GC Warning: Repeated allocation of very large block (appr. size 775012352):
	May lead to memory leak and poor performance.
GC Warning: Repeated allocation of very large block (appr. size 775012352):
	May lead to memory leak and poor performance.
none gripped kernels... GC Warning: Repeated allocation of very large block (appr. size 775012352):
	May lead to memory leak and poor performance.
quase particles... GC Warning: Repeated allocation of very large block (appr. size 775012352):
	May lead to memory leak and poor performance.

$1 = ((13755 14086 3 (#f32(255.0 255.0 255.0 255.0 255.0 255.0 255.0 # # ...) ...)) # ...)
;; 300.318682s real time, 369.596406s run time.  5.252115s spent in GC.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: GC Warning related to large mem block allocation - Help needed

[David Pirotte]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

Hello,

> > If all you are doing is trying to get Guile not to issue warnings about big
> > allocations, I think all you need to do is put -DGC_IGNORE_WARN in the
> > CFLAGS when you build Guile.  

> Thanks for the suggestion, but it does not work.

For those interested, Mike did find a way to get rid of those warnings, and posted it
in #guile:

	<spk121> daviid: to quell BDW-GC large alloc warnings via environment
        variables, you can set GC_LARGE_ALLOC_WARN_INTERVAL to something much
        larger than its default of 5

which works perfectly, thanks Mike!

David.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: GC Warning related to large mem block allocation - Help needed

[Freja Nordsiek]

David Pirotte and Guile Devel,


Changing GC_LARGE_ALLOC_WARN_LEVEL seems like it is the best solution to
me. Looking at the suggestions to change the allocator earlier in the
discussion, I decided to look more into the feasibility of that solution
to see if that alternative was fixable to avoid having to change
GC_LARGE_ALLOC_WARN_LEVEL. I did some digging around
on https://github.com/ivmai/bdwgc and  http://www.hboehm.info/gc/ and
have dug around a bit inside bytevectors.c in the past. My conclusion is
that while changing the allocator properly would fix the warning, it
could introduce more problems. So, sadly, it seems changing
GC_LARGE_ALLOC_WARN_LEVEL is the only solution. Unless my analysis is
wrong (would be nice if it was because changing the allocator would help
performance too). My analysis follows:



The warning is thrown because large arrays can cause major performance
problems for garbage collectors that work like BDWGC. They decide to
keep or collect objects based on whether anything in other objects that
are being kept or in the stack point to them (either at their head or
somewhere in their interiors). The standard BDWGC malloc (GC_MALLOC)
allocates objects that could potentially have pointers in them and thus
need to be searched for pointers to other objects. Such searching can be
expensive. The BDWGC atomic malloc (GC_MALLOC_ATOMIC) is basically
declaring that the object does not contain pointers and thus does not
need to be searched, which saves a lot of effort for the GC. But,
regardless of whether the allocation is atomic (no pointers) or not,
BDWGC still needs to search everything else for pointers to the objects.
Structs and things like that have a pointer or two need to be declared
to have pointers even if the rest is not pointers. But the rest of the
data is effectively random pointers when BDWGC looks at them. Same goes
for everything on the stack that is not a pointer. The larger an
allocated array is, the more likely that some non-pointer data will
accidentally point to its interior if it is looked at through the lens
of a pointer. This is why BDWGC throws the warning when large arrays are
allocated repeatedly with GC_MALLOC and GC_MALLOC_ATOMIC. There is a
high probability that many of them will be kept around beyond what is
required (and thus they take up RAM) due to non-pointers accidentally
pointing to them, so BDWGC lets the programmer and user know.

To help mitigate this, BDWGC offers GC_MALLOC_IGNORE_OFF_PAGE and
GC_MALLOC_ATOMIC_IGNORE_OFF_PAGE (note, the latter function is not
mentioned on the readme on the git page but is mentioned at
http://www.hboehm.info/gc/gcinterface.html) which do the same
allocations, but only considers them pointed to if pointers or data
BDWGC thinks might be pointers points to the first 512 bytes of the
objects. Since they then look like 512 byte long objects to BDWGC for
the purpose of deciding whether to keep or collect them, there is a much
lower probability of them being accidentally kept a long time. There is
one major catch. If one is still using the object but one's only pointer
to them is pointing at somewhere after the 512 byte mark, they could get
prematurely collected.

Now, going to SRFI-4 vectors and R6RS bytevectors, which underneath use
mostly the same code in Guile, they are  allocated in make_bytevector
with GC_MALLOC_ATOMIC (indirectly through SCM_GC_MALLOC_POINTERLESS) and
an SCM with a pointer to the head returned by the function. In
principle, that could be changed to do a size check and then use
GC_MALLOC_ATOMIC_IGNORE_OFF_PAGE if it is larger than 100 kB (note,
changing it to the non-atomic version while it would get rid of the
warning and make sure it doesn't get kept too long on accident, would
mean that it is searched inside for pointers which could then keep other
stuff on accident).

The only worry then would be that it would get collected while still
being used. I think most cases, this would not be a problem. However, if
someone makes a new bytevector from an existing one from somewhere in
the middle, it is possible that the new one would only point to the
middle and not the head and thus could be collected prematurely (would
need to do some more digging to see if the new one would be allocated
using make_bytevector_from_buffer). Or, if someone was using C code to
say take the norm of the vector (very common operation often done with
BLAS) and the scheme code wasn't going to use the bytevector anymore,
there might only be a pointer on the stack pointing to the current
element that the C code is reading and as soon as it gets past the 512
byte mark, the bytearray might get collected while it is still being
worked on which would be a disaster. So I am not sure that the
allocation could be safely changed to use
GC_MALLOC_ATOMIC_IGNORE_OFF_PAGE if the bytevector is large. I do not
know enough about Guile internals yet to know if typical pure scheme
operations would run into problems. I think it is definitely possible
that there are FFI cases where problems could be run into, which would
then mean the coder has to take extra precautions to prevent collection,
which could be a major problem for changing the allocation Guile 2.0.x
and 2.2.x since it would be a major API change. Wouldn't be such an
issue for 3.x series since the API could be changed but it would be a
bit of a surprising result for people to have to worry about if using
FFI. I could be wrong on this - a pointer to the head might still be
kept on the stack and then there is no problem.

So, it seems, that disabling the warning through
GC_LARGE_ALLOC_WARN_LEVEL or some other method is the only safe
solution, unless my analysis above is wrong and the allocation code
could be safely changed.


Freja Nordsiek


On 12/31/2017 02:22 PM, David Pirotte wrote:
> Hello,
>
>>> If all you are doing is trying to get Guile not to issue warnings about big
>>> allocations, I think all you need to do is put -DGC_IGNORE_WARN in the
>>> CFLAGS when you build Guile.  
>> Thanks for the suggestion, but it does not work.
> For those interested, Mike did find a way to get rid of those warnings, and posted it
> in #guile:
>
> 	<spk121> daviid: to quell BDW-GC large alloc warnings via environment
>         variables, you can set GC_LARGE_ALLOC_WARN_INTERVAL to something much
>         larger than its default of 5
>
> which works perfectly, thanks Mike!
>
> David.
>
>

Re: GC Warning related to large mem block allocation - Help needed

[Daniel Llorens]

On 01 Jan 2018, at 15:11, Freja Nordsiek <fnordsie@posteo.net> wrote:

> The only worry then would be that it would get collected while still
> being used. I think most cases, this would not be a problem. However, if
> someone makes a new bytevector from an existing one from somewhere in
> the middle, it is possible that the new one would only point to the
> middle and not the head and thus could be collected prematurely (would
> need to do some more digging to see if the new one would be allocated
> using make_bytevector_from_buffer). Or, if someone was using C code to
> say take the norm of the vector (very common operation often done with
> BLAS) and the scheme code wasn't going to use the bytevector anymore,
> there might only be a pointer on the stack pointing to the current
> element that the C code is reading and as soon as it gets past the 512
> byte mark, the bytearray might get collected while it is still being
> worked on which would be a disaster. So I am not sure that the
> allocation could be safely changed to use
> GC_MALLOC_ATOMIC_IGNORE_OFF_PAGE if the bytevector is large. I do not
> know enough about Guile internals yet to know if typical pure scheme
> operations would run into problems. I think it is definitely possible
> that there are FFI cases where problems could be run into, which would
> then mean the coder has to take extra precautions to prevent collection,
> which could be a major problem for changing the allocation Guile 2.0.x
> and 2.2.x since it would be a major API change. Wouldn't be such an
> issue for 3.x series since the API could be changed but it would be a
> bit of a surprising result for people to have to worry about if using
> FFI. I could be wrong on this - a pointer to the head might still be
> kept on the stack and then there is no problem.

Hi Freja,

thanks for these comments. I know too little about GC, but I've dug now and then in the bytevector / array code. As a user of large arrays, I'm interested in making them more usable. 

The primary means for indirect access to bytevectors / arrays in Guile is the array API. All array objects contain a reference to their ‘backing store’, and array handles keep a second copy of this reference, plus a pointer to the head of the backing store (not to the head of the array itself). The array API uses a get/release mechanism whose only purpose is to keep those pointers on the stack. This makes using arrays harder and I've found it annoying, so I've asked Andy about it a couple times.

AFAIU, the only functions in the public API that can create a vector/bytevector object from raw memory are the scm_take_xxx series (which are used internally by pointer->bytevector). Those functions are clearly meant to be used with ‘foreign’ storage, but if one were to use them with Guile-managed storage, then I think it's understood that the user is responsible for retaining the relevant pointers.

That's also how I understand the comments above make_bytevector_from_buffer.

Also AFAIU, the only ways to get a pointer to bytevector storage using the public API are 1) scm_xxx_elements / scm_xxx_writable_elements, 2) the macro SCM_BYTEVECTOR_CONTENTS, or 3) the FFI function bytevector->pointer.

The scm_xxx_elements functions take an array handle and enforce the get/release interface, so they should cause no issues.

I think it would be fair to add a warning to the manual that the user is responsible for retaining the pointer obtained from SCM_BYTEVECTOR_CONTENTS around any calls. I think that's what the scm_remember_upto_here functions are for, although I've never had to use them myself. I've also never used SCM_BYTEVECTOR_CONTENTS directly. I'm not sure it belongs in the public API to be honest.

Then bytevector->pointer uses SCM_BYTEVECTOR_CONTENTS internally, but it also keeps (SCM_BYTEVECTOR_CONTENTS(bv) + offset) in a ‘pointer_weak_refs’ table. So it seems possible for this to happen:

y = make_bytevector
x = bytevector->pointer(y, offset = 512+1)
do_stuff_with(x /* y is collected */)

either in Scheme or in C. But it would be solved if bytevector->pointer kept just SCM_BYTEVECTOR_CONTENTS(bv) instead of (SCM_BYTEVECTOR_CONTENTS(bv) + offset) in the table. I'm not sure why it keeps (... + offset). Does this make sense?

So I'd be interested in trying out GC_MALLOC_ATOMIC_IGNORE_OFF_PAGE, if this fix to bytevector->pointer makes sense and no one can point out another *concrete* situation where it would result in a GC bug.

Regards

	Daniel