[MLton] Crash fread(...) failed (only read 0) (Cannot allocate memory) during deepFlatten with MLton 20070826

[Matthew Fluet]

On Mon, 5 May 2008, Nicolas Bertolotti wrote:
> I tried a number of different solutions. It seems that moving the 
> GC_generationalMaps struct to the GC_heap struct requires lots of 
> changes in different parts of the code and would also lead to some 
> useless computations when we manipulate the secondary heap (either 
> adding some code to determine that we don't need to manipulate the 
> card/cross map in that case or manipulate it anyway).
>
> I finally implemented a solution in which the data structures don't 
> change and which is less intrusive.

For future reference, it is much easier to evaluate a patch that applies 
cleanly to SVN HEAD.

After looking at your patch and considering alternatives, I have to agree 
that moving the GC_generationalMaps struct into the GC_heap struct would 
probably be fairly intrusive.

> While I was debugging this solution, it appeared that the function 
> growHeap() may actually shrink the heap when the minimum size is less 
> than the current size which, I guess, is not the expected behavior.

There is no bug in shrinking the size of the heap (under the original 
code), but I agree that it is counter intuitive.  If the desired heap size 
is greater than the current heap size, then there seems to be no reason to 
resize the heap to one smaller than the current heap size.

> Using this patch, I don't have any sporadic crashes when building my 
> application (or MLton itself) on the 4 Gb debian box.
>
> I am interested in getting some feed-back about this patch so don't 
> hesitate.

--- mlton/runtime/gc/heap.c	2008-04-11 19:12:27.000000000 +0200
+++ mltonp1/runtime/gc/heap.c	2008-05-05 01:37:57.000000000 +0200
@ -299,21 +335,26 @@
      from = curHeapp->start + size;
      to = newHeap.start + size;
      remaining = size;
+    copyCardMapAndCrossMap(s, &newHeap);
+    GC_decommit(orig + curHeapp->size, computeCardMapAndCrossMapSize(s, curHeapp->size));
  copy:
      assert (remaining == (size_t)(from - curHeapp->start)
              and from >= curHeapp->start
              and to >= newHeap.start);
      if (remaining < COPY_CHUNK_SIZE) {
        GC_memcpy (orig, newHeap.start, remaining);
+      GC_release (orig, curHeapp->size);
      } else {
+      size_t keep;
        remaining -= COPY_CHUNK_SIZE;
        from -= COPY_CHUNK_SIZE;
        to -= COPY_CHUNK_SIZE;
        GC_memcpy (from, to, COPY_CHUNK_SIZE);
-      shrinkHeap (s, curHeapp, remaining);
+      keep = align (remaining, s->sysvals.pageSize);
+      GC_decommit (orig + keep, orig + curHeapp->size);
+      curHeapp->size = keep;
        goto copy;
      }
-    releaseHeap (s, curHeapp);
      newHeap.oldGenSize = size;
      *curHeapp = newHeap;
    } else {

This seem suspicious:
+      GC_decommit (orig + keep, orig + curHeapp->size);
The second argument should be the size of the region to be released.  It 
seems that the line should be more like:
+      GC_decommit (curHeapp->start + keep, curHeapp->size - keep);
which corresponds to the behavior of the shrinkHeap function in the 
original code.

> Last point: the function growHeap() stores the current heap to disk when 
> it can not allocate enough memory for the new heap. As this function is 
> sometimes called with a minimum size that is less than the current heap 
> size and this operation is quite costly, don't you think we could avoid 
> it and keep the heap as is in that case?

It is not as simple as leaving the heap alone if we would otherwise page 
to disk.  Note that after failing to remap the current heap to a new size 
(via remapHeap), the growHeap function shrinks the current heap (via 
shrinkHeap (s, curHeapp, curHeapp->oldGenSize)) in order to free memory 
for createHeap to allocate a heap of the desired size.  If, while 
retaining the live data in the current heap, we can't allocate a new heap 
of the minimum size, then we can't simply keep the current heap, because 
it will have been shrunk below the minimum size.  (The minimum size will 
be the size of the live data in the current heap plus some size for new 
allocations.)  Note that it would be futile to try to remap the current 
heap back to the minimum size -- if the platform had mremap, then the 
original remapHeap wouldn't have failed, because the minimum size is less 
than the current heap, so after sufficient backoffs we would call mremap 
with a new size less then the old size, which should succeed.

It would be possible to not shrink the current heap before attempting to 
allocate a new heap of the desired size, but that runs the risk of not 
freeing the memory that would allow us to allocate a heap of the desired 
size.