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

8275527: Refactor forward pointer access #5955

Closed
wants to merge 14 commits into from
@@ -60,12 +60,13 @@ class G1ResetSkipCompactingClosure : public HeapRegionClosure {

size_t G1FullGCCompactTask::G1CompactRegionClosure::apply(oop obj) {
size_t size = obj->size();
HeapWord* destination = cast_from_oop<HeapWord*>(obj->forwardee());
if (destination == NULL) {
if (!obj->is_forwarded()) {
// Object not moving
return size;
}

HeapWord* destination = cast_from_oop<HeapWord*>(obj->forwardee());

// copy object and reinit its mark
HeapWord* obj_addr = cast_from_oop<HeapWord*>(obj);
assert(obj_addr != destination, "everything in this pass should be moving");
@@ -103,21 +103,9 @@ void G1FullGCCompactionPoint::forward(oop object, size_t size) {
// Store a forwarding pointer if the object should be moved.
if (cast_from_oop<HeapWord*>(object) != _compaction_top) {
object->forward_to(cast_to_oop(_compaction_top));
assert(object->is_forwarded(), "must be forwarded");
} else {
if (object->forwardee() != NULL) {
// 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.
object->init_mark();
} else {
Copy link
Member

@stefank stefank Oct 21, 2021

Choose a reason for hiding this comment

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

Could you explain why this was removed?

Copy link
Contributor Author

@rkennke rkennke Oct 28, 2021

Choose a reason for hiding this comment

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

The G1 Full GC code decoded the fwdptr without checking if object is actually forwarded (i.e. by checking the lowest two mark word bits), and then compare the result with NULL. In order for this to work, it modifies the mark word of not forwarded objects to prototype mark. This is all quite messy and can be avoided by checking is_forwarded() before actually decoding the fwdptr.

// Make sure object has the correct mark-word set or that it will be
// fixed when restoring the preserved marks.
assert(object->mark() == markWord::prototype() || // Correct mark
object->mark_must_be_preserved(), // Will be restored by PreservedMarksSet
"should have correct prototype obj: " PTR_FORMAT " mark: " PTR_FORMAT " prototype: " PTR_FORMAT,
p2i(object), object->mark().value(), markWord::prototype().value());
}
assert(object->forwardee() == NULL, "should be forwarded to NULL");
assert(!object->is_forwarded(), "must not be forwarded");
}

// Update compaction values.
@@ -77,19 +77,13 @@ template <class T> inline void G1AdjustClosure::adjust_pointer(T* p) {
return;
}

oop forwardee = obj->forwardee();
if (forwardee == NULL) {
// Not forwarded, return current reference.
assert(obj->mark() == markWord::prototype() || // Correct mark
obj->mark_must_be_preserved(), // Will be restored by PreservedMarksSet
"Must have correct prototype or be preserved, obj: " PTR_FORMAT ", mark: " PTR_FORMAT ", prototype: " PTR_FORMAT,
p2i(obj), obj->mark().value(), markWord::prototype().value());
return;
if (obj->is_forwarded()) {
oop forwardee = obj->forwardee();
// Forwarded, just update.
assert(G1CollectedHeap::heap()->is_in_reserved(forwardee), "should be in object space");
RawAccess<IS_NOT_NULL>::oop_store(p, forwardee);
}

// Forwarded, just update.
assert(G1CollectedHeap::heap()->is_in_reserved(forwardee), "should be in object space");
RawAccess<IS_NOT_NULL>::oop_store(p, forwardee);
}

inline void G1AdjustClosure::do_oop(oop* p) { do_oop_work(p); }
@@ -168,8 +168,7 @@ size_t G1FullGCPrepareTask::G1PrepareCompactLiveClosure::apply(oop object) {
size_t G1FullGCPrepareTask::G1RePrepareClosure::apply(oop obj) {
// We only re-prepare objects forwarded within the current region, so
// skip objects that are already forwarded to another region.
oop forwarded_to = obj->forwardee();
if (forwarded_to != NULL && !_current->is_in(forwarded_to)) {
if (obj->is_forwarded() && !_current->is_in(obj->forwardee())) {
return obj->size();
}

@@ -1,5 +1,5 @@
/*
* Copyright (c) 2001, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2001, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -233,8 +233,8 @@ void G1ParCopyClosure<barrier, should_mark>::do_oop_work(T* p) {
if (state.is_in_cset()) {
oop forwardee;
markWord m = obj->mark();
if (m.is_marked()) {
forwardee = cast_to_oop(m.decode_pointer());
if (m.is_forwarded()) {
forwardee = m.forwardee();
} else {
forwardee = _par_scan_state->copy_to_survivor_space(state, obj, m);
}
@@ -204,8 +204,8 @@ void G1ParScanThreadState::do_oop_evac(T* p) {
}

markWord m = obj->mark();
if (m.is_marked()) {
obj = cast_to_oop(m.decode_pointer());
if (m.is_forwarded()) {
obj = m.forwardee();
} else {
obj = do_copy_to_survivor_space(region_attr, obj, m);
}
@@ -856,7 +856,7 @@ class G1KeepAliveClosure: public OopClosure {
return;
}
if (region_attr.is_in_cset()) {
assert( obj->is_forwarded(), "invariant" );
assert(obj->is_forwarded(), "invariant" );
*p = obj->forwardee();
} else {
assert(!obj->is_forwarded(), "invariant" );
@@ -137,15 +137,15 @@ inline oop PSPromotionManager::copy_to_survivor_space(oop o) {
// in o. There may be multiple threads racing on it, and it may be forwarded
// at any time.
markWord m = o->mark();
if (!m.is_marked()) {
if (!m.is_forwarded()) {
return copy_unmarked_to_survivor_space<promote_immediately>(o, m);
} else {
// Ensure any loads from the forwardee follow all changes that precede
// the release-cmpxchg that performed the forwarding, possibly in some
// other thread.
OrderAccess::acquire();
// Return the already installed forwardee.
return cast_to_oop(m.decode_pointer());
return cast_to_oop(m.forwardee());
}
}

@@ -88,13 +88,8 @@ template <class T> inline void MarkSweep::adjust_pointer(T* p) {
oop obj = CompressedOops::decode_not_null(heap_oop);
assert(Universe::heap()->is_in(obj), "should be in heap");

oop new_obj = cast_to_oop(obj->mark().decode_pointer());

assert(new_obj != NULL || // is forwarding ptr?
obj->mark() == markWord::prototype(), // not gc marked?
"should be forwarded");

if (new_obj != NULL) {
if (obj->is_forwarded()) {
oop new_obj = obj->forwardee();
assert(is_object_aligned(new_obj), "oop must be aligned");
RawAccess<IS_NOT_NULL>::oop_store(p, new_obj);
}
@@ -376,7 +376,7 @@ HeapWord* CompactibleSpace::forward(oop q, size_t size,
// if the object isn't moving we can just set the mark to the default
// mark and handle it specially later on.
q->init_mark();
assert(q->forwardee() == NULL, "should be forwarded to NULL");
assert(!q->is_forwarded(), "should not be forwarded");
}

compact_top += size;
@@ -536,7 +536,7 @@ void CompactibleSpace::compact() {

debug_only(HeapWord* prev_obj = NULL);
while (cur_obj < end_of_live) {
if (!cast_to_oop(cur_obj)->is_gc_marked()) {
if (!cast_to_oop(cur_obj)->is_forwarded()) {
debug_only(prev_obj = cur_obj);
// The first word of the dead object contains a pointer to the next live object or end of space.
cur_obj = *(HeapWord**)cur_obj;
@@ -209,7 +209,7 @@ class markWord {
}

// used to encode pointers during GC
markWord clear_lock_bits() { return markWord(value() & ~lock_mask_in_place); }
markWord clear_lock_bits() const { return markWord(value() & ~lock_mask_in_place); }

// age operations
markWord set_marked() { return markWord((value() & ~lock_mask_in_place) | marked_value); }
@@ -243,7 +243,13 @@ class markWord {
inline static markWord encode_pointer_as_mark(void* p) { return from_pointer(p).set_marked(); }

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

inline bool is_forwarded() const { return is_marked(); }
inline oop forwardee() const {
assert(is_forwarded(), "only decode when actually forwarded");
return cast_to_oop(decode_pointer());
}
};
Copy link
Member

@stefank stefank Nov 1, 2021

Choose a reason for hiding this comment

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

This brings the forwarded/forwardee terminology into the markWord. The markWord was previously decoupled from those to concepts. I would personally let those function names stay in oopDesc and not leak down into the markWord. If you do want to keep it here, could you update the comments at the top that describes the bits?

//    [ptr             | 11]  marked             used to mark an object

Copy link
Contributor Author

@rkennke rkennke Nov 10, 2021

Choose a reason for hiding this comment

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

Yeah, I am not quite sure about this. We have a couple of places where we need to use the markWord direcly, and they read m.is_marked() (when it really means is_forwarded, even though it's the same in the implementation), and then goes on to cast_to_oop(m.decode_pointer()) which reads more ugly than simply m.forwardee() which also comes with an assert and the cast.

I reverted the markWord change and related call-sites now. Maybe this warrants more thinking/discussion.


// Support atomic operations.
@@ -262,7 +262,7 @@ bool oopDesc::is_gc_marked() const {
bool oopDesc::is_forwarded() const {
// The extra heap check is needed since the obj might be locked, in which case the
// mark would point to a stack location and have the sentinel bit cleared
return mark().is_marked();
return mark().is_forwarded();
}

// Used by scavengers
@@ -289,12 +289,12 @@ oop oopDesc::forward_to_atomic(oop p, markWord compare, atomic_memory_order orde
// 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 mark().forwardee();
}

// The following method needs to be MT safe.
uint oopDesc::age() const {
assert(!is_forwarded(), "Attempt to read age from forwarded mark");
assert(!mark().is_marked(), "Attempt to read age from forwarded mark");
if (has_displaced_mark()) {
return displaced_mark().age();
} else {
@@ -303,7 +303,7 @@ uint oopDesc::age() const {
}

void oopDesc::incr_age() {
assert(!is_forwarded(), "Attempt to increment age of forwarded mark");
assert(!mark().is_marked(), "Attempt to increment age of forwarded mark");
if (has_displaced_mark()) {
set_displaced_mark(displaced_mark().incr_age());
} else {