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
@@ -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" );
@@ -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;
@@ -289,12 +289,13 @@ 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 {
assert(is_forwarded(), "only decode when actually forwarded");
return cast_to_oop(mark().decode_pointer());
}

// 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 +304,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 {