Skip to content
Closed
Changes from 2 commits
Commits
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
83 changes: 36 additions & 47 deletions src/hotspot/share/gc/parallel/psClosure.inline.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,63 +77,52 @@ class PSRootsClosure: public OopClosure {
typedef PSRootsClosure</*promote_immediately=*/false> PSScavengeRootsClosure;
typedef PSRootsClosure</*promote_immediately=*/true> PSPromoteRootsClosure;

// Scavenges a single oop in a ClassLoaderData.
class PSScavengeFromCLDClosure: public OopClosure {
private:
// Scavenges the oop in a ClassLoaderData.
class PSScavengeCLDClosure: public CLDClosure {
PSPromotionManager* _pm;
Copy link
Member

Choose a reason for hiding this comment

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

To make the two classes consistent w.r.t. newlines.

Suggested change
PSPromotionManager* _pm;
PSPromotionManager* _pm;

// Used to redirty a scanned cld if it has oops
// pointing to the young generation after being scanned.
ClassLoaderData* _scanned_cld;
public:
PSScavengeFromCLDClosure(PSPromotionManager* pm) : _pm(pm), _scanned_cld(nullptr) { }
void do_oop(narrowOop* p) { ShouldNotReachHere(); }
void do_oop(oop* p) {
ParallelScavengeHeap* psh = ParallelScavengeHeap::heap();
assert(!psh->is_in_reserved(p), "GC barrier needed");
if (PSScavenge::should_scavenge(p)) {
assert(PSScavenge::should_scavenge(p, true), "revisiting object?");
PSScavengeCLDClosure(PSPromotionManager* pm) : _pm(pm) { }

oop o = RawAccess<IS_NOT_NULL>::oop_load(p);
oop new_obj = _pm->copy_to_survivor_space</*promote_immediately=*/false>(o);
RawAccess<IS_NOT_NULL>::oop_store(p, new_obj);

if (PSScavenge::is_obj_in_young(new_obj)) {
do_cld_barrier();
}
void do_cld(ClassLoaderData* cld) {
// If the cld has not been dirtied we know that there are
// no references into the young gen, so we can skip it.
if (!cld->has_modified_oops()) {
return;
}
}

void set_scanned_cld(ClassLoaderData* cld) {
assert(_scanned_cld == nullptr || cld == nullptr, "Should always only handling one cld at a time");
_scanned_cld = cld;
}
// Scavenges a single oop in a ClassLoaderData.
class CLDOopClosure : public OopClosure {
Copy link
Member

Choose a reason for hiding this comment

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

This makes the do_cld function much harder to read, IMHO. So, three alternatives:

  1. Keep the class outside PSScavengeCLDClosure, just like it was before this patch.
  2. Have the class inside PSScavengeCLDClosure, but not in a function
  3. Have the class inside a function inside PSScavengeCLDClosure.

I prefer (1) because it allows you to look at PSScavengeCLDClosure without having to see the details about the second class. I'm OK with (2) and like that it encapsulates the other class. I don't think we should do (3) because it significantly hurts the readability of do_cld.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have moved the oop-closure class def outside.

PSPromotionManager* _pm;

private:
void do_cld_barrier() {
assert(_scanned_cld != nullptr, "Should not be called without having a scanned cld");
_scanned_cld->record_modified_oops();
}
};
public:
// Records whether this CLD contains oops pointing into young-gen after scavenging.
bool _has_oop_into_young_gen;
Copy link
Member

Choose a reason for hiding this comment

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

Comment says oops but name says oop. Maybe use _has_oops_into_young_gen?


// Scavenges the oop in a ClassLoaderData.
class PSScavengeCLDClosure: public CLDClosure {
private:
PSScavengeFromCLDClosure _oop_closure;
public:
PSScavengeCLDClosure(PSPromotionManager* pm) : _oop_closure(pm) { }
void do_cld(ClassLoaderData* cld) {
// If the cld has not been dirtied we know that there's
// no references into the young gen and we can skip it.
CLDOopClosure(PSPromotionManager* pm) : _pm(pm), _has_oop_into_young_gen(false) {}

void do_oop(narrowOop* p) { ShouldNotReachHere(); }
void do_oop(oop* p) {
ParallelScavengeHeap* psh = ParallelScavengeHeap::heap();
assert(!psh->is_in_reserved(p), "GC barrier needed");
if (PSScavenge::should_scavenge(p)) {
assert(PSScavenge::should_scavenge(p, true), "revisiting object?");

if (cld->has_modified_oops()) {
// Setup the promotion manager to redirty this cld
// if references are left in the young gen.
_oop_closure.set_scanned_cld(cld);
oop o = RawAccess<IS_NOT_NULL>::oop_load(p);
oop new_obj = _pm->copy_to_survivor_space</*promote_immediately=*/false>(o);
RawAccess<IS_NOT_NULL>::oop_store(p, new_obj);

if (PSScavenge::is_obj_in_young(new_obj) && !_has_oop_into_young_gen) {
_has_oop_into_young_gen = true;
}
}
}
} oop_closure{_pm};

// Clean the cld since we're going to scavenge all the metadata.
cld->oops_do(&_oop_closure, ClassLoaderData::_claim_none, /*clear_modified_oops*/true);
// Clean the cld since we're going to scavenge all the metadata.
cld->oops_do(&oop_closure, ClassLoaderData::_claim_none, /*clear_modified_oops*/true);

_oop_closure.set_scanned_cld(nullptr);
if (oop_closure._has_oop_into_young_gen) {
cld->record_modified_oops();
}
}
};
Expand Down