Fix stale forwarding bits bug #777
Conversation
When copying, we erroneously assumed that the forwarding bits of the new copy must not have been set if the forwarding bits metadata is on the side. If the new copy is allocated in a previously free chunk, it may contain stale forwarding bits from the previous GC. Now we clear forwarding bits in post_copy regardless whether the forwarding bits metadata is in the header or on the side. Given that post_copy always clears forwarding bits, PrepareBlockState now only clears forwarding bits for defrag source blocks of ImmixSpace. Closes: mmtk#776
| @@ -808,6 +802,18 @@ impl<VM: VMBinding> GCWork<VM> for PrepareBlockState<VM> { | |||
| block.set_state(BlockState::Unmarked); | |||
| debug_assert!(!block.get_state().is_reusable()); | |||
| debug_assert_ne!(block.get_state(), BlockState::Marked); | |||
| // Clear forwarding bits if necessary. | |||
| if is_defrag_source { | |||
| if let MetadataSpec::OnSide(side) = *VM::VMObjectModel::LOCAL_FORWARDING_BITS_SPEC { | |||
There was a problem hiding this comment.
This moves the bzero code from reset_object_mark() which does bzero unconditionally for the entire chunk to here, which is conditional (is_defrag_source and not Unallocated), and zeroed at block granularity. Is this an optimization?
There was a problem hiding this comment.
Yes. This is an optimisation. It will still be correct if we zero entire chunks unconditionally.
Previously, it was necessary to clear all forwarding bits of all chunks because post_copy assumed all forwarding bits for all chunks were cleared. Since post_copy now unconditionally clears the forwarding bits of new copies, we no longer need to clear the forwarding bits of all chunks. We only need to ensure all objects that can be moved have their forwarding bits (of the old copies) cleared before copying them.
An alternative solution is letting PrepareBlockState clear all forwarding bits of all Allocated and Free chunks so that post_copy can still elide the clearing of forwarding bits. That would be too costly because very few blocks are defrag sources. If the number of copied objects is small, it should be faster to clear the forwarding bits for less blocks in PrepareBlockState and clear the forwarding bits for more objects in post_copy.
There was a problem hiding this comment.
There was a problem hiding this comment.
Sure. I'll mention it when merging.
qinsoon
added
the
PR-testing
|
The Julia tests failed. But it seems to be a network issue. I don't think it is related with this PR. |
When copying, we erroneously assumed that the forwarding bits of the new copy must not have been set if the forwarding bits metadata is on the side. That assumption was introduced in mmtk@d341506 as an optimisation to reduce the number of forwarding bits clearing. However, it is unsound. If the new copy is allocated in a previously free chunk, it may contain stale forwarding bits from the previous GC. Now we clear forwarding bits in post_copy regardless whether the forwarding bits metadata is in the header or on the side. Given that post_copy always clears forwarding bits, PrepareBlockState now only clears forwarding bits for defrag source blocks of ImmixSpace. Closes: mmtk#776
When copying, we erroneously assumed that the forwarding bits of the new copy must not have been set if the forwarding bits metadata is on the side. That assumption was introduced in mmtk@d341506 as an optimisation to reduce the number of forwarding bits clearing. However, it is unsound. If the new copy is allocated in a previously free chunk, it may contain stale forwarding bits from the previous GC. Now we clear forwarding bits in post_copy regardless whether the forwarding bits metadata is in the header or on the side. Given that post_copy always clears forwarding bits, PrepareBlockState now only clears forwarding bits for defrag source blocks of ImmixSpace. Closes: mmtk#776
When copying, we erroneously assumed that the forwarding bits of the new copy must not have been set if the forwarding bits metadata is on the side. That assumption was introduced in mmtk@d341506 as an optimisation to reduce the number of forwarding bits clearing. However, it is unsound. If the new copy is allocated in a previously free chunk, it may contain stale forwarding bits from the previous GC. Now we clear forwarding bits in post_copy regardless whether the forwarding bits metadata is in the header or on the side. Given that post_copy always clears forwarding bits, PrepareBlockState now only clears forwarding bits for defrag source blocks of ImmixSpace. Closes: mmtk#776 (cherry picked from commit 4cefff7)
When copying, we erroneously assumed that the forwarding bits of the new copy must not have been set if the forwarding bits metadata is on the side. If the new copy is allocated in a previously free chunk, it may contain stale forwarding bits from the previous GC.
Now we clear forwarding bits in post_copy regardless whether the forwarding bits metadata is in the header or on the side.
Given that post_copy always clears forwarding bits, PrepareBlockState now only clears forwarding bits for defrag source blocks of ImmixSpace.
Closes: #776