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" );
@@ -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->forward_to_atomic(obj, obj_mark) == NULL) {
if (obj->forward_to_atomic(obj, obj_mark)) {
// We won any races, we "own" this object.
assert(obj == obj->forwardee(), "Sanity");

@@ -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,10 @@ 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 { return cast_to_oop(decode_pointer()); }
};

// Support atomic operations.
@@ -294,7 +294,7 @@ oop oopDesc::forwardee() const {

// 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 {