Re: [Chicken-hackers] [PATCH] Another subtle GC problem

[Peter Bex]

On Wed, Oct 23, 2013 at 11:43:24AM +0200, Moritz Heidkamp wrote:
> looks OK and works fine AFAICT. Pushed!

Thanks!

> > When shrinking the heap, this check won't need to be run because
> > we've just done a GC, so there *should be* no pressure on
> > the nursery, so it won't get copied to the heap.  But I don't
> > fully grok this part yet, so don't take my word for it.
> 
> So this would get rid of one comparsion operation per major GC?

No, the patch adds an *extra* comparison per major GC.  I was just
really thinking out loud, because I was a little worried that the
resizing logic for adding the stack size might be broken when
*shrinking* the heap.  But like I said, I think it works due to the
fact that shrinking only happens after performing a minor GC.

Cheers,
Peter
-- 
http://www.more-magic.net

___
Chicken-hackers mailing list
Chicken-hackers@nongnu.org
https://lists.nongnu.org/mailman/listinfo/chicken-hackers

Re: [Chicken-hackers] [PATCH] Another subtle GC problem

[Moritz Heidkamp]

Hi,

Peter Bex  writes:
> I decided to take another look at the GC in an attempt to
> understand it better, and found yet another iffy line of code:
>
> When comparing the heap growth to the current heap size plus
> the stack size, two unsigned quantities are subtracted from
> eachother (size is a C_uword parameter to C_rereclaim2,
> heap_size is a size_t, which is also unsigned), and then
> compared to be less than stack_size * 2.
>
> If the heap is not growing, but shrinking, this unsigned
> subtraction will underflow and cause it to result in a
> huge value.  This happens to work, but I think the attached
> patch makes it more explicit what's really happening: it
> only does the subtraction and comparison when the new size
> is bigger than the old heap size (so we're growing the heap,
> due to memory pressure).

looks OK and works fine AFAICT. Pushed!


> When shrinking the heap, this check won't need to be run because
> we've just done a GC, so there *should be* no pressure on
> the nursery, so it won't get copied to the heap.  But I don't
> fully grok this part yet, so don't take my word for it.

So this would get rid of one comparsion operation per major GC? I'd
guess this is negligible, no?

Thanks!
Moritz

___
Chicken-hackers mailing list
Chicken-hackers@nongnu.org
https://lists.nongnu.org/mailman/listinfo/chicken-hackers

[Chicken-hackers] [PATCH] Another subtle GC problem

[Peter Bex]

Hi all,

I decided to take another look at the GC in an attempt to
understand it better, and found yet another iffy line of code:

When comparing the heap growth to the current heap size plus
the stack size, two unsigned quantities are subtracted from
eachother (size is a C_uword parameter to C_rereclaim2,
heap_size is a size_t, which is also unsigned), and then
compared to be less than stack_size * 2.

If the heap is not growing, but shrinking, this unsigned
subtraction will underflow and cause it to result in a
huge value.  This happens to work, but I think the attached
patch makes it more explicit what's really happening: it
only does the subtraction and comparison when the new size
is bigger than the old heap size (so we're growing the heap,
due to memory pressure).

When shrinking the heap, this check won't need to be run because
we've just done a GC, so there *should be* no pressure on
the nursery, so it won't get copied to the heap.  But I don't
fully grok this part yet, so don't take my word for it.

Cheers,
Peter
-- 
http://www.more-magic.net
>From 4be43f6d19445226acb467a70eac6b1a036e947f Mon Sep 17 00:00:00 2001
From: Peter Bex 
Date: Thu, 17 Oct 2013 21:23:18 +0200
Subject: [PATCH] Don't do a shady unsigned comparison, but ensure we're
 growing the heap before checking it grows enough to fit the stack

---
 runtime.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/runtime.c b/runtime.c
index 0ed18de..4189476 100644
--- a/runtime.c
+++ b/runtime.c
@@ -3296,12 +3296,13 @@ C_regparm void C_fcall C_rereclaim2(C_uword size, int 
double_plus)
   if(size < MINIMAL_HEAP_SIZE) size = MINIMAL_HEAP_SIZE;
 
   /*
-   * Heap must at least grow enough to accommodate first generation
-   * (nursery).  Because we're calculating the total heap size here
-   * (fromspace *AND* tospace), we have to double the stack size,
-   * otherwise we'd accommodate only half the stack in the tospace.
+   * When heap grows, ensure it's enough to accommodate first
+   * generation (nursery).  Because we're calculating the total heap
+   * size here (fromspace *AND* tospace), we have to double the stack
+   * size, otherwise we'd accommodate only half the stack in the tospace.
*/
-  if(size - heap_size < stack_size * 2) size = heap_size + stack_size * 2;
+  if(size > heap_size && size - heap_size < stack_size * 2)
+size = heap_size + stack_size * 2;
  
   if(size > C_maximal_heap_size) size = C_maximal_heap_size;
 
-- 
1.8.3.4

___
Chicken-hackers mailing list
Chicken-hackers@nongnu.org
https://lists.nongnu.org/mailman/listinfo/chicken-hackers