From efe819c1ce551e0cd3b257797097af8848dfe0c7 Mon Sep 17 00:00:00 2001 From: Kunal Sareen Date: Fri, 29 Apr 2022 17:05:49 +1000 Subject: [PATCH 1/3] Don't panic if forwarding word is not FORWARDED and add debug assertions Some policies (such as Immix) can leave objects inplace and can reset the forwarding word while tracing (such as when Immix is out of space in its copy allocators). In such a case, we simply want to return the current object instead of attempting to read the forwarding pointer. This commit removes our faulty assumption and assertion and adds further debug assertions for a sanity check. Closes #579 --- src/policy/immix/immixspace.rs | 34 +++++++++++++++++++++++++++++++++- src/util/object_forwarding.rs | 14 +++++++++----- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/src/policy/immix/immixspace.rs b/src/policy/immix/immixspace.rs index 3c4802f80e..d2e5968f56 100644 --- a/src/policy/immix/immixspace.rs +++ b/src/policy/immix/immixspace.rs @@ -391,11 +391,43 @@ impl ImmixSpace { debug_assert!(!super::BLOCK_ONLY); let forwarding_status = ForwardingWord::attempt_to_forward::(object); if ForwardingWord::state_is_forwarded_or_being_forwarded(forwarding_status) { - ForwardingWord::spin_and_get_forwarded_object::(object, forwarding_status) + // We lost the forwarding race as some other thread has set the forwarding word; wait + // until the object has been forwarded by the winner. Note that the object may not + // necessarily get forwarded since Immix opportunistically moves objects. + let new_object = + ForwardingWord::spin_and_get_forwarded_object::(object, forwarding_status); + #[cfg(debug_assertions)] + { + if new_object == object { + debug_assert!( + self.is_marked(object, self.mark_state) || self.defrag.space_exhausted() || Self::is_pinned(object), + "Forwarded object is the same as original object {:?} even though it should have been copied", + object, + ); + } else { + // new_object != object + debug_assert!( + !Block::containing::(new_object).is_defrag_source(), + "Block {:?} containing forwarded object {:?} should not be a defragmentation source", + Block::containing::(new_object), + new_object, + ); + } + } + new_object } else if self.is_marked(object, self.mark_state) { + // We won the forwarding race but the object is already marked so we clear the + // forwarding status and return the unmoved object + debug_assert!( + self.defrag.space_exhausted() || Self::is_pinned(object), + "Forwarded object is the same as original object {:?} even though it should have been copied", + object, + ); ForwardingWord::clear_forwarding_bits::(object); object } else { + // We won the forwarding race; actually forward and copy the object if it is not pinned + // and we have sufficient space in our copy allocator let new_object = if Self::is_pinned(object) || self.defrag.space_exhausted() { self.attempt_mark(object, self.mark_state); ForwardingWord::clear_forwarding_bits::(object); diff --git a/src/util/object_forwarding.rs b/src/util/object_forwarding.rs index 4b167cb463..c0f2a547df 100644 --- a/src/util/object_forwarding.rs +++ b/src/util/object_forwarding.rs @@ -63,12 +63,16 @@ pub fn spin_and_get_forwarded_object( if forwarding_bits == FORWARDED { read_forwarding_pointer::(object) } else { - panic!( - "Invalid forwarding state 0x{:x} 0x{:x} for object {}", + // For some policies (such as Immix), we can have interleaving such that one thread clears + // the forwarding word while another thread was stuck spinning in the above loop. + // See: https://github.com/mmtk/mmtk-core/issues/579 + debug_assert!( + forwarding_bits != BEING_FORWARDED, + "Invalid/Corrupted forwarding word {} for object {:?}", forwarding_bits, - read_forwarding_pointer::(object), - object - ) + object, + ); + object } } From e7bba53ef529165b89c268b00809bbe493ea9c9f Mon Sep 17 00:00:00 2001 From: Kunal Sareen Date: Fri, 29 Apr 2022 17:45:37 +1000 Subject: [PATCH 2/3] Fix debug asserts --- src/policy/immix/immixspace.rs | 7 ++++--- src/util/object_forwarding.rs | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/policy/immix/immixspace.rs b/src/policy/immix/immixspace.rs index d2e5968f56..b550a60a0b 100644 --- a/src/policy/immix/immixspace.rs +++ b/src/policy/immix/immixspace.rs @@ -394,6 +394,7 @@ impl ImmixSpace { // We lost the forwarding race as some other thread has set the forwarding word; wait // until the object has been forwarded by the winner. Note that the object may not // necessarily get forwarded since Immix opportunistically moves objects. + #[allow(clippy::let_and_return)] let new_object = ForwardingWord::spin_and_get_forwarded_object::(object, forwarding_status); #[cfg(debug_assertions)] @@ -401,14 +402,14 @@ impl ImmixSpace { if new_object == object { debug_assert!( self.is_marked(object, self.mark_state) || self.defrag.space_exhausted() || Self::is_pinned(object), - "Forwarded object is the same as original object {:?} even though it should have been copied", + "Forwarded object is the same as original object {} even though it should have been copied", object, ); } else { // new_object != object debug_assert!( !Block::containing::(new_object).is_defrag_source(), - "Block {:?} containing forwarded object {:?} should not be a defragmentation source", + "Block {:?} containing forwarded object {} should not be a defragmentation source", Block::containing::(new_object), new_object, ); @@ -420,7 +421,7 @@ impl ImmixSpace { // forwarding status and return the unmoved object debug_assert!( self.defrag.space_exhausted() || Self::is_pinned(object), - "Forwarded object is the same as original object {:?} even though it should have been copied", + "Forwarded object is the same as original object {} even though it should have been copied", object, ); ForwardingWord::clear_forwarding_bits::(object); diff --git a/src/util/object_forwarding.rs b/src/util/object_forwarding.rs index c0f2a547df..0b834ad6d8 100644 --- a/src/util/object_forwarding.rs +++ b/src/util/object_forwarding.rs @@ -67,8 +67,8 @@ pub fn spin_and_get_forwarded_object( // the forwarding word while another thread was stuck spinning in the above loop. // See: https://github.com/mmtk/mmtk-core/issues/579 debug_assert!( - forwarding_bits != BEING_FORWARDED, - "Invalid/Corrupted forwarding word {} for object {:?}", + forwarding_bits == FORWARDED || forwarding_bits == FORWARDING_NOT_TRIGGERED_YET, + "Invalid/Corrupted forwarding word {:x} for object {}", forwarding_bits, object, ); From 3a48eb818765b11b6650caaf2054c104dd01569b Mon Sep 17 00:00:00 2001 From: Kunal Sareen Date: Fri, 29 Apr 2022 18:15:41 +1000 Subject: [PATCH 3/3] Remove unnecessary check in debug assert --- src/util/object_forwarding.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/object_forwarding.rs b/src/util/object_forwarding.rs index 0b834ad6d8..1253de8582 100644 --- a/src/util/object_forwarding.rs +++ b/src/util/object_forwarding.rs @@ -67,7 +67,7 @@ pub fn spin_and_get_forwarded_object( // the forwarding word while another thread was stuck spinning in the above loop. // See: https://github.com/mmtk/mmtk-core/issues/579 debug_assert!( - forwarding_bits == FORWARDED || forwarding_bits == FORWARDING_NOT_TRIGGERED_YET, + forwarding_bits == FORWARDING_NOT_TRIGGERED_YET, "Invalid/Corrupted forwarding word {:x} for object {}", forwarding_bits, object,