Mark compact #494
Mark compact #494
Conversation
qinsoon
left a comment
qinsoon
left a comment
There was a problem hiding this comment.
Thanks for the PR. Apart from introducing the mark compact plan, this PR also introduces a few necessary changes for the MC plan. However, I find the design for those needs to be improved.
- GC header for certain policies/allocators (GC header is applied to plans in the PR).
- Alloc bit flag for allocators (Alloc bit flag is implemented as a new allocator type in the PR).
- Two extra methods in the Collection trait (the new methods are unclear in the PR).
I would suggest addressing those issues first. There are also a few changes needed for the mark compact plan itself. I haven't reviewed the binding PR yet. I will review it once the core PR is ready (possibly I will need to update the binding for JikesRVM and V8 as well - depending on the changes in this PR).
2. heap corruption due to stale forwarding pointers
qinsoon
left a comment
qinsoon
left a comment
There was a problem hiding this comment.
This PR looks much better now. Apart from some minor comments, the only major change I suggest is to possibly remove get_extra_header_bytes() from Plan and remove gc_extra_header from PlanConstarints. Please see the inline comments.
src/plan/plan_constraints.rs
Outdated
| pub moves_objects: bool, | ||
| pub gc_header_bits: usize, | ||
| pub gc_header_words: usize, | ||
| pub gc_extra_header_words: usize, |
There was a problem hiding this comment.
Please refer to the comments on get_extra_header_bytes().
|
When this PR is ready, I will work on the JikesRVM/V8 change for the PR. |
qinsoon
left a comment
qinsoon
left a comment
There was a problem hiding this comment.
LGTM. It is a pretty clean implementation for mark compact. Thanks for the PR.
|
@tianleq Can you check and reply to my last two comments for this PR? I have updated the JikesRVM binding (mmtk/mmtk-jikesrvm#94) and the V8 binding (mmtk/mmtk-v8#48) for this PR. I assume once the comments are resolved, we can merge this PR. |
2 participants
mmtk-core is ready for review. There is an issue in fast path allocation in the binding and ci failed because of it. I am still looking into that.