Start address passed to Space::grow_space does not necessarily need to be chunk-aligned

[wenyuzhao]

Consider the following scenario:

In LargeObjectSpace, allocate one 5MB object and calls allocate_contiguous_chunks during allocation.
The allocated object is guaranteed to be chunk-aligned. highwater_mark is updated to the start address of the 1st chunk at here.
The subsequent grow_space then receives a chunk-aligned start address.

     1st chunk         2nd chunk
[aaaaaaaaaaaaaaaa] [aaaaaaaa--------]
 ^
 highwater_mark

Then allocate another 1MB object.
highwater_mark will be updated to some chunk-unaligned address within the 2nd chunk.
So under this case, the subsequent grow_space will receive an unaligned start address.

     1st chunk         2nd chunk
[aaaaaaaaaaaaaaaa] [aaaabbbb--------]
                        ^
                        highwater_mark

---

So either the chunk alignment check in grow_space is incorrect, or we should update highwater_mark to the end address of the object allocation, instead of the start address.

[qinsoon]

That assertion only checks the address is chunk aligned when new_chunk = true. The assertion tries to make sure that we do not overwrite the SFT entry (e.g. in your example, the SFT for 2nd chunk should only be set once). A weaker assertion could be to make sure we do not overwrite SFT to a different value.

In your example, the first allocation should initialize SFT map for both chunks, and the second allocation should not return new_chunk = true as it is not a new chunk. Maybe we should use the end of an object as highwater_mark.

[wenyuzhao]

That assertion only checks the address is chunk aligned when new_chunk = true. The assertion tries to make sure that we do not overwrite the SFT entry (e.g. in your example, the SFT for 2nd chunk should only be set once). A weaker assertion could be to make sure we do not overwrite SFT to a different value.

In your example, the first allocation should initialize SFT map for both chunks, and the second allocation should not return new_chunk = true as it is not a new chunk. Maybe we should use the end of an object as highwater_mark.

Well, the second case will have new_chunk = true. The updating of highwater_mark crosses the chunk boundary. As checked here.

[wenyuzhao]

@qinsoon Please use this patch (and run ./examples/build.py) to reproduce the bug.

[qinsoon]

@qinsoon Please use this patch (and run ./examples/build.py) to reproduce the bug.

Thanks a lot. Does this block your work? Is this high priority?

[wenyuzhao]

@qinsoon Please use this patch (and run ./examples/build.py) to reproduce the bug.

Thanks a lot. Does this block your work? Is this high priority?

Not quite urgent. I have quite a lot stuff to upstream/fix and this is only one of them. I can upstream others first.

[qinsoon]

The assertion has been removed in #461. I realized the actual problem is the SFTMap.update() function, which takes start and chunks as arguments. If start is not chunk aligned, it will be aligned down to the nearest chunk, and only set the next given number of chunks (specified by the chunks arg). So if start is not aligned to chunk, you may end up setting fewer chunks than you would expect.

I have a fix for this in #476. We should be able to close this issue once the PR is merged.