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

8256516: Simplify clearing References #1286

Closed
wants to merge 2 commits into from
Closed
Changes from all 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
2 changes: 1 addition & 1 deletion src/hotspot/share/classfile/javaClasses.hpp
Original file line number Diff line number Diff line change
@@ -875,7 +875,7 @@ class java_lang_ref_Reference: AllStatic {
static inline oop weak_referent_no_keepalive(oop ref);
static inline oop phantom_referent_no_keepalive(oop ref);
static inline oop unknown_referent_no_keepalive(oop ref);
static inline void set_referent_raw(oop ref, oop value);
static inline void clear_referent(oop ref);
static inline HeapWord* referent_addr_raw(oop ref);
static inline oop next(oop ref);
static inline void set_next(oop ref, oop value);
4 changes: 2 additions & 2 deletions src/hotspot/share/classfile/javaClasses.inline.hpp
Original file line number Diff line number Diff line change
@@ -113,8 +113,8 @@ oop java_lang_ref_Reference::unknown_referent_no_keepalive(oop ref) {
return ref->obj_field_access<ON_UNKNOWN_OOP_REF | AS_NO_KEEPALIVE>(_referent_offset);
}

void java_lang_ref_Reference::set_referent_raw(oop ref, oop value) {
ref->obj_field_put_raw(_referent_offset, value);
void java_lang_ref_Reference::clear_referent(oop ref) {
ref->obj_field_put_raw(_referent_offset, nullptr);
}

HeapWord* java_lang_ref_Reference::referent_addr_raw(oop ref) {
13 changes: 10 additions & 3 deletions src/hotspot/share/gc/shared/referenceProcessor.cpp
Original file line number Diff line number Diff line change
@@ -260,8 +260,6 @@ void DiscoveredListIterator::load_ptrs(DEBUG_ONLY(bool allow_null_referent)) {
assert(_current_discovered_addr && oopDesc::is_oop_or_null(discovered),
"Expected an oop or NULL for discovered field at " PTR_FORMAT, p2i(discovered));
_next_discovered = discovered;

_referent_addr = java_lang_ref_Reference::referent_addr_raw(_current_discovered);
_referent = java_lang_ref_Reference::unknown_referent_no_keepalive(_current_discovered);
assert(Universe::heap()->is_in_or_null(_referent),
"Wrong oop found in java.lang.Reference object");
@@ -295,8 +293,17 @@ void DiscoveredListIterator::remove() {
_refs_list.dec_length(1);
}

void DiscoveredListIterator::make_referent_alive() {
HeapWord* addr = java_lang_ref_Reference::referent_addr_raw(_current_discovered);
if (UseCompressedOops) {
_keep_alive->do_oop((narrowOop*)addr);
} else {
_keep_alive->do_oop((oop*)addr);
}
}

void DiscoveredListIterator::clear_referent() {
RawAccess<>::oop_store(_referent_addr, oop(NULL));
java_lang_ref_Reference::clear_referent(_current_discovered);
}

void DiscoveredListIterator::enqueue() {
11 changes: 2 additions & 9 deletions src/hotspot/share/gc/shared/referenceProcessor.hpp
Original file line number Diff line number Diff line change
@@ -71,7 +71,6 @@ class DiscoveredListIterator {
HeapWord* _current_discovered_addr;
oop _next_discovered;

HeapWord* _referent_addr;
oop _referent;

OopClosure* _keep_alive;
@@ -120,14 +119,8 @@ class DiscoveredListIterator {
// Remove the current reference from the list
void remove();

// Make the referent alive.
inline void make_referent_alive() {
if (UseCompressedOops) {
_keep_alive->do_oop((narrowOop*)_referent_addr);
} else {
_keep_alive->do_oop((oop*)_referent_addr);
}
}
// Apply the keep_alive function to the referent address.
void make_referent_alive();
Comment on lines +122 to +123
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if moving this from the .hpp to .cpp has performance implications for callers. Maybe move to .inline.hpp?

Copy link
Author

Choose a reason for hiding this comment

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

The reason for moving it out of the .hpp was of course that the
change to call java_lang_ref_Reference::referent_addr_raw needs to #include
javaClasses.inline.hpp.

I don't see this move having any measureable performance difference, and not
even sure what the sign of any change might be. A better refactoring might
be to package up the common remove/make_referent_alive/move_to_next sequence.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it is probably only affects the path that is infrequently taken, i.e. marking through the resurrected referents during weak reference processing, that is probably only finalizers. It is fine to have it in .cpp.


// Do enqueuing work, i.e. notifying the GC about the changed discovered pointers.
void enqueue();
1 change: 0 additions & 1 deletion src/hotspot/share/gc/shared/referenceProcessor.inline.hpp
Original file line number Diff line number Diff line change
@@ -61,7 +61,6 @@ DiscoveredListIterator::DiscoveredListIterator(DiscoveredList& refs_list,
_current_discovered(refs_list.head()),
_current_discovered_addr(NULL),
_next_discovered(NULL),
_referent_addr(NULL),
_referent(NULL),
_keep_alive(keep_alive),
_is_alive(is_alive),
Original file line number Diff line number Diff line change
@@ -88,8 +88,8 @@ static oop reference_referent(oop reference) {
return CompressedOops::decode(heap_oop);
}

static void reference_set_referent(oop reference, oop referent) {
java_lang_ref_Reference::set_referent_raw(reference, referent);
static void reference_clear_referent(oop reference) {
java_lang_ref_Reference::clear_referent(reference);
}

template <typename T>
@@ -316,7 +316,7 @@ void ShenandoahReferenceProcessor::make_inactive(oop reference, ReferenceType ty
reference_set_next(reference, reference);
} else {
// Clear referent
reference_set_referent(reference, NULL);
reference_clear_referent(reference);
Copy link
Member

Choose a reason for hiding this comment

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

Now I am looking at this code and wonder if we could just inline reference_clear_referent and reference_set_next both in Shenandoah and ZGC code. Probably something for a followup.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to keep the reference_* helper functions as is in ZGC.

}
}

@@ -590,4 +590,4 @@ void ShenandoahReferenceProcessor::collect_statistics() {
discovered[REF_SOFT], discovered[REF_WEAK], discovered[REF_FINAL], discovered[REF_PHANTOM]);
log_info(gc,ref)("Enqueued references: Soft: " SIZE_FORMAT ", Weak: " SIZE_FORMAT ", Final: " SIZE_FORMAT ", Phantom: " SIZE_FORMAT,
enqueued[REF_SOFT], enqueued[REF_WEAK], enqueued[REF_FINAL], enqueued[REF_PHANTOM]);
}
}
6 changes: 3 additions & 3 deletions src/hotspot/share/gc/z/zReferenceProcessor.cpp
Original file line number Diff line number Diff line change
@@ -72,8 +72,8 @@ static oop reference_referent(oop reference) {
return Atomic::load(reference_referent_addr(reference));
}

static void reference_set_referent(oop reference, oop referent) {
java_lang_ref_Reference::set_referent_raw(reference, referent);
static void reference_clear_referent(oop reference) {
java_lang_ref_Reference::clear_referent(reference);
}

static oop* reference_discovered_addr(oop reference) {
@@ -226,7 +226,7 @@ void ZReferenceProcessor::make_inactive(oop reference, ReferenceType type) const
reference_set_next(reference, reference);
} else {
// Clear referent
reference_set_referent(reference, NULL);
reference_clear_referent(reference);
}
}