Skip to content
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

Implement self-forwarding of objects that preserves header bits #10

Closed
wants to merge 4 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -111,7 +111,7 @@ void G1FullGCCompactionPoint::forward(SlidingForwarding* const forwarding, oop o
// used to do the same check. However, it is more reliable to first check the lower bits (is_forwarded())
// instead before accepting the forwardee. The code in G1FullCompactTask has been changed accordingly,
// which should make this block superfluous.
if ((cast_from_oop<uintptr_t>(object->forwardee()) & 0x00000000ffffffff) != 0) {
if ((reinterpret_cast<uintptr_t>(object->mark().decode_pointer()) & 0x00000000ffffffff) != 0) {
// Object should not move but mark-word is used so it looks like the
// object is forwarded. Need to clear the mark and it's no problem
// since it will be restored by preserved marks.
@@ -124,7 +124,7 @@ void G1FullGCCompactionPoint::forward(SlidingForwarding* const forwarding, oop o
"should have correct prototype obj: " PTR_FORMAT " mark: " PTR_FORMAT " prototype: " PTR_FORMAT,
p2i(object), object->mark().value(), markWord::prototype().value());
}
assert((cast_from_oop<uintptr_t>(object->forwardee()) & 0x00000000ffffffff) == 0, "should be forwarded to NULL");
assert((reinterpret_cast<uintptr_t>(object->mark().decode_pointer()) & 0x00000000ffffffff) == 0, "should be forwarded to NULL");
}

// Update compaction values.
@@ -234,7 +234,7 @@ void G1ParCopyClosure<barrier, should_mark>::do_oop_work(T* p) {
oop forwardee;
markWord m = obj->mark();
if (m.is_marked()) {
forwardee = cast_to_oop(m.decode_pointer());
forwardee = obj->forwardee(m);
Copy link
Collaborator

@tschatzl tschatzl Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code has been done that way intentionally: the changed code reloads the value from the header from memory (from what I can tell from generated code), while the original code does not, just reusing the value in the register.

Even if this is a nano-nano-optimization I would prefer to keep it as is. Some of the other changes seem to cause very similar "regressions".

(Yeah, I'm late, sorry).

Copy link
Collaborator Author

@rkennke rkennke Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code has been done that way intentionally: the changed code reloads the value from the header from memory (from what I can tell from generated code), while the original code does not, just reusing the value in the register.

Even if this is a nano-nano-optimization I would prefer to keep it as is. Some of the other changes seem to cause very similar "regressions".

(Yeah, I'm late, sorry).

I think I preserved the original behavior of re-using the already-loaded header. Notice that I added an oopDesc::forwardee(markWord m) to do that.

Copy link
Collaborator

@tschatzl tschatzl Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then ignore me :)

} else {
forwardee = _par_scan_state->copy_to_survivor_space(state, obj, m);
}
@@ -197,7 +197,7 @@ void G1ParScanThreadState::do_oop_evac(T* p) {

markWord m = obj->mark();
if (m.is_marked()) {
obj = cast_to_oop(m.decode_pointer());
obj = obj->forwardee(m);
} else {
obj = do_copy_to_survivor_space(region_attr, obj, m);
}
@@ -600,7 +600,7 @@ NOINLINE
oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m) {
assert(_g1h->is_in_cset(old), "Object " PTR_FORMAT " should be in the CSet", p2i(old));

oop forward_ptr = old->forward_to_atomic(old, m, memory_order_relaxed);
oop forward_ptr = old->forward_to_self_atomic(m, memory_order_relaxed);
if (forward_ptr == NULL) {
// Forward-to-self succeeded. We are the "owner" of the object.
HeapRegion* r = _g1h->heap_region_containing(old);
@@ -352,7 +352,7 @@ oop PSPromotionManager::oop_promotion_failed(oop obj, markWord obj_mark) {
// this started. If it is the same (i.e., no forwarding
// pointer has been installed), then this thread owns
// it.
if (obj->cas_forward_to(obj, obj_mark)) {
if (obj->forward_to_self_atomic(obj_mark) == NULL) {
// We won any races, we "own" this object.
assert(obj == obj->forwardee(), "Sanity");

@@ -144,7 +144,7 @@ inline oop PSPromotionManager::copy_to_survivor_space(oop o) {
// other thread.
OrderAccess::acquire();
// Return the already installed forwardee.
return cast_to_oop(m.decode_pointer());
return o->forwardee(m);
}
}

@@ -679,7 +679,7 @@ void DefNewGeneration::handle_promotion_failure(oop old) {
_promotion_failed_info.register_copy_failure(old->size());
_preserved_marks_set.get()->push_if_necessary(old, old->mark());
// forward to self
old->forward_to(old);
old->forward_to_self();

_promo_failure_scan_stack.push(old);

@@ -72,8 +72,8 @@ class SlidingForwarding : public CHeapObj<mtGC> {

static const size_t NUM_REGIONS = ONE << NUM_REGION_BITS;

// We need the lowest two bits to indicate a forwarded object.
static const int BASE_SHIFT = 2;
// We need the lowest three bits to indicate a forwarded object and self-forwarding.
static const int BASE_SHIFT = 3;

// The compressed address bits start here.
static const int COMPRESSED_BITS_SHIFT = BASE_SHIFT + NUM_REGION_BITS;
@@ -98,23 +98,25 @@ class markWord {
// Constants
static const int age_bits = 4;
static const int lock_bits = 2;
static const int first_unused_gap_bits = 1;
static const int max_hash_bits = BitsPerWord - age_bits - lock_bits - first_unused_gap_bits;
static const int self_forwarded_bits = 1;
static const int max_hash_bits = BitsPerWord - age_bits - lock_bits - self_forwarded_bits;
static const int hash_bits = max_hash_bits > 25 ? 25 : max_hash_bits;
static const int second_unused_gap_bits = LP64_ONLY(1) NOT_LP64(0);
#ifdef _LP64
static const int klass_bits = 32;
#endif

static const int lock_shift = 0;
static const int age_shift = lock_bits + first_unused_gap_bits;
static const int self_forwarded_shift = lock_shift + lock_bits;
static const int age_shift = self_forwarded_shift + self_forwarded_bits;
static const int hash_shift = age_shift + age_bits;
#ifdef _LP64
static const int klass_shift = hash_shift + hash_bits;
#endif

static const uintptr_t lock_mask = right_n_bits(lock_bits);
static const uintptr_t lock_mask_in_place = lock_mask << lock_shift;
static const uintptr_t self_forwarded_mask = right_n_bits(self_forwarded_bits);
static const uintptr_t self_forwarded_mask_in_place = self_forwarded_mask << self_forwarded_shift;
static const uintptr_t age_mask = right_n_bits(age_bits);
static const uintptr_t age_mask_in_place = age_mask << age_shift;
static const uintptr_t hash_mask = right_n_bits(hash_bits);
@@ -265,6 +267,14 @@ class markWord {

// Recover address of oop from encoded form used in mark
inline void* decode_pointer() { return (void*)clear_lock_bits().value(); }

inline bool self_forwarded() const {
return mask_bits(value(), self_forwarded_mask_in_place) != 0;
}

inline markWord set_self_forwarded() const {
return markWord(value() | self_forwarded_mask_in_place | marked_value);
}
};

// Support atomic operations.
@@ -247,15 +247,17 @@ class oopDesc {
void verify_forwardee(oop forwardee) NOT_DEBUG_RETURN;

inline void forward_to(oop p);
inline bool cas_forward_to(oop p, markWord compare, atomic_memory_order order = memory_order_conservative);
inline void forward_to_self();
shipilev marked this conversation as resolved.
Show resolved Hide resolved

// Like "forward_to", but inserts the forwarding pointer atomically.
// Exactly one thread succeeds in inserting the forwarding pointer, and
// this call returns "NULL" for that thread; any other thread has the
// value of the forwarding pointer returned and does not modify "this".
inline oop forward_to_atomic(oop p, markWord compare, atomic_memory_order order = memory_order_conservative);
inline oop forward_to_self_atomic(markWord compare, atomic_memory_order order = memory_order_conservative);

inline oop forwardee() const;
inline oop forwardee(markWord header) const;

// Age of object during scavenge
inline uint age() const;
@@ -277,35 +277,56 @@ bool oopDesc::is_forwarded() const {
void oopDesc::forward_to(oop p) {
verify_forwardee(p);
markWord m = markWord::encode_pointer_as_mark(p);
assert(m.decode_pointer() == p, "encoding must be reversable");
assert(forwardee(m) == p, "encoding must be reversable");
set_mark(m);
}

// Used by parallel scavengers
bool oopDesc::cas_forward_to(oop p, markWord compare, atomic_memory_order order) {
verify_forwardee(p);
markWord m = markWord::encode_pointer_as_mark(p);
assert(m.decode_pointer() == p, "encoding must be reversable");
return cas_set_mark(m, compare, order) == compare;
void oopDesc::forward_to_self() {
verify_forwardee(this);
markWord m = mark().set_self_forwarded();
assert(forwardee(m) == cast_to_oop(this), "encoding must be reversable");
set_mark(m);
}

oop oopDesc::forward_to_atomic(oop p, markWord compare, atomic_memory_order order) {
verify_forwardee(p);
markWord m = markWord::encode_pointer_as_mark(p);
assert(m.decode_pointer() == p, "encoding must be reversable");
assert(forwardee(m) == p, "encoding must be reversable");
markWord old_mark = cas_set_mark(m, compare, order);
if (old_mark == compare) {
return NULL;
} else {
return forwardee(old_mark);
}
}

oop oopDesc::forward_to_self_atomic(markWord compare, atomic_memory_order order) {
verify_forwardee(this);
markWord m = compare.set_self_forwarded();
assert(forwardee(m) == cast_to_oop(this), "encoding must be reversable");
markWord old_mark = cas_set_mark(m, compare, order);
if (old_mark == compare) {
return NULL;
} else {
return cast_to_oop(old_mark.decode_pointer());
assert(old_mark.is_marked(), "must be marked here");
return forwardee(old_mark);
}
}

// Note that the forwardee is not the same thing as the displaced_mark.
// The forwardee is used when copying during scavenge and mark-sweep.
// It does need to clear the low two locking- and GC-related bits.
oop oopDesc::forwardee() const {
return cast_to_oop(mark().decode_pointer());
return forwardee(mark());
}

oop oopDesc::forwardee(markWord header) const {
assert(header.is_marked(), "must be forwarded");
if (header.self_forwarded()) {
return cast_to_oop(this);
} else {
return cast_to_oop(header.decode_pointer());
}
}

// The following method needs to be MT safe.