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
@@ -36,6 +36,7 @@
#include "oops/access.inline.hpp"
#include "oops/compressedOops.inline.hpp"
#include "oops/oop.inline.hpp"
#include "oops/oopForwarding.hpp"

class RemoveSelfForwardPtrObjClosure: public ObjectClosure {
G1CollectedHeap* _g1h;
@@ -70,7 +71,8 @@ class RemoveSelfForwardPtrObjClosure: public ObjectClosure {
HeapWord* obj_addr = cast_from_oop<HeapWord*>(obj);
assert(_hr->is_in(obj_addr), "sanity");

if (obj->is_forwarded() && obj->forwardee() == obj) {
OopForwarding fwd(obj);
if (fwd.is_forwarded() && fwd.forwardee() == obj) {
// The object failed to move.

zap_dead_objects(_last_forwarded_object_end, obj_addr);
@@ -32,6 +32,7 @@
#include "gc/g1/heapRegion.inline.hpp"
#include "gc/shared/gcTraceTime.inline.hpp"
#include "logging/log.hpp"
#include "oops/oopForwarding.hpp"
#include "oops/oop.inline.hpp"
#include "utilities/ticks.hpp"

@@ -60,12 +61,14 @@ 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) {
OopForwarding fwd(obj);
if (!fwd.is_forwarded()) {
// Object not moving
return size;
}

HeapWord* destination = fwd.forwardee<HeapWord*>();

// 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");
@@ -25,6 +25,7 @@
#include "precompiled.hpp"
#include "gc/g1/g1FullGCCompactionPoint.hpp"
#include "gc/g1/heapRegion.hpp"
#include "oops/oopForwarding.hpp"
#include "oops/oop.inline.hpp"
#include "utilities/debug.hpp"

@@ -102,22 +103,10 @@ 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));
OopForwarding::forward_to(object, cast_to_oop(_compaction_top));
assert(OopForwarding(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(!OopForwarding(object).is_forwarded(), "must not be forwarded");
}

// Update compaction values.
@@ -36,6 +36,7 @@
#include "memory/universe.hpp"
#include "oops/access.inline.hpp"
#include "oops/compressedOops.inline.hpp"
#include "oops/oopForwarding.hpp"
#include "oops/oop.inline.hpp"

template <typename T>
@@ -77,19 +78,14 @@ 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;
OopForwarding fwd(obj);
if (fwd.is_forwarded()) {
oop forwardee = fwd.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); }
@@ -36,6 +36,7 @@
#include "gc/shared/referenceProcessor.hpp"
#include "logging/log.hpp"
#include "memory/iterator.inline.hpp"
#include "oops/oopForwarding.hpp"
#include "oops/oop.inline.hpp"
#include "utilities/ticks.hpp"

@@ -168,8 +169,8 @@ 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)) {
OopForwarding fwd(obj);
if (fwd.is_forwarded() && !_current->is_in(fwd.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
@@ -36,6 +36,7 @@
#include "memory/iterator.inline.hpp"
#include "oops/access.inline.hpp"
#include "oops/compressedOops.inline.hpp"
#include "oops/oopForwarding.hpp"
#include "oops/oopsHierarchy.hpp"
#include "oops/oop.inline.hpp"
#include "runtime/prefetch.inline.hpp"
@@ -55,8 +56,7 @@ inline void G1ScanClosureBase::prefetch_and_push(T* p, const oop obj) {
// problems before we go into push_on_queue to know where the
// problem is coming from
assert((obj == RawAccess<>::oop_load(p)) ||
(obj->is_forwarded() &&
obj->forwardee() == RawAccess<>::oop_load(p)),
(OopForwarding(obj).forwardee() == RawAccess<>::oop_load(p)),
"p should still be pointing to obj or to its forwardee");

_par_scan_state->push_on_queue(ScannerTask(p));
@@ -232,11 +232,11 @@ void G1ParCopyClosure<barrier, should_mark>::do_oop_work(T* p) {
const G1HeapRegionAttr state = _g1h->region_attr(obj);
if (state.is_in_cset()) {
oop forwardee;
markWord m = obj->mark();
if (m.is_marked()) {
forwardee = cast_to_oop(m.decode_pointer());
OopForwarding fwd = OopForwarding(obj);
if (fwd.is_forwarded()) {
forwardee = fwd.forwardee();
} else {
forwardee = _par_scan_state->copy_to_survivor_space(state, obj, m);
forwardee = _par_scan_state->copy_to_survivor_space(state, obj, fwd);
}
assert(forwardee != NULL, "forwardee should not be NULL");
RawAccess<IS_NOT_NULL>::oop_store(p, forwardee);
@@ -40,6 +40,7 @@
#include "memory/allocation.inline.hpp"
#include "oops/access.inline.hpp"
#include "oops/oop.inline.hpp"
#include "oops/oopForwarding.hpp"
#include "runtime/atomic.hpp"
#include "runtime/prefetch.inline.hpp"
#include "utilities/globalDefinitions.hpp"
@@ -203,11 +204,11 @@ void G1ParScanThreadState::do_oop_evac(T* p) {
return;
}

markWord m = obj->mark();
if (m.is_marked()) {
obj = cast_to_oop(m.decode_pointer());
OopForwarding fwd(obj);
if (fwd.is_forwarded()) {
obj = fwd.forwardee();
} else {
obj = do_copy_to_survivor_space(region_attr, obj, m);
obj = do_copy_to_survivor_space(region_attr, obj, fwd);
}
RawAccess<IS_NOT_NULL>::oop_store(p, obj);

@@ -220,9 +221,10 @@ void G1ParScanThreadState::do_partial_array(PartialArrayScanTask task) {

assert(_g1h->is_in_reserved(from_obj), "must be in heap.");
assert(from_obj->is_objArray(), "must be obj array");
assert(from_obj->is_forwarded(), "must be forwarded");
OopForwarding fwd(from_obj);
assert(fwd.is_forwarded(), "must be forwarded");

oop to_obj = from_obj->forwardee();
oop to_obj = fwd.forwardee();
assert(from_obj != to_obj, "should not be chunking self-forwarded objects");
assert(to_obj->is_objArray(), "must be obj array");
objArrayOop to_array = objArrayOop(to_obj);
@@ -250,8 +252,8 @@ void G1ParScanThreadState::start_partial_objarray(G1HeapRegionAttr dest_attr,
oop from_obj,
oop to_obj) {
assert(from_obj->is_objArray(), "precondition");
assert(from_obj->is_forwarded(), "precondition");
assert(from_obj->forwardee() == to_obj, "precondition");
assert(OopForwarding(from_obj).is_forwarded(), "precondition");
assert(OopForwarding(from_obj).forwardee() == to_obj, "precondition");
assert(from_obj != to_obj, "should not be scanning self-forwarded objects");
assert(to_obj->is_objArray(), "precondition");

@@ -439,7 +441,7 @@ void G1ParScanThreadState::undo_allocation(G1HeapRegionAttr dest_attr,
MAYBE_INLINE_EVACUATION
oop G1ParScanThreadState::do_copy_to_survivor_space(G1HeapRegionAttr const region_attr,
oop const old,
markWord const old_mark) {
const OopForwarding& fwd) {
assert(region_attr.is_in_cset(),
"Unexpected region attr type: %s", region_attr.get_type_str());

@@ -449,7 +451,7 @@ oop G1ParScanThreadState::do_copy_to_survivor_space(G1HeapRegionAttr const regio
const size_t word_sz = old->size_given_klass(klass);

uint age = 0;
G1HeapRegionAttr dest_attr = next_region_attr(region_attr, old_mark, age);
G1HeapRegionAttr dest_attr = next_region_attr(region_attr, fwd.mark(), age);
HeapRegion* const from_region = _g1h->heap_region_containing(old);
uint node_index = from_region->node_index();

@@ -462,7 +464,7 @@ oop G1ParScanThreadState::do_copy_to_survivor_space(G1HeapRegionAttr const regio
if (obj_ptr == NULL) {
// This will either forward-to-self, or detect that someone else has
// installed a forwarding pointer.
return handle_evacuation_failure_par(old, old_mark, word_sz);
return handle_evacuation_failure_par(old, fwd, word_sz);
}
}

@@ -474,15 +476,15 @@ oop G1ParScanThreadState::do_copy_to_survivor_space(G1HeapRegionAttr const regio
// Doing this after all the allocation attempts also tests the
// undo_allocation() method too.
undo_allocation(dest_attr, obj_ptr, word_sz, node_index);
return handle_evacuation_failure_par(old, old_mark, word_sz);
return handle_evacuation_failure_par(old, fwd, word_sz);
}

// We're going to allocate linearly, so might as well prefetch ahead.
Prefetch::write(obj_ptr, PrefetchCopyIntervalInBytes);
Copy::aligned_disjoint_words(cast_from_oop<HeapWord*>(old), obj_ptr, word_sz);

const oop obj = cast_to_oop(obj_ptr);
const oop forward_ptr = old->forward_to_atomic(obj, old_mark, memory_order_relaxed);
const oop forward_ptr = fwd.forward_to_atomic(obj, memory_order_relaxed);
if (forward_ptr == NULL) {

{
@@ -542,8 +544,8 @@ oop G1ParScanThreadState::do_copy_to_survivor_space(G1HeapRegionAttr const regio
ATTRIBUTE_FLATTEN
oop G1ParScanThreadState::copy_to_survivor_space(G1HeapRegionAttr region_attr,
oop old,
markWord old_mark) {
return do_copy_to_survivor_space(region_attr, old, old_mark);
const OopForwarding& fwd) {
return do_copy_to_survivor_space(region_attr, old, fwd);
}

G1ParScanThreadState* G1ParScanThreadStateSet::state_for_worker(uint worker_id) {
@@ -600,10 +602,10 @@ void G1ParScanThreadStateSet::record_unused_optional_region(HeapRegion* hr) {
}

NOINLINE
oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m, size_t word_sz) {
oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, const OopForwarding& fwd, size_t word_sz) {
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 = fwd.forward_to_atomic(old, 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);
@@ -612,7 +614,7 @@ oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m, siz
_g1h->hr_printer()->evac_failure(r);
}

_preserved_marks->push_if_necessary(old, m);
_preserved_marks->push_if_necessary(old, fwd.mark());
_evacuation_failed_info.register_copy_failure(word_sz);

// For iterating objects that failed evacuation currently we can reuse the
@@ -43,6 +43,7 @@ class G1EvacuationRootClosures;
class G1OopStarChunkedList;
class G1PLABAllocator;
class HeapRegion;
class OopForwarding;
class PreservedMarks;
class PreservedMarksSet;
class outputStream;
@@ -170,7 +171,7 @@ class G1ParScanThreadState : public CHeapObj<mtGC> {

oop do_copy_to_survivor_space(G1HeapRegionAttr region_attr,
oop obj,
markWord old_mark);
const OopForwarding& fwd);

// This method is applied to the fields of the objects that have just been copied.
template <class T> void do_oop_evac(T* p);
@@ -204,7 +205,7 @@ class G1ParScanThreadState : public CHeapObj<mtGC> {
inline void update_numa_stats(uint node_index);

public:
oop copy_to_survivor_space(G1HeapRegionAttr region_attr, oop obj, markWord old_mark);
oop copy_to_survivor_space(G1HeapRegionAttr region_attr, oop obj, const OopForwarding& fwd);

inline void trim_queue();
inline void trim_queue_partially();
@@ -214,7 +215,7 @@ class G1ParScanThreadState : public CHeapObj<mtGC> {
void reset_trim_ticks();

// An attempt to evacuate "obj" has failed; take necessary steps.
oop handle_evacuation_failure_par(oop obj, markWord m, size_t word_sz);
oop handle_evacuation_failure_par(oop obj, const OopForwarding& fwd, size_t word_sz);

template <typename T>
inline void remember_root_into_optional_region(T* p);
@@ -32,6 +32,7 @@
#include "gc/g1/g1OopStarChunkedList.inline.hpp"
#include "gc/g1/g1RemSet.hpp"
#include "oops/access.inline.hpp"
#include "oops/oopForwarding.hpp"
#include "oops/oop.inline.hpp"

inline void G1ParScanThreadState::push_on_queue(ScannerTask task) {
@@ -111,7 +112,7 @@ template <class T> void G1ParScanThreadState::write_ref_field_post(T* p, oop obj
// References to the current collection set are references to objects that failed
// evacuation. Currently these regions are always relabelled as old without
// remembered sets, so skip them.
assert(dest_attr.is_in_cset() == (obj->is_forwarded() && obj->forwardee() == obj),
assert(dest_attr.is_in_cset() == (OopForwarding(obj).is_forwarded() && (OopForwarding(obj).forwardee() == obj)),
"Only evac-failed objects must be in the collection set here but " PTR_FORMAT " is not", p2i(obj));
if (dest_attr.is_in_cset()) {
return;
@@ -57,6 +57,7 @@
#include "gc/shared/workerThread.hpp"
#include "jfr/jfrEvents.hpp"
#include "memory/resourceArea.hpp"
#include "oops/oopForwarding.hpp"
#include "utilities/ticks.hpp"

#if TASKQUEUE_STATS
@@ -856,10 +857,11 @@ class G1KeepAliveClosure: public OopClosure {
return;
}
if (region_attr.is_in_cset()) {
assert( obj->is_forwarded(), "invariant" );
*p = obj->forwardee();
OopForwarding fwd(obj);
assert(fwd.is_forwarded(), "invariant" );
*p = OopForwarding(obj).forwardee();
} else {
assert(!obj->is_forwarded(), "invariant" );
assert(!OopForwarding(obj).is_forwarded(), "invariant" );
assert(region_attr.is_humongous(),
"Only allowed G1HeapRegionAttr state is IsHumongous, but is %d", region_attr.type());
_g1h->set_humongous_is_live(obj);
@@ -990,7 +992,7 @@ void G1YoungCollector::process_discovered_references(G1ParScanThreadStateSet* pe
bool G1STWIsAliveClosure::do_object_b(oop p) {
// An object is reachable if it is outside the collection set,
// or is inside and copied.
return !_g1h->is_in_cset(p) || p->is_forwarded();
return !_g1h->is_in_cset(p) || OopForwarding(p).is_forwarded();
}

void G1YoungCollector::post_evacuate_cleanup_1(G1ParScanThreadStateSet* per_thread_states) {