munificent / craftinginterpreters

Repository for the book "Crafting Interpreters"
http://www.craftinginterpreters.com/
Other
8.87k stars 1.04k forks source link

GC - collectGarbage() called recursively #1128

Closed pumkinpal closed 2 months ago

pumkinpal commented 1 year ago

I've observed that collectGarbage() can be called recursively during the sweep phase as freeing objects calls reallocate() while vm.bytesAllocated is still greater than vm.nextGC, which is only updated after the sweep phase ends.

Updating vm.nextGC before calling markRoots() in collectGarbage() seems to solve this issue as it ensures that vm.nextGC won't be less than vm.bytesAllocated throughout the sweep, preventing reallocate() from calling collectGarbage().

void collectGarbage()
{
#ifdef CLOX_DEBUG_LOG_GC
    printf("-- gc begin\n");
    size_t before = vm.bytesAllocated;
#endif

    // Update nextGC before mark and sweep to ensure GC isn't re-triggered by freeing memory
    vm.nextGC = vm.bytesAllocated;

    markRoots();
    traceReferences();
    tableRemoveWhite(&vm.strings);
    sweep();

    vm.nextGC = vm.bytesAllocated * CLOX_GC_HEAP_GROW_FACTOR;

#ifdef CLOX_DEBUG_LOG_GC
    printf("-- gc end\n");
    printf("   collected %zu bytes (from %zu to %zu) next at %zu\n",
        before - vm.bytesAllocated, before, vm.bytesAllocated,
        vm.nextGC);
#endif
}
mwwwa commented 2 months ago

Hi @pumkinpal

I ran into the same issue, and found out that I made a mistake writing the code in reallocate().

My mistake was to not nest the two ifs:

    vm.bytesAllocated += newSize - oldSize;
    if (newSize > oldSize) {
#ifdef DEBUG_STRESS_GC
        collectGarbage();
#endif
    }
    if (vm.bytesAllocated > vm.nextGC) {
        collectGarbage();
    }
    if (newSize == 0) {

But in the source, the second if is nested inside the first if. During the sweep phase, objects only get freed, so newSize is 0, which is never > oldSize. Thus bytesAllocated isn't even compared with nextGC and the recursive call is not made.

Hope that helps 😊