Operation of HeapMakeSpace() under Glulx?

I’ve been looking at the code underlying the heap memory management, and I have a question about routine HeapMakeSpace():

[ HeapMakeSpace size multiple  newblocksize newblock B n;
    for (::) {
	    if (multiple) {
		    if (HeapNetFreeSpace(multiple) >= size) rtrue;
	    } else {
		    if (HeapLargestFreeBlock(0) >= size) rtrue;
	    }
	    newblocksize = 1;
	    for (n=0: (n<SMALLEST_BLK_WORTH_ALLOCATING) || (newblocksize<size): n++)
		    newblocksize = newblocksize*2; ! double until >= requested *data* size or 4096
	    while (newblocksize < size+16) newblocksize = newblocksize*2; ! double once more if can't accommodate worst-case header?
	    newblock = VM_AllocateMemory(newblocksize);
	    if (newblock == 0) rfalse;
	    newblock->BLK_HEADER_N = n; ! 2^n < size if extra doubling to accommodate worst-case header??
	    newblock-->BLK_HEADER_KOV = 0;
	    newblock-->BLK_HEADER_RCOUNT = 0;
	    newblock->BLK_HEADER_FLAGS = BLK_FLAG_MULTIPLE;
	    newblock-->BLK_NEXT = NULL;
	    newblock-->BLK_PREV = NULL;
	    for (B = Flex_Heap-->BLK_NEXT:B ~= NULL:B = B-->BLK_NEXT)
		    if (B-->BLK_NEXT == NULL) {
			    B-->BLK_NEXT = newblock;
			    newblock-->BLK_PREV = B;
			    jump Linked;
		    }
	    Flex_Heap-->BLK_NEXT = newblock;
	    newblock-->BLK_PREV = Flex_Heap;
	    .Linked; ;
	    #ifdef BLKVALUE_TRACE;
	    print "Increasing heap to free space map: "; FlexDebugDecomposition(Flex_Heap, 0);
	    #endif;
    }
    rtrue;
];

As noted in the added comments above, it seems like there might be a problem with the way that the “raw” block size (-->BLK_HEADER_N) value is being set in cases where an extra doubling is required to accommodate an extra 16 bytes (i.e. BLK_DATA_MULTI_OFFSET?) in addition to the requested size parameter. I am assuming that the size parameter means the requested usable “data” size (i.e. what can be stored in the block), not the “raw” size (i.e. bytes required by the block for data plus block header).

For example, if requesting 8190 data bytes, the for loop will leave n at 13 (2^13 == 8192 >= 8190) and leave newblocksize at 8192, but the immediately following while loop (which seems like it can have at most one iteration) will increase newblocksize to 16384 without also increasing n to 14.

This can only matter in Glulx, because VM_AllocateMemory() always returns zero in Z-Machine, and the routine returns false because no new block can be created. Does this mismatch somehow not matter in the context of Glulx? It seems to me like it would be establishing the new block with a too-small size indicator in any case, which would provide misleading results from routine FlexSize() (and the many routines that depend upon it).

2 Likes

Looking at this further, there are some more potentially problematic aspects:

First, the value of BLK_DATA_MULTI_OFFSET under Glulx is currently 20, not 16, though I think that it may have once been defined as 16, as that’s what the documentation in Flex.i6t says is the worst case.

Second, it seems like the issue above might have a bad interaction with routine FlexAllocate():

[ FlexAllocate size kov flags
    dsize n m free_block min_m max_m smallest_oversized_block secondhalf i hsize head tail;
    
    if (HeapMakeSpace(size, flags & BLK_FLAG_MULTIPLE) == false) FlexError("ran out"); ! what if too-small indicator on obtained block?

    ! Calculate the header size for a block of this KOV
    if (flags & BLK_FLAG_MULTIPLE) hsize = BLK_DATA_MULTI_OFFSET;
    else hsize = BLK_DATA_OFFSET;
    ! Calculate the data size
    n=0; for (dsize=1: ((dsize < hsize+size) || (n<3+(WORDSIZE/2))): dsize=dsize*2) n++; ! dsize for size 8190 is 16384? / odd minimum calc

    ! Seek a free block closest to the correct size, but starting from the
    ! block after the fixed head-free-block, which we can't touch
    min_m = 10000; max_m = 0;
    for (free_block = Flex_Heap-->BLK_NEXT:
	    free_block ~= NULL:
	    free_block = free_block-->BLK_NEXT) {
	    m = free_block->BLK_HEADER_N; ! evaluates to too-small value for new block obtained by HeapMakeSpace()?
	    ! Current block the ideal size
	    if (m == n) jump CorrectSizeFound;
	    ! Current block too large: find the smallest which is larger than needed
	    if (m > n) {
		    if (min_m > m) {
			    min_m = m;
			    smallest_oversized_block = free_block;
		    }
	    }
	    ! Current block too small: find the largest which is smaller than needed
	    if (m < n) { ! this case would apply to new block obtained?
		    if (max_m < m) {
			    max_m = m;
		    }
	    }
    }

    if (min_m == 10000) {
	    ! Case I: No block is large enough to hold the entire size
	    if (flags & BLK_FLAG_MULTIPLE == 0) FlexError("too fragmented"); ! this case would apply to new block obtained? ! LAST COMMENT
	    ! Set dsize to the size in bytes if the largest block available
	    for (dsize=1: max_m > 0: dsize=dsize*2) max_m--;
	    ! Split as a head (dsize-hsize), which we can be sure fits into one block,
	    ! plus a tail (size-(dsize-hsize), which might be a list of blocks
	    head = FlexAllocate(dsize-hsize, kov, flags);
	    if (head == 0) FlexError("for head block not available");
	    tail = FlexAllocate(size-(dsize-hsize), kov, flags);
	    if (tail == 0) FlexError("for tail block not available");
	    head-->BLK_NEXT = tail;
	    tail-->BLK_PREV = head;
	    return head;
    }

    ! Case II: No block is the right size, but some exist which are too big
    ! Set dsize to the size in bytes of the smallest oversized block
    for (dsize=1,m=1: m<=min_m: dsize=dsize*2) m++;
    free_block = smallest_oversized_block;
    while (min_m > n) {
	    ! Repeatedly halve free_block at the front until the two smallest
	    ! fragments left are the correct size: then take the frontmost
	    dsize = dsize/2;
	    ! print "Halving size to ", dsize, "^";
	    secondhalf = free_block + dsize;
	    secondhalf-->BLK_NEXT = free_block-->BLK_NEXT;
	    if (secondhalf-->BLK_NEXT ~= NULL)
		    (secondhalf-->BLK_NEXT)-->BLK_PREV = secondhalf;
	    secondhalf-->BLK_PREV = free_block;
	    free_block-->BLK_NEXT = secondhalf;
	    free_block->BLK_HEADER_N = (free_block->BLK_HEADER_N) - 1;
	    secondhalf->BLK_HEADER_N = free_block->BLK_HEADER_N;
	    secondhalf-->BLK_HEADER_KOV = free_block-->BLK_HEADER_KOV;
	    secondhalf-->BLK_HEADER_RCOUNT = 0;
	    secondhalf->BLK_HEADER_FLAGS = free_block->BLK_HEADER_FLAGS;
	    min_m--;
    }
    
    ! Once that is done, free_block points to a block which is exactly the
    ! right size, so we can fall into...
    
    ! Case III: There is a free block which has the correct size.
    .CorrectSizeFound;
    ! Delete the free block from the double linked list of free blocks: note
    ! that it cannot be the head of this list, which is fixed
    if (free_block-->BLK_NEXT == NULL) {
	    ! We remove final block, so previous is now final
	    (free_block-->BLK_PREV)-->BLK_NEXT = NULL;
    } else {
	    ! We remove a middle block, so join previous to next
	    (free_block-->BLK_PREV)-->BLK_NEXT = free_block-->BLK_NEXT;
	    (free_block-->BLK_NEXT)-->BLK_PREV = free_block-->BLK_PREV;
    }
    free_block-->BLK_HEADER_KOV = KindAtomic(kov);
    free_block-->BLK_HEADER_RCOUNT = 1;
    free_block->BLK_HEADER_FLAGS = flags;
    if (flags & BLK_FLAG_MULTIPLE) {
	    free_block-->BLK_NEXT = NULL;
	    free_block-->BLK_PREV = NULL;
    }
    
    ! Zero out the data bytes in the memory allocated
    for (i=hsize:i<dsize:i++) free_block->i=0;
    return free_block;
];

Looking over the commented lines above (ending with LAST COMMENT), it seems as though the freshly-obtained but mislabeled new free block would not meet the size requirement calculated for dsize, e.g. 16384 for a requested size parameter of 8190. (Note that in this routine the size parameter means still seems to mean the number of bytes requested for data storage.) Wouldn’t this mean that the routine will see no satisfactory blocks available and respond with a “too fragmented” RTE?

Third, the way that a minimum value for dsize is calculated, the expression

(n<3+(WORDSIZE/2))

evaluates to 4 (i.e. 2^4 = 16) for Z-Machine and 5 (i.e. 2^5 = 32) for Glulx. These are fine at present, but they don’t seem future-proof in the event of further changes to the block header. Something like:

n=0; for (dsize=1: ((dsize < hsize+size) || (IncreasingPowersOfTwo_TB-->n < (BLK_DATA_MULTI_OFFSET+WORDSIZE))): dsize=dsize*2) n++;

might do the trick?

EDIT: Logic adjusted so that minimum size is governed by BLK_DATA_MULTI_OFFSET+WORDSIZE because any free block resulting from a split will need to be a multiple block of the same size, even if a single block is being requested. On Z-Machine, it’s possible to fit a 1-word short block into a block of size 8, but it’s not possible to fit the multiple block header into a block of size 8, so splitting a free size 16 block isn’t possible under the existing system.

1 Like

This definitely is a real problem. Here’s some code to demonstrate the issue:

"6M62 Heap Expansion Issue Under Glulx"

Place is a room.

To allocate a dead block of size (N - number):
    (- FlexAllocate({N}, 0); -).

To allocate a heap-consuming block:
    (- FlexAllocate(MEMORY_HEAP_SIZE-16, 0); -).

To heap debug full:
    (- HeapDebug(true); -).

When play begins:
    say "STARTUP[line break]";
    heap debug full;
    allocate a heap-consuming block;
    say "[line break]FULLY USED[line break]";
    heap debug full;
    [for following, size values in range 4085 to 4096 will demonstrate the problem]
    [similarly for values V and N such that (2^N - BLK_DATA_OFFSET) < V < 2^N  for any N > 12; note BLK_DATA_OFFSET == 12]
    allocate a dead block of size 1;
    say "[line break]REQUESTED ADDITIONAL[line break]";
    heap debug full.

Test me with "showheap".

Adding the following to your code will correct the issue in 6M62:

Include (- Replace HeapMakeSpace; -) before "Flex.i6t".

Include (-

[ HeapMakeSpace size multiple  newblocksize newblock B n hsize; ! MODIFIED
        for (::) {
                if (multiple) {
		    hsize = BLK_DATA_MULTI_OFFSET; ! ADDED
                        if (HeapNetFreeSpace(multiple) >= size) rtrue;
                } else {
		    hsize = BLK_DATA_OFFSET; ! ADDED
                        if (HeapLargestFreeBlock(0) >= size) rtrue;
                }
                newblocksize = 1;
                for (n=0: (n<SMALLEST_BLK_WORTH_ALLOCATING) || (newblocksize<(size+hsize)): n++) ! MODIFIED
                        newblocksize = newblocksize*2;
                ! while (newblocksize < size+hsize) { n++; newblocksize = newblocksize*2; } ! DELETED
                newblock = VM_AllocateMemory(newblocksize);
                if (newblock == 0) rfalse;
                newblock->BLK_HEADER_N = n;
                newblock-->BLK_HEADER_KOV = 0;
                newblock-->BLK_HEADER_RCOUNT = 0;
                newblock->BLK_HEADER_FLAGS = BLK_FLAG_MULTIPLE;
                newblock-->BLK_NEXT = NULL;
                newblock-->BLK_PREV = NULL;
                for (B = Flex_Heap-->BLK_NEXT:B ~= NULL:B = B-->BLK_NEXT)
                        if (B-->BLK_NEXT == NULL) {
                                B-->BLK_NEXT = newblock;
                                newblock-->BLK_PREV = B;
                                jump Linked;
                        }
                Flex_Heap-->BLK_NEXT = newblock;
                newblock-->BLK_PREV = Flex_Heap;
                .Linked; ;
                #ifdef BLKVALUE_TRACE;
                print "Increasing heap to free space map: "; FlexDebugDecomposition(Flex_Heap, 0);
                #endif;
        }
        rtrue;
];

-) after "Make Space" in "Flex.i6t".

EDIT: Updated to include the line closing the inclusion and indicating placement. Thanks to Zed for spotting this error!

If anybody with write access to the Friends of I7 patches extension (extensions/6M62 Patches.i7x at master · i7/extensions · GitHub) wants to add the above, that’s fine by me. (See also the next post for the correction to the hardcoded 16 issue mentioned above.)

This issue and the one discussed in the following post (see below) are corrected in the extension that I mentioned at Constant MEMORY_HEAP_SIZE -- what drives its value?, as well.

2 Likes

The following demonstrates that the current definition of HeapInitialise() in Flex.i6t sets up the first free block of the heap in a location that overwrites the value of -->BLK_NEXT for the head free block:

"6M62 Heap Setup Issue Under Glulx"

Place is a room.

L is a list of numbers that varies.

To decide which number is head free block prev pointer:
    (- Flex_Heap-->BLK_PREV -).

To decide which number is head free block next pointer:
    (- Flex_Heap-->BLK_NEXT -).

To decide which number is first free block size:
    (- (Flex_Heap-->BLK_NEXT)->BLK_HEADER_N -).

To decide which number is first free block flags:
    (- (Flex_Heap-->BLK_NEXT)->BLK_HEADER_FLAGS -).

To decide which number is hfb prev byte 0:
    (- (Flex_Heap->((BLK_PREV*WORDSIZE)+0)) -).

To decide which number is hfb prev byte 1:
    (- (Flex_Heap->((BLK_PREV*WORDSIZE)+1)) -).

To decide which number is hfb prev byte 2:
    (- (Flex_Heap->((BLK_PREV*WORDSIZE)+2)) -).

To decide which number is hfb prev byte 3:
    (- (Flex_Heap->((BLK_PREV*WORDSIZE)+3)) -).

To heap debug full:
    (- HeapDebug(true); -).

To decide which number is head free block location:
    (- Flex_Heap -).

To decide which number is first usable free block location:
    (- Flex_Heap-->BLK_NEXT -).

To decide which number is multi block header offset:
    (- BLK_DATA_MULTI_OFFSET -).

When play begins:
    say "BEFORE ALLOCATIONS.";
    say "HFB = [head free block location].";
    say "FUFB = [first usable free block location].";
    say "multi offset = [multi block header offset].";
    say "diff of block addresses = [first usable free block location minus head free block location]. (!)[line break]";
    say "HFB-->BLK_NEXT = [head free block next pointer]."; [OK]
    say "HFB-->BLK_PREV = [head free block prev pointer]. (should be -1/NULL)[line break]"; [value gets stomped by kov and flags for first usable free block]
    say "HFB-->BLK_PREV byte 0 = [hfb prev byte 0].";
    say "HFB-->BLK_PREV byte 1 = [hfb prev byte 1].";
    say "HFB-->BLK_PREV byte 2 = [hfb prev byte 2].";
    say "HFB-->BLK_PREV byte 3 = [hfb prev byte 3].";
    say "FUFB->BLK_HEADER_N = [first free block size] (exponent).";
    say "FUFB->BLK_HEADER_FLAGS = [first free block flags] (bitflags).";
    heap debug full;
    say line break;
    repeat with X running from 1 to 100:
	    add X to L;
    say "AFTER ALLOCATIONS.";
    say "HFB = [head free block location].";
    say "FUFB = [first usable free block location]."; [notice change after allocation of a block at front of former first free block]
    say "HFB-->BLK_NEXT = [head free block next pointer]."; [updated due to allocation]
    say "HFB-->BLK_PREV = [head free block prev pointer]. (note changed value!)"; [presumably overwritten to kov/flags of block allocated to L]
    say "HFB-->BLK_PREV byte 0 = [hfb prev byte 0].";
    say "HFB-->BLK_PREV byte 1 = [hfb prev byte 1].";
    say "HFB-->BLK_PREV byte 2 = [hfb prev byte 2].";
    say "HFB-->BLK_PREV byte 3 = [hfb prev byte 3].";
    heap debug full.

This can be corrected by adding the following to your source:

Include (-
Array Flex_Heap -> MEMORY_HEAP_SIZE + BLK_DATA_MULTI_OFFSET; ! allow room for head-free-block
-) instead of "The Heap" in "Flex.i6t".

Include (-

[ HeapInitialise n bsize blk2;
        blk2 = Flex_Heap + BLK_DATA_MULTI_OFFSET; ! MODIFIED
        Flex_Heap->BLK_HEADER_N = 4;
        Flex_Heap-->BLK_HEADER_KOV = 0;
        Flex_Heap-->BLK_HEADER_RCOUNT = MAX_POSITIVE_NUMBER;
        Flex_Heap->BLK_HEADER_FLAGS = BLK_FLAG_MULTIPLE;
        Flex_Heap-->BLK_NEXT = blk2;
        Flex_Heap-->BLK_PREV = NULL;
        for (bsize=1: bsize < MEMORY_HEAP_SIZE: bsize=bsize*2) n++;
        blk2->BLK_HEADER_N = n;
        blk2-->BLK_HEADER_KOV = 0;
        blk2-->BLK_HEADER_RCOUNT = 0;
        blk2->BLK_HEADER_FLAGS = BLK_FLAG_MULTIPLE;
        blk2-->BLK_NEXT = NULL;
        blk2-->BLK_PREV = Flex_Heap;
];

-) instead of "Initialisation" in "Flex.i6t".