Add a stop-the-world, serial Compressor #1340
Conversation
|
The current failure to compile |
|
Thanks for the PR @no-defun-allowed! The major thing I see is that currently the Compressor plan is not using the LOS. Was this intentional or accidental? |
Definitely accidental. Thanks for the review. |
You can cherry pick this commit to the PR: qinsoon@ac90d4b. It runs mock VM with side metadata. Compressor currently fails for tests that are not using contiguous space. |
Thanks!
Right. Should we skip such tests when running with the Compressor? |
|
|
||
| pub fn mark_end_of_object(&self, object: ObjectReference) { | ||
| let end_of_object = | ||
| object.to_raw_address() + VM::VMObjectModel::get_current_size(object) - MIN_OBJECT_SIZE; |
There was a problem hiding this comment.
Why do we need to minus MIN_OBJECT_SIZE? It looks like you try to compute an address that is guaranteed inside the object.
Also the start of the allocation should be object.to_object_start::<VM>() rather than object.to_raw_address() if you want to add the size to it.
There was a problem hiding this comment.
You need to set the bit at the end of the object. Consider an object at 0x10000000 of size 0x10, then we want the mark bits to be set for 0x10000000 and 0x1000000f, not 0x10000000 and 0x10000010. Since each machine word (8-byte on 64-bits and 4-byte on 32-bits) maps down to a single bit, it's sufficient to subtract the minimum object size to set the bit there.
There was a problem hiding this comment.
Notably, this requires the start and end of the object to map to different bits. Hence the debug assertion in the function.
There was a problem hiding this comment.
You need to set the bit at the end of the object. Consider an object at 0x10000000 of size 0x10, then we want the mark bits to be set for 0x10000000 and 0x1000000f, not 0x10000000 and 0x10000010.
Then why not just use -1?
Since each machine word (8-byte on 64-bits and 4-byte on 32-bits) maps down to a single bit, it's sufficient to subtract the minimum object size to set the bit there.
It seems you are assuming the min object size is one word, which might not be always true in the future. You could also use -word_size.
| if KIND == TRACE_KIND_MARK { | ||
| self.trace_mark_object(queue, object) | ||
| } else if KIND == TRACE_KIND_FORWARD { | ||
| self.trace_forward_object(queue, object) |
There was a problem hiding this comment.
This looks like a second transitive closure. I assume this only happens for roots. Right? If so, I suggest making it much clearer (e.g. TRACE_KIND_FORWARD -> TRACE_KIND_FORWARD_ROOT, self.trace_forward_object -> self.forward_root, etc), and making the code more distinguishable from mark compact.
There was a problem hiding this comment.
Right and right.
Is there any intrinsic reason why the compressor cannot run with discontiguous spaces? I feel there is none. It would be better to allow running Compressor with discontiguous spaces. At the same time, there might be practical reasons that this might be not easy, or it requires too much efforts. It is also okay to skip those tests. |
| }; | ||
|
|
||
| lazy_static! { | ||
| /// When nogc_multi_space is disabled, force all the allocation go to the default allocator and space. |
There was a problem hiding this comment.
| /// When nogc_multi_space is disabled, force all the allocation go to the default allocator and space. | |
| /// When using `compressor_single_space`, force all the allocations to go to the default allocator and space. |
I feel that way too. The judgement was more about how closely we want to follow the Compressor paper - one simple approach to support discontiguous spaces would be to compress [sic] each chunk of a space, which would also give a simple way to parallelise (which is not the way in the paper). The block offset vector is currently a Rust |
This PR adds a Compressor-esque garbage collector, which is based on MarkCompact but uses a mark bitmap for forwarding metadata like the Compressor.