-
Notifications
You must be signed in to change notification settings - Fork 996
runtime (gc): add a new "list" GC #2004
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Size comparison: |
aykevl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the code (not super in-depth though) and it looks reasonable to me. Thank you for this effort! This is indeed much easier to review when it's separated from a precise GC (which will be quite a bit of work and may also be useful for -gc=conservative).
Can you add a test for this GC? You can add a test in main_test.go, see the opt=1, opt=0, and ldflags tests.
| // sort the allocation list in ascending address order. | ||
| func (list *allocList) sort() { | ||
| // Sort the list by repeatedly moving the lowest-address element to the front. | ||
| // This is selection sort (https://en.wikipedia.org/wiki/Selection_sort). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly worried that a quadratic algorithm in a GC might be a problem. Maybe not on the AVR though with its very limited memory.
I think there is a way to avoid this sorting, at least if it's fine to limit the maximum object size to half the address space (which seems like a reasonable limitation to me). You can use one bit of allocHeader.len (e.g. the top bit) to store the mark state. Then, during sweeping, you can iterate through all objects and remove the ones that haven't been marked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just an idea for how it can be improved. The code as-is already looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think this may be better as-is. That tagging strategy would actually also make the scan quadratic (the list needs to be fully traversed each time we need to find what to scan). I think the current strategy with the scan stack is at least a bit more readable.
| } | ||
|
|
||
| // Search the list of unmarked allocations for this root address. | ||
| alloc := activeAllocs.search(root) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also looks like a quadratic algorithm here: for every possible root, all allocations are scanned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately yes. Not sure if this can/should be changed. This worked much better with the precise GC because there were far fewer roots to deal with. This is also fairly reasonable on AVR where I don't really see more than a dozen allocations very often.
|
|
||
| // findMem searches the heap for a free span large enough to contain an allocation of the specified size. | ||
| // If there are no sufficiently large free spans available, this returns nil. | ||
| func findMem(size uintptr) *allocHeader { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things I realize now:
-
The memory should be aligned. You can use the
alignfunction for this. This is a no-op on AVR and should be optimized away entirely. On other platforms, it is necessary for correct functioning.size = align(size)
-
You might want to use a
zeroSizedAllocglobal (like in gc_conservative.go) to return for when the size is zero. This does sometimes happen in practice, although it is rare. The Go spec explicitly allows this:Pointers to distinct zero-size variables may or may not be equal.
On the other hand, it's rare and costs RAM/flash, so it would be reasonable to leave this out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the 0-sized alloc in because it only costs 1 byte of RAM (and saves 28 bytes of flash for some reason).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not entirely sure what you mean with the alignment. Assuming that heapStart is aligned (which I picked up from the conservative collector), findMem should always return appropriately aligned data. The header preserves pointer/word alignment, and every subsequent start is corrected with an align call.
This memory manager uses a simple linked list to track active allocations, and measures the gaps between them to find available memory. Performance is not great for large heaps, but is sufficient for microcontrollers with single-digit kilobytes of memory.
|
@aykevl I am currently planning to do some GC cleanup to try to ensure that all of the conservative GCs behave similarly. Should we merge this first or should I push this to later? |
|
Either way, please note merge conflicts @niaow |
|
I am going to close this. This GC has too many issues and we have other priorities right now. |
This memory manager uses a simple linked list to track active allocations, and measures the gaps between them to find available memory. Performance is not great for large heaps, but is sufficient for microcontrollers with single-digit kilobytes of memory.
This is effectively a simplified version of #1193 containing only the GC itself so that it is easier to review.
The
./testdata/gc.gotest was run on an Arduino Uno and passes. It is slow.The false-positive rate problem still applies to microcontrollers where the address space is mostly used.