Skip to content

8305898: Alternative self-forwarding mechanism #20603

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

8305898: Alternative self-forwarding mechanism #20603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
86239af
8305898: Alternative self-forwarding mechanism
rkennke Aug 15, 2024
ba54b2d
Remove special handling of non-self-fwded objects
rkennke Aug 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/hotspot/share/gc/g1/g1CollectedHeap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@
#include "gc/shared/isGCActiveMark.hpp"
#include "gc/shared/locationPrinter.inline.hpp"
#include "gc/shared/oopStorageParState.hpp"
#include "gc/shared/preservedMarks.inline.hpp"
#include "gc/shared/referenceProcessor.inline.hpp"
#include "gc/shared/suspendibleThreadSet.hpp"
#include "gc/shared/taskqueue.inline.hpp"
Expand Down
2 changes: 0 additions & 2 deletions src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ G1GCPhaseTimes::G1GCPhaseTimes(STWGCTimer* gc_timer, uint max_gc_threads) :
_gc_par_phases[UpdateDerivedPointers] = new WorkerDataArray<double>("UpdateDerivedPointers", "Update Derived Pointers (ms):", max_gc_threads);
#endif
_gc_par_phases[EagerlyReclaimHumongousObjects] = new WorkerDataArray<double>("EagerlyReclaimHumongousObjects", "Eagerly Reclaim Humongous Objects (ms):", max_gc_threads);
_gc_par_phases[RestorePreservedMarks] = new WorkerDataArray<double>("RestorePreservedMarks", "Restore Preserved Marks (ms):", max_gc_threads);
_gc_par_phases[ProcessEvacuationFailedRegions] = new WorkerDataArray<double>("ProcessEvacuationFailedRegions", "Process Evacuation Failed Regions (ms):", max_gc_threads);

_gc_par_phases[ScanHR]->create_thread_work_items("Scanned Cards:", ScanHRScannedCards);
Expand Down Expand Up @@ -512,7 +511,6 @@ double G1GCPhaseTimes::print_post_evacuate_collection_set(bool evacuation_failed
debug_time("Post Evacuate Cleanup 2", _cur_post_evacuate_cleanup_2_time_ms);
if (evacuation_failed) {
debug_phase(_gc_par_phases[RecalculateUsed], 1);
debug_phase(_gc_par_phases[RestorePreservedMarks], 1);
debug_phase(_gc_par_phases[ProcessEvacuationFailedRegions], 1);
}
#if COMPILER2_OR_JVMCI
Expand Down
1 change: 0 additions & 1 deletion src/hotspot/share/gc/g1/g1GCPhaseTimes.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ class G1GCPhaseTimes : public CHeapObj<mtGC> {
UpdateDerivedPointers,
#endif
EagerlyReclaimHumongousObjects,
RestorePreservedMarks,
ProcessEvacuationFailedRegions,
ResetMarkingState,
NoteStartOfMark,
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/gc/g1/g1OopClosures.inline.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ void G1ParCopyClosure<barrier, should_mark>::do_oop_work(T* p) {
oop forwardee;
markWord m = obj->mark();
if (m.is_forwarded()) {
forwardee = m.forwardee();
forwardee = obj->forwardee(m);
} else {
forwardee = _par_scan_state->copy_to_survivor_space(state, obj, m);
}
Expand Down
13 changes: 2 additions & 11 deletions src/hotspot/share/gc/g1/g1ParScanThreadState.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
#include "gc/shared/continuationGCSupport.inline.hpp"
#include "gc/shared/partialArrayState.hpp"
#include "gc/shared/partialArrayTaskStepper.inline.hpp"
#include "gc/shared/preservedMarks.inline.hpp"
#include "gc/shared/stringdedup/stringDedup.hpp"
#include "gc/shared/taskqueue.inline.hpp"
#include "memory/allocation.inline.hpp"
Expand All @@ -59,7 +58,6 @@

G1ParScanThreadState::G1ParScanThreadState(G1CollectedHeap* g1h,
G1RedirtyCardsQueueSet* rdcqs,
PreservedMarks* preserved_marks,
uint worker_id,
uint num_workers,
G1CollectionSet* collection_set,
Expand Down Expand Up @@ -90,7 +88,6 @@ G1ParScanThreadState::G1ParScanThreadState(G1CollectedHeap* g1h,
_numa(g1h->numa()),
_obj_alloc_stat(nullptr),
ALLOCATION_FAILURE_INJECTOR_ONLY(_allocation_failure_inject_counter(0) COMMA)
_preserved_marks(preserved_marks),
_evacuation_failed_info(),
_evac_failure_regions(evac_failure_regions),
_evac_failure_enqueued_cards(0)
Expand Down Expand Up @@ -216,7 +213,7 @@ void G1ParScanThreadState::do_oop_evac(T* p) {

markWord m = obj->mark();
if (m.is_forwarded()) {
obj = m.forwardee();
obj = obj->forwardee(m);
} else {
obj = do_copy_to_survivor_space(region_attr, obj, m);
}
Expand Down Expand Up @@ -595,7 +592,6 @@ G1ParScanThreadState* G1ParScanThreadStateSet::state_for_worker(uint worker_id)
if (_states[worker_id] == nullptr) {
_states[worker_id] =
new G1ParScanThreadState(_g1h, rdcqs(),
_preserved_marks_set.get(worker_id),
worker_id,
_num_workers,
_collection_set,
Expand Down Expand Up @@ -655,7 +651,7 @@ NOINLINE
oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m, size_t word_sz, bool cause_pinned) {
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 == nullptr) {
// Forward-to-self succeeded. We are the "owner" of the object.
G1HeapRegion* r = _g1h->heap_region_containing(old);
Expand All @@ -668,8 +664,6 @@ oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m, siz
// evacuation failure recovery.
_g1h->mark_evac_failure_object(_worker_id, old, word_sz);

_preserved_marks->push_if_necessary(old, m);

ContinuationGCSupport::transform_stack_chunk(old);

_evacuation_failed_info.register_copy_failure(word_sz);
Expand Down Expand Up @@ -727,7 +721,6 @@ G1ParScanThreadStateSet::G1ParScanThreadStateSet(G1CollectedHeap* g1h,
_g1h(g1h),
_collection_set(collection_set),
_rdcqs(G1BarrierSet::dirty_card_queue_set().allocator()),
_preserved_marks_set(true /* in_c_heap */),
_states(NEW_C_HEAP_ARRAY(G1ParScanThreadState*, num_workers, mtGC)),
_rdc_buffers(NEW_C_HEAP_ARRAY(BufferNodeList, num_workers, mtGC)),
_surviving_young_words_total(NEW_C_HEAP_ARRAY(size_t, collection_set->young_region_length() + 1, mtGC)),
Expand All @@ -736,7 +729,6 @@ G1ParScanThreadStateSet::G1ParScanThreadStateSet(G1CollectedHeap* g1h,
_evac_failure_regions(evac_failure_regions),
_partial_array_state_allocator(num_workers)
{
_preserved_marks_set.init(num_workers);
for (uint i = 0; i < num_workers; ++i) {
_states[i] = nullptr;
_rdc_buffers[i] = BufferNodeList();
Expand All @@ -749,5 +741,4 @@ G1ParScanThreadStateSet::~G1ParScanThreadStateSet() {
FREE_C_HEAP_ARRAY(G1ParScanThreadState*, _states);
FREE_C_HEAP_ARRAY(size_t, _surviving_young_words_total);
FREE_C_HEAP_ARRAY(BufferNodeList, _rdc_buffers);
_preserved_marks_set.reclaim();
}
7 changes: 0 additions & 7 deletions src/hotspot/share/gc/g1/g1ParScanThreadState.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
#include "gc/shared/gc_globals.hpp"
#include "gc/shared/partialArrayState.hpp"
#include "gc/shared/partialArrayTaskStepper.hpp"
#include "gc/shared/preservedMarks.hpp"
#include "gc/shared/stringdedup/stringDedup.hpp"
#include "gc/shared/taskqueue.hpp"
#include "memory/allocation.hpp"
Expand All @@ -48,8 +47,6 @@ class G1EvacuationRootClosures;
class G1OopStarChunkedList;
class G1PLABAllocator;
class G1HeapRegion;
class PreservedMarks;
class PreservedMarksSet;
class outputStream;

class G1ParScanThreadState : public CHeapObj<mtGC> {
Expand Down Expand Up @@ -108,7 +105,6 @@ class G1ParScanThreadState : public CHeapObj<mtGC> {
// Per-thread evacuation failure data structures.
ALLOCATION_FAILURE_INJECTOR_ONLY(size_t _allocation_failure_inject_counter;)

PreservedMarks* _preserved_marks;
EvacuationFailedInfo _evacuation_failed_info;
G1EvacFailureRegions* _evac_failure_regions;
// Number of additional cards into evacuation failed regions enqueued into
Expand All @@ -127,7 +123,6 @@ class G1ParScanThreadState : public CHeapObj<mtGC> {
public:
G1ParScanThreadState(G1CollectedHeap* g1h,
G1RedirtyCardsQueueSet* rdcqs,
PreservedMarks* preserved_marks,
uint worker_id,
uint num_workers,
G1CollectionSet* collection_set,
Expand Down Expand Up @@ -248,7 +243,6 @@ class G1ParScanThreadStateSet : public StackObj {
G1CollectedHeap* _g1h;
G1CollectionSet* _collection_set;
G1RedirtyCardsQueueSet _rdcqs;
PreservedMarksSet _preserved_marks_set;
G1ParScanThreadState** _states;
BufferNodeList* _rdc_buffers;
size_t* _surviving_young_words_total;
Expand All @@ -266,7 +260,6 @@ class G1ParScanThreadStateSet : public StackObj {

G1RedirtyCardsQueueSet* rdcqs() { return &_rdcqs; }
BufferNodeList* rdc_buffers() { return _rdc_buffers; }
PreservedMarksSet* preserved_marks_set() { return &_preserved_marks_set; }

void flush_stats();
void record_unused_optional_region(G1HeapRegion* hr);
Expand Down
1 change: 0 additions & 1 deletion src/hotspot/share/gc/g1/g1YoungCollector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@
#include "gc/shared/gcTraceTime.inline.hpp"
#include "gc/shared/gcTimer.hpp"
#include "gc/shared/gc_globals.hpp"
#include "gc/shared/preservedMarks.hpp"
#include "gc/shared/referenceProcessor.hpp"
#include "gc/shared/weakProcessor.inline.hpp"
#include "gc/shared/workerPolicy.hpp"
Expand Down
25 changes: 1 addition & 24 deletions src/hotspot/share/gc/g1/g1YoungGCPostEvacuateTasks.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
#include "gc/g1/g1RemSet.hpp"
#include "gc/g1/g1YoungGCPostEvacuateTasks.hpp"
#include "gc/shared/bufferNode.hpp"
#include "gc/shared/preservedMarks.inline.hpp"
#include "jfr/jfrEvents.hpp"
#include "oops/access.inline.hpp"
#include "oops/compressedOops.inline.hpp"
Expand Down Expand Up @@ -252,7 +251,7 @@ class G1PostEvacuateCollectionSetCleanupTask1::RestoreEvacFailureRegionsTask : p
{
// Process marked object.
assert(obj->is_forwarded() && obj->forwardee() == obj, "must be self-forwarded");
obj->init_mark();
obj->unset_self_forwarded();
hr->update_bot_for_block(obj_addr, obj_end_addr);

// Statistics
Expand Down Expand Up @@ -477,27 +476,6 @@ class G1PostEvacuateCollectionSetCleanupTask2::EagerlyReclaimHumongousObjectsTas
}
};

class G1PostEvacuateCollectionSetCleanupTask2::RestorePreservedMarksTask : public G1AbstractSubTask {
PreservedMarksSet* _preserved_marks;
WorkerTask* _task;

public:
RestorePreservedMarksTask(PreservedMarksSet* preserved_marks) :
G1AbstractSubTask(G1GCPhaseTimes::RestorePreservedMarks),
_preserved_marks(preserved_marks),
_task(preserved_marks->create_task()) { }

virtual ~RestorePreservedMarksTask() {
delete _task;
}

double worker_cost() const override {
return _preserved_marks->num();
}

void do_work(uint worker_id) override { _task->work(worker_id); }
};

class RedirtyLoggedCardTableEntryClosure : public G1CardTableEntryClosure {
size_t _num_dirtied;
G1CollectedHeap* _g1h;
Expand Down Expand Up @@ -979,7 +957,6 @@ G1PostEvacuateCollectionSetCleanupTask2::G1PostEvacuateCollectionSetCleanupTask2
}

if (evac_failure_regions->has_regions_evac_failed()) {
add_parallel_task(new RestorePreservedMarksTask(per_thread_states->preserved_marks_set()));
add_parallel_task(new ProcessEvacuationFailedRegionsTask(evac_failure_regions));
}
add_parallel_task(new RedirtyLoggedCardsTask(evac_failure_regions,
Expand Down
2 changes: 0 additions & 2 deletions src/hotspot/share/gc/g1/g1YoungGCPostEvacuateTasks.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ class G1PostEvacuateCollectionSetCleanupTask1 : public G1BatchedTask {
// - Update Derived Pointers (s)
// - Clear Retained Region Data (on evacuation failure)
// - Redirty Logged Cards
// - Restore Preserved Marks (on evacuation failure)
// - Free Collection Set
// - Resize TLABs
class G1PostEvacuateCollectionSetCleanupTask2 : public G1BatchedTask {
Expand All @@ -67,7 +66,6 @@ class G1PostEvacuateCollectionSetCleanupTask2 : public G1BatchedTask {

class ProcessEvacuationFailedRegionsTask;
class RedirtyLoggedCardsTask;
class RestorePreservedMarksTask;
class FreeCollectionSetTask;
class ResizeTLABsTask;

Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/gc/parallel/psPromotionManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,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) == nullptr) {
if (obj->forward_to_self_atomic(obj_mark) == nullptr) {
// We won any races, we "own" this object.
assert(obj == obj->forwardee(), "Sanity");

Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/gc/parallel/psPromotionManager.inline.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ inline oop PSPromotionManager::copy_to_survivor_space(oop o) {
return copy_unmarked_to_survivor_space<promote_immediately>(o, m);
} else {
// Return the already installed forwardee.
return m.forwardee();
return o->forwardee(m);
}
}

Expand Down
19 changes: 3 additions & 16 deletions src/hotspot/share/gc/serial/defNewGeneration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
#include "gc/shared/gcTimer.hpp"
#include "gc/shared/gcTrace.hpp"
#include "gc/shared/gcTraceTime.inline.hpp"
#include "gc/shared/preservedMarks.inline.hpp"
#include "gc/shared/referencePolicy.hpp"
#include "gc/shared/referenceProcessorPhaseTimes.hpp"
#include "gc/shared/space.hpp"
Expand Down Expand Up @@ -227,7 +226,6 @@ DefNewGeneration::DefNewGeneration(ReservedSpace rs,
const char* policy)
: Generation(rs, initial_size),
_promotion_failed(false),
_preserved_marks_set(false /* in_c_heap */),
_promo_failure_drain_in_progress(false),
_string_dedup_requests()
{
Expand Down Expand Up @@ -609,8 +607,6 @@ bool DefNewGeneration::collect(bool clear_all_soft_refs) {

age_table()->clear();
to()->clear(SpaceDecorator::Mangle);
// The preserved marks should be empty at the start of the GC.
_preserved_marks_set.init(1);

YoungGenScanClosure young_gen_cl(this);
OldGenScanClosure old_gen_cl(this);
Expand Down Expand Up @@ -681,8 +677,6 @@ bool DefNewGeneration::collect(bool clear_all_soft_refs) {
// Reset the PromotionFailureALot counters.
NOT_PRODUCT(heap->reset_promotion_should_fail();)
}
// We should have processed and cleared all the preserved marks.
_preserved_marks_set.reclaim();

heap->trace_heap_after_gc(_gc_tracer);

Expand All @@ -706,32 +700,25 @@ void DefNewGeneration::remove_forwarding_pointers() {
// starts. (The mark word is overloaded: `is_marked()` == `is_forwarded()`.)
struct ResetForwardedMarkWord : ObjectClosure {
void do_object(oop obj) override {
if (obj->is_forwarded()) {
obj->init_mark();
if (obj->is_self_forwarded()) {
Copy link
Member

@albertnetymk albertnetymk Aug 21, 2024

Choose a reason for hiding this comment

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

Why is is_self_forwarded treated specially? I'd expect the is_forwarded case alone to be enough here.

Copy link
Contributor Author

@rkennke rkennke Aug 22, 2024

Choose a reason for hiding this comment

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

Because I'd like self-forwarded marks not to be init-ed. Otherwise we'd have to preserve/restore them. Simply unset_self_forwarded() is enough to get them back to the original state.

Copy link
Member

@albertnetymk albertnetymk Aug 22, 2024

Choose a reason for hiding this comment

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

Otherwise we'd have to preserve/restore them.

OK, then it's incorrect to use init_mark() for self-fwd objs.

This method is for self-fwd objs only. Can we remove the else if part? The non-self-fwd objs are essentially dead, right?

Copy link
Contributor Author

@rkennke rkennke Aug 22, 2024

Choose a reason for hiding this comment

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

I wasn't sure about this. What happens to successfully-promoted objects? Are we sure that no references point to their from-space parts? If yes, then I'd remove the else-if part and place an assert there instead.

Copy link
Member

@stefank stefank Aug 22, 2024

Choose a reason for hiding this comment

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

The else if part is needed when we later turn on UseCompactObjectHeaders, because the "normal" forwarding pointers then destroy the klass pointers, causing the object_iterate to fail to read the size of the objects.

Copy link
Member

@albertnetymk albertnetymk Aug 22, 2024

Choose a reason for hiding this comment

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

What happens to successfully-promoted objects? ...

Successfully-forwarded objs are don't live in eden/from spaces, which are the spaces this closure is applied on. One can't have assert here, because as we iterate over eden/from spaces, we will encounter successfully-forwarded objs, but they should just be skipped.

The else if part is needed when we later turn on UseCompactObjectHeaders...

I believe so, but it should be in the UseCompactObjectHeaders PR, not this one.

Copy link
Contributor Author

@rkennke rkennke Aug 22, 2024

Choose a reason for hiding this comment

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

Ok, thanks for the explanation!

I don't think it's needed for the UCOH part - the iterator fetched Klass*/size from forwardee if it encounters forwarded objects.

Copy link
Member

@stefank stefank Aug 22, 2024

Choose a reason for hiding this comment

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

You didn't change the iterators for the Serial GC, only for Parallel. But, yes, we can deal with this in the UCOH PR.

obj->unset_self_forwarded();
}
}
} cl;
eden()->object_iterate(&cl);
from()->object_iterate(&cl);

restore_preserved_marks();
}

void DefNewGeneration::restore_preserved_marks() {
_preserved_marks_set.restore(nullptr);
}

void DefNewGeneration::handle_promotion_failure(oop old) {
log_debug(gc, promotion)("Promotion failure size = " SIZE_FORMAT ") ", old->size());

_promotion_failed = true;
_promotion_failed_info.register_copy_failure(old->size());
_preserved_marks_set.get()->push_if_necessary(old, old->mark());

ContinuationGCSupport::transform_stack_chunk(old);

// forward to self
old->forward_to(old);
old->forward_to_self();

_promo_failure_scan_stack.push(old);

Expand Down
6 changes: 0 additions & 6 deletions src/hotspot/share/gc/serial/defNewGeneration.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
#include "gc/shared/copyFailedInfo.hpp"
#include "gc/shared/gc_globals.hpp"
#include "gc/shared/generationCounters.hpp"
#include "gc/shared/preservedMarks.hpp"
#include "gc/shared/stringdedup/stringDedup.hpp"
#include "gc/shared/tlab_globals.hpp"
#include "utilities/align.hpp"
Expand Down Expand Up @@ -99,11 +98,6 @@ class DefNewGeneration: public Generation {
// therefore we must remove their forwarding pointers.
void remove_forwarding_pointers();

virtual void restore_preserved_marks();

// Preserved marks
PreservedMarksSet _preserved_marks_set;

Stack<oop, mtGC> _promo_failure_scan_stack;
void drain_promo_failure_scan_stack(void);
bool _promo_failure_drain_in_progress;
Expand Down
Loading