Avoid clearing chunks at start of GC

[dipinhora]

Prior to this commit, at the start of GC, the heap clears all
chunks by setting them all to empty in ponyint_heap_startgc.
This is an expensive operation as it requires visiting all
chunks and updating their chunk->slots and chunk->shallow
values regardless of whether they are still in use or not.
chunk->shallow is only ever used as part of the GC process.

The old logic for heap GC was:

• mark all slots in all chunks as empty so they can all be freed by GC (clear chunks)
• recursively trace all roots and mark any slots still in use as not empty so they don't get freed by GC
• sweep all chunks freeing (and running finalisers) any slots that are empty

This commit uses the chunk->shallow with a sentinel
value to avoid clearing chunks for every sizeclass except
for sizeclass 0 which is still cleared and makes sure that
the mark and sweep steps clear the chunk if
chunk->shallow has the sentinel value and needs to be
cleared. Additionally, ponyint_heap_ismarked is removed
as it is not used.

The new logic for heap GC is:

• only mark all slots in sizeclass 0 chunks as empty so they can all be freed by GC (clear chunks)
  • chunks in all other sizeclasses will already have their chunk->shallow set to a sentinel value to indicate that they need to be cleared as part of trace/sweep
• recursively trace all roots and mark any slots still in use as not empty so they don't get freed by GC
  • As part of mark, clear the chunk if the chunk->shallow has the sentinel value (to preserve the illusion that the chunk was cleared before GC was started) before proceeding with normal marking logic
• sweep all chunks freeing (and running finalisers) any slots that are empty
  • As part of sweep, clear the chunk if the chunk->shallow has the sentinel value (to preserve the illusion that the chunk was cleared before GC was started) before proceeding with normal sweeping logic
  • Also as part of sweep, set chunk->shallow to the sentinel value in prep for the next GC iteration
• modify ponyint_heap_alloc* functions to set chunk->shallow to the sentinel value in prep for the next GC iteration for all newly allocated chunks

[SeanTAllen]

Im not particularly familiar with this section of the code. It's been a long time. Why was it clearing all the chunks before? Is there some case where that is needed? Did there used to be?

[dipinhora]

Clearing chunks is absolutely essential for the heap GC process to function correctly.

The current logic is:

• mark all slots in all chunks as empty so they can all be freed by GC (clear chunks)
• recursively trace all roots and mark any slots still in use as not empty so they don't get freed by GC
• sweep all chunks freeing (and running finalisers) any slots that are empty

The new logic in this PR is:

• only mark all slots in sizeclass 0 chunks as empty so they can all be freed by GC (clear chunks)
  • all other chunks already have their chunk->shallow set to a sentinel value to indicate that they need to be cleared as part of trace/sweep
• recursively trace all roots and mark any slots still in use as not empty so they don't get freed by GC
  • As part of mark, clear the chunk if the chunk->shallow has the sentinel value (to preserve the illusion that the chunk was cleared before GC was started) before proceeding with normal marking logic
• sweep all chunks freeing (and running finalisers) any slots that are empty
  • As part of sweep, clear the chunk if the chunk->shallow has the sentinel value (to preserve the illusion that the chunk was cleared before GC was started) before proceeding with normal sweeping logic
  • Also as part of sweep, set chunk->shallow to the sentinel value in prep for the next GC iteration
• modify ponyint_heap_alloc* functions to set chunk->shallow to the sentinel value in prep for the next GC iteration for all newly allocated chunks

The misnamed Init test in test/libponyrt/mem/heap.cc tests running GC on a heap and should fail if the logic isn't correctly preserved in the new version of the code in this PR (it helped me catch a bug while i was developing this change):

ponyc/test/libponyrt/mem/heap.cc

Lines 9 to 82 in bca5a30

	TEST(Heap, Init)
	{
	pony_actor_t* actor = (pony_actor_t*)0xDEADBEEFDEADBEEF;
	heap_t heap;
	ponyint_heap_init(&heap);
	void* p = ponyint_heap_alloc(actor, &heap, 127);
	ASSERT_EQ((size_t)128, heap.used);
	chunk_t* chunk = (chunk_t*)ponyint_pagemap_get(p);
	ASSERT_EQ(actor, ponyint_heap_owner(chunk));
	size_t size = ponyint_heap_size(chunk);
	ASSERT_EQ(size, (size_t)128);
	void* p2 = ponyint_heap_alloc(actor, &heap, 99);
	ASSERT_NE(p, p2);
	ASSERT_EQ((size_t)256, heap.used);
	heap.next_gc = 0;
	ponyint_heap_startgc(&heap);
	ponyint_heap_mark(chunk, p);
	ponyint_heap_endgc(&heap);
	ASSERT_EQ((size_t)128, heap.used);
	void* p3 = ponyint_heap_alloc(actor, &heap, 107);
	ASSERT_EQ(p2, p3);
	ponyint_heap_used(&heap, 1024);
	ASSERT_EQ((size_t)1280, heap.used);
	heap.next_gc = 0;
	ponyint_heap_startgc(&heap);
	ponyint_heap_mark_shallow(chunk, p3);
	ponyint_heap_endgc(&heap);
	ASSERT_EQ((size_t)128, heap.used);
	void* p4 = ponyint_heap_alloc(actor, &heap, 67);
	ASSERT_EQ(p, p4);
	size_t large_size = (1 << 22) - 7;
	void* p5 = ponyint_heap_alloc(actor, &heap, large_size);
	chunk_t* chunk5 = (chunk_t*)ponyint_pagemap_get(p5);
	ASSERT_EQ(actor, ponyint_heap_owner(chunk5));
	size_t adjust_size = ponyint_pool_adjust_size(large_size);
	ASSERT_NE(chunk5, (chunk_t*)NULL);
	char* p5_end = (char*)p5 + adjust_size;
	char* p5_curr = (char*)p5;
	chunk_t* p5_chunk = NULL;
	while(p5_curr < p5_end)
	{
	p5_chunk = (chunk_t*)ponyint_pagemap_get(p5_curr);
	p5_curr += POOL_ALIGN;
	ASSERT_EQ(chunk5, p5_chunk);
	}
	p5_chunk = (chunk_t*)ponyint_pagemap_get(p5_end);
	ASSERT_NE(chunk5, p5_chunk);
	size_t size5 = ponyint_heap_size(chunk5);
	ASSERT_EQ(adjust_size, size5);
	ASSERT_EQ(256 + adjust_size, heap.used);
	heap.next_gc = 0;
	ponyint_heap_startgc(&heap);
	ponyint_heap_mark_shallow(chunk5, p5);
	ponyint_heap_endgc(&heap);
	ASSERT_EQ(adjust_size, heap.used);
	ponyint_heap_destroy(&heap);
	}

[SeanTAllen]

If I don't beat you to it, can you add that awesome explanation to the first comment on this PR so I can copy all of it easily when I squash and make it the commit comment?

With a bit of editing to make it flow so it's not a response to someone but just general information on the commit.

[SeanTAllen]

I'm torn on if this should get a change log entry. It's likely to have a performance impact but I don't want to devise a bs benchmark to be able to quantify.

I'm marking this as getting a changelog entry and release notes. If anyone disagrees, please let me know before this gets merged.

For the release notes, I think it should be a short entry that basically says we made the gc process more efficient without going into much technical detail.

[ponylang-main]

Hi @dipinhora,

The changelog - added label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 4143.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

[dipinhora]

i've updated the first comment of this PR and added release notes..