Skip to content

Commit

Permalink
8267188: gc/stringdedup/TestStringDeduplicationInterned.java fails wi…
Browse files Browse the repository at this point in the history 
…th Shenandoah

Reviewed-by: phh, shade
Backport-of: 7212561dd1ec65d7f31792959f0eaaab6229eaf4
  • Loading branch information
William Kemper authored and Paul Hohensee committed Jun 21, 2023
1 parent 48d7af6 commit 9a49698
Show file tree
Hide file tree
Showing 8 changed files with 83 additions and 77 deletions.
14 changes: 8 additions & 6 deletions src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp
View file
Expand Up @@ -86,9 +86,11 @@ class ShenandoahConcurrentMarkingTask : public AbstractGangTask {
ShenandoahObjToScanQueue* q = _cm->get_queue(worker_id);
ShenandoahReferenceProcessor* rp = heap->ref_processor();
assert(rp != NULL, "need reference processor");
StringDedup::Requests requests;
_cm->mark_loop(worker_id, _terminator, rp,
true /*cancellable*/,
ShenandoahStringDedup::is_enabled() ? ENQUEUE_DEDUP : NO_DEDUP);
ShenandoahStringDedup::is_enabled() ? ENQUEUE_DEDUP : NO_DEDUP,
&requests);
}
};

Expand Down Expand Up @@ -134,6 +136,7 @@ class ShenandoahFinalMarkingTask : public AbstractGangTask {

ShenandoahParallelWorkerSession worker_session(worker_id);
ShenandoahReferenceProcessor* rp = heap->ref_processor();
StringDedup::Requests requests;

// First drain remaining SATB buffers.
{
Expand All @@ -144,14 +147,15 @@ class ShenandoahFinalMarkingTask : public AbstractGangTask {
while (satb_mq_set.apply_closure_to_completed_buffer(&cl)) {}
assert(!heap->has_forwarded_objects(), "Not expected");

ShenandoahMarkRefsClosure<NO_DEDUP> mark_cl(q, rp);
ShenandoahMarkRefsClosure mark_cl(q, rp);
ShenandoahSATBAndRemarkThreadsClosure tc(satb_mq_set,
ShenandoahIUBarrier ? &mark_cl : NULL);
Threads::threads_do(&tc);
}
_cm->mark_loop(worker_id, _terminator, rp,
false /*not cancellable*/,
_dedup_string ? ENQUEUE_DEDUP : NO_DEDUP);
_dedup_string ? ENQUEUE_DEDUP : NO_DEDUP,
&requests);
assert(_cm->task_queues()->is_empty(), "Should be empty");
}
};
Expand Down Expand Up @@ -189,9 +193,7 @@ ShenandoahMarkConcurrentRootsTask::ShenandoahMarkConcurrentRootsTask(ShenandoahO
void ShenandoahMarkConcurrentRootsTask::work(uint worker_id) {
ShenandoahConcurrentWorkerSession worker_session(worker_id);
ShenandoahObjToScanQueue* q = _queue_set->queue(worker_id);
// Cannot enable string deduplication during root scanning. Otherwise,
// may result lock inversion between stack watermark and string dedup queue lock.
ShenandoahMarkRefsClosure<NO_DEDUP> cl(q, _rp);
ShenandoahMarkRefsClosure cl(q, _rp);
_root_scanner.roots_do(&cl, worker_id);
}

Expand Down
41 changes: 20 additions & 21 deletions src/hotspot/share/gc/shenandoah/shenandoahMark.cpp
View file
Expand Up @@ -36,7 +36,6 @@

ShenandoahMarkRefsSuperClosure::ShenandoahMarkRefsSuperClosure(ShenandoahObjToScanQueue* q, ShenandoahReferenceProcessor* rp) :
MetadataVisitingOopIterateClosure(rp),
_stringDedup_requests(),
_queue(q),
_mark_context(ShenandoahHeap::heap()->marking_context()),
_weak(false)
Expand All @@ -56,7 +55,7 @@ void ShenandoahMark::clear() {
}

template <bool CANCELLABLE, StringDedupMode STRING_DEDUP>
void ShenandoahMark::mark_loop_prework(uint w, TaskTerminator *t, ShenandoahReferenceProcessor *rp) {
void ShenandoahMark::mark_loop_prework(uint w, TaskTerminator *t, ShenandoahReferenceProcessor *rp, StringDedup::Requests* const req) {
ShenandoahObjToScanQueue* q = get_queue(w);

ShenandoahHeap* const heap = ShenandoahHeap::heap();
Expand All @@ -66,60 +65,60 @@ void ShenandoahMark::mark_loop_prework(uint w, TaskTerminator *t, ShenandoahRefe
// play nice with specialized_oop_iterators.
if (heap->unload_classes()) {
if (heap->has_forwarded_objects()) {
using Closure = ShenandoahMarkUpdateRefsMetadataClosure<STRING_DEDUP>;
using Closure = ShenandoahMarkUpdateRefsMetadataClosure;
Closure cl(q, rp);
mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
mark_loop_work<Closure, CANCELLABLE, STRING_DEDUP>(&cl, ld, w, t, req);
} else {
using Closure = ShenandoahMarkRefsMetadataClosure<STRING_DEDUP>;
using Closure = ShenandoahMarkRefsMetadataClosure;
Closure cl(q, rp);
mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
mark_loop_work<Closure, CANCELLABLE, STRING_DEDUP>(&cl, ld, w, t, req);
}
} else {
if (heap->has_forwarded_objects()) {
using Closure = ShenandoahMarkUpdateRefsClosure<STRING_DEDUP>;
using Closure = ShenandoahMarkUpdateRefsClosure;
Closure cl(q, rp);
mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
mark_loop_work<Closure, CANCELLABLE, STRING_DEDUP>(&cl, ld, w, t, req);
} else {
using Closure = ShenandoahMarkRefsClosure<STRING_DEDUP>;
using Closure = ShenandoahMarkRefsClosure;
Closure cl(q, rp);
mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
mark_loop_work<Closure, CANCELLABLE, STRING_DEDUP>(&cl, ld, w, t, req);
}
}

heap->flush_liveness_cache(w);
}

void ShenandoahMark::mark_loop(uint worker_id, TaskTerminator* terminator, ShenandoahReferenceProcessor *rp,
bool cancellable, StringDedupMode dedup_mode) {
bool cancellable, StringDedupMode dedup_mode, StringDedup::Requests* const req) {
if (cancellable) {
switch(dedup_mode) {
case NO_DEDUP:
mark_loop_prework<true, NO_DEDUP>(worker_id, terminator, rp);
mark_loop_prework<true, NO_DEDUP>(worker_id, terminator, rp, req);
break;
case ENQUEUE_DEDUP:
mark_loop_prework<true, ENQUEUE_DEDUP>(worker_id, terminator, rp);
mark_loop_prework<true, ENQUEUE_DEDUP>(worker_id, terminator, rp, req);
break;
case ALWAYS_DEDUP:
mark_loop_prework<true, ALWAYS_DEDUP>(worker_id, terminator, rp);
mark_loop_prework<true, ALWAYS_DEDUP>(worker_id, terminator, rp, req);
break;
}
} else {
switch(dedup_mode) {
case NO_DEDUP:
mark_loop_prework<false, NO_DEDUP>(worker_id, terminator, rp);
mark_loop_prework<false, NO_DEDUP>(worker_id, terminator, rp, req);
break;
case ENQUEUE_DEDUP:
mark_loop_prework<false, ENQUEUE_DEDUP>(worker_id, terminator, rp);
mark_loop_prework<false, ENQUEUE_DEDUP>(worker_id, terminator, rp, req);
break;
case ALWAYS_DEDUP:
mark_loop_prework<false, ALWAYS_DEDUP>(worker_id, terminator, rp);
mark_loop_prework<false, ALWAYS_DEDUP>(worker_id, terminator, rp, req);
break;
}
}
}

template <class T, bool CANCELLABLE>
void ShenandoahMark::mark_loop_work(T* cl, ShenandoahLiveData* live_data, uint worker_id, TaskTerminator *terminator) {
template <class T, bool CANCELLABLE, StringDedupMode STRING_DEDUP>
void ShenandoahMark::mark_loop_work(T* cl, ShenandoahLiveData* live_data, uint worker_id, TaskTerminator *terminator, StringDedup::Requests* const req) {
uintx stride = ShenandoahMarkLoopStride;

ShenandoahHeap* heap = ShenandoahHeap::heap();
Expand Down Expand Up @@ -147,7 +146,7 @@ void ShenandoahMark::mark_loop_work(T* cl, ShenandoahLiveData* live_data, uint w

for (uint i = 0; i < stride; i++) {
if (q->pop(t)) {
do_task<T>(q, cl, live_data, &t);
do_task<T, STRING_DEDUP>(q, cl, live_data, req, &t);
} else {
assert(q->is_empty(), "Must be empty");
q = queues->claim_next();
Expand Down Expand Up @@ -176,7 +175,7 @@ void ShenandoahMark::mark_loop_work(T* cl, ShenandoahLiveData* live_data, uint w
for (uint i = 0; i < stride; i++) {
if (q->pop(t) ||
queues->steal(worker_id, t)) {
do_task<T>(q, cl, live_data, &t);
do_task<T, STRING_DEDUP>(q, cl, live_data, req, &t);
work++;
} else {
break;
Expand Down
18 changes: 10 additions & 8 deletions src/hotspot/share/gc/shenandoah/shenandoahMark.hpp
View file
Expand Up @@ -45,8 +45,8 @@ class ShenandoahMark: public StackObj {
ShenandoahMark();

public:
template<class T, StringDedupMode STRING_DEDUP>
static inline void mark_through_ref(T* p, ShenandoahObjToScanQueue* q, ShenandoahMarkingContext* const mark_context, StringDedup::Requests* const req, bool weak);
template<class T>
static inline void mark_through_ref(T* p, ShenandoahObjToScanQueue* q, ShenandoahMarkingContext* const mark_context, bool weak);

static void clear();

Expand All @@ -56,8 +56,8 @@ class ShenandoahMark: public StackObj {

// ---------- Marking loop and tasks
private:
template <class T>
inline void do_task(ShenandoahObjToScanQueue* q, T* cl, ShenandoahLiveData* live_data, ShenandoahMarkTask* task);
template <class T, StringDedupMode STRING_DEDUP>
inline void do_task(ShenandoahObjToScanQueue* q, T* cl, ShenandoahLiveData* live_data, StringDedup::Requests* const req, ShenandoahMarkTask* task);

template <class T>
inline void do_chunked_array_start(ShenandoahObjToScanQueue* q, T* cl, oop array, bool weak);
Expand All @@ -67,15 +67,17 @@ class ShenandoahMark: public StackObj {

inline void count_liveness(ShenandoahLiveData* live_data, oop obj);

template <class T, bool CANCELLABLE>
void mark_loop_work(T* cl, ShenandoahLiveData* live_data, uint worker_id, TaskTerminator *t);
template <class T, bool CANCELLABLE,StringDedupMode STRING_DEDUP>
void mark_loop_work(T* cl, ShenandoahLiveData* live_data, uint worker_id, TaskTerminator *t, StringDedup::Requests* const req);

template <bool CANCELLABLE, StringDedupMode STRING_DEDUP>
void mark_loop_prework(uint worker_id, TaskTerminator *terminator, ShenandoahReferenceProcessor *rp);
void mark_loop_prework(uint worker_id, TaskTerminator *terminator, ShenandoahReferenceProcessor *rp, StringDedup::Requests* const req);

template <StringDedupMode STRING_DEDUP>
inline void dedup_string(oop obj, StringDedup::Requests* const req);
protected:
void mark_loop(uint worker_id, TaskTerminator* terminator, ShenandoahReferenceProcessor *rp,
bool cancellable, StringDedupMode dedup_mode);
bool cancellable, StringDedupMode dedup_mode, StringDedup::Requests* const req);
};

#endif // SHARE_GC_SHENANDOAH_SHENANDOAHMARK_HPP
Expand Down
45 changes: 20 additions & 25 deletions src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp
View file
Expand Up @@ -40,8 +40,22 @@
#include "runtime/prefetch.inline.hpp"
#include "utilities/powerOfTwo.hpp"

template <class T>
void ShenandoahMark::do_task(ShenandoahObjToScanQueue* q, T* cl, ShenandoahLiveData* live_data, ShenandoahMarkTask* task) {
template <StringDedupMode STRING_DEDUP>
void ShenandoahMark::dedup_string(oop obj, StringDedup::Requests* const req) {
if (STRING_DEDUP == ENQUEUE_DEDUP) {
if (ShenandoahStringDedup::is_candidate(obj)) {
req->add(obj);
}
} else if (STRING_DEDUP == ALWAYS_DEDUP) {
if (ShenandoahStringDedup::is_string_candidate(obj) &&
!ShenandoahStringDedup::dedup_requested(obj)) {
req->add(obj);
}
}
}

template <class T, StringDedupMode STRING_DEDUP>
void ShenandoahMark::do_task(ShenandoahObjToScanQueue* q, T* cl, ShenandoahLiveData* live_data, StringDedup::Requests* const req, ShenandoahMarkTask* task) {
oop obj = task->obj();

shenandoah_assert_not_forwarded(NULL, obj);
Expand All @@ -56,6 +70,7 @@ void ShenandoahMark::do_task(ShenandoahObjToScanQueue* q, T* cl, ShenandoahLiveD
if (obj->is_instance()) {
// Case 1: Normal oop, process as usual.
obj->oop_iterate(cl);
dedup_string<STRING_DEDUP>(obj, req);
} else if (obj->is_objArray()) {
// Case 2: Object array instance and no chunk is set. Must be the first
// time we visit it, start the chunked processing.
Expand Down Expand Up @@ -208,7 +223,6 @@ inline void ShenandoahMark::do_chunked_array(ShenandoahObjToScanQueue* q, T* cl,

class ShenandoahSATBBufferClosure : public SATBBufferClosure {
private:
StringDedup::Requests _stringdedup_requests;
ShenandoahObjToScanQueue* _queue;
ShenandoahHeap* _heap;
ShenandoahMarkingContext* const _mark_context;
Expand All @@ -222,24 +236,15 @@ class ShenandoahSATBBufferClosure : public SATBBufferClosure {

void do_buffer(void **buffer, size_t size) {
assert(size == 0 || !_heap->has_forwarded_objects(), "Forwarded objects are not expected here");
if (ShenandoahStringDedup::is_enabled()) {
do_buffer_impl<ENQUEUE_DEDUP>(buffer, size);
} else {
do_buffer_impl<NO_DEDUP>(buffer, size);
}
}

template<StringDedupMode STRING_DEDUP>
void do_buffer_impl(void **buffer, size_t size) {
for (size_t i = 0; i < size; ++i) {
oop *p = (oop *) &buffer[i];
ShenandoahMark::mark_through_ref<oop, STRING_DEDUP>(p, _queue, _mark_context, &_stringdedup_requests, false);
ShenandoahMark::mark_through_ref<oop>(p, _queue, _mark_context, false);
}
}
};

template<class T, StringDedupMode STRING_DEDUP>
inline void ShenandoahMark::mark_through_ref(T* p, ShenandoahObjToScanQueue* q, ShenandoahMarkingContext* const mark_context, StringDedup::Requests* const req, bool weak) {
template<class T>
inline void ShenandoahMark::mark_through_ref(T* p, ShenandoahObjToScanQueue* q, ShenandoahMarkingContext* const mark_context, bool weak) {
T o = RawAccess<>::oop_load(p);
if (!CompressedOops::is_null(o)) {
oop obj = CompressedOops::decode_not_null(o);
Expand All @@ -257,16 +262,6 @@ inline void ShenandoahMark::mark_through_ref(T* p, ShenandoahObjToScanQueue* q,
if (marked) {
bool pushed = q->push(ShenandoahMarkTask(obj, skip_live, weak));
assert(pushed, "overflow queue should always succeed pushing");

if ((STRING_DEDUP == ENQUEUE_DEDUP) && ShenandoahStringDedup::is_candidate(obj)) {
assert(ShenandoahStringDedup::is_enabled(), "Must be enabled");
req->add(obj);
} else if ((STRING_DEDUP == ALWAYS_DEDUP) &&
ShenandoahStringDedup::is_string_candidate(obj) &&
!ShenandoahStringDedup::dedup_requested(obj)) {
assert(ShenandoahStringDedup::is_enabled(), "Must be enabled");
req->add(obj);
}
}

shenandoah_assert_marked(p, obj);
Expand Down
17 changes: 6 additions & 11 deletions src/hotspot/share/gc/shenandoah/shenandoahOopClosures.hpp
View file
Expand Up @@ -40,13 +40,12 @@ enum StringDedupMode {

class ShenandoahMarkRefsSuperClosure : public MetadataVisitingOopIterateClosure {
private:
StringDedup::Requests _stringDedup_requests;
ShenandoahObjToScanQueue* _queue;
ShenandoahMarkingContext* const _mark_context;
bool _weak;

protected:
template <class T, StringDedupMode STRING_DEDUP>
template <class T>
void work(T *p);

public:
Expand All @@ -65,7 +64,7 @@ class ShenandoahMarkUpdateRefsSuperClosure : public ShenandoahMarkRefsSuperClosu
protected:
ShenandoahHeap* const _heap;

template <class T, StringDedupMode STRING_DEDUP>
template <class T>
inline void work(T* p);

public:
Expand All @@ -76,11 +75,10 @@ class ShenandoahMarkUpdateRefsSuperClosure : public ShenandoahMarkRefsSuperClosu
};
};

template <StringDedupMode STRING_DEDUP>
class ShenandoahMarkUpdateRefsClosure : public ShenandoahMarkUpdateRefsSuperClosure {
private:
template <class T>
inline void do_oop_work(T* p) { work<T, STRING_DEDUP>(p); }
inline void do_oop_work(T* p) { work<T>(p); }

public:
ShenandoahMarkUpdateRefsClosure(ShenandoahObjToScanQueue* q, ShenandoahReferenceProcessor* rp) :
Expand All @@ -91,11 +89,10 @@ class ShenandoahMarkUpdateRefsClosure : public ShenandoahMarkUpdateRefsSuperClos
virtual bool do_metadata() { return false; }
};

template <StringDedupMode STRING_DEDUP>
class ShenandoahMarkUpdateRefsMetadataClosure : public ShenandoahMarkUpdateRefsSuperClosure {
private:
template <class T>
inline void do_oop_work(T* p) { work<T, STRING_DEDUP>(p); }
inline void do_oop_work(T* p) { work<T>(p); }

public:
ShenandoahMarkUpdateRefsMetadataClosure(ShenandoahObjToScanQueue* q, ShenandoahReferenceProcessor* rp) :
Expand All @@ -107,11 +104,10 @@ class ShenandoahMarkUpdateRefsMetadataClosure : public ShenandoahMarkUpdateRefsS
};


template <StringDedupMode STRING_DEDUP>
class ShenandoahMarkRefsClosure : public ShenandoahMarkRefsSuperClosure {
private:
template <class T>
inline void do_oop_work(T* p) { work<T, STRING_DEDUP>(p); }
inline void do_oop_work(T* p) { work<T>(p); }

public:
ShenandoahMarkRefsClosure(ShenandoahObjToScanQueue* q, ShenandoahReferenceProcessor* rp) :
Expand All @@ -123,11 +119,10 @@ class ShenandoahMarkRefsClosure : public ShenandoahMarkRefsSuperClosure {
};


template <StringDedupMode STRING_DEDUP>
class ShenandoahMarkRefsMetadataClosure : public ShenandoahMarkRefsSuperClosure {
private:
template <class T>
inline void do_oop_work(T* p) { work<T, STRING_DEDUP>(p); }
inline void do_oop_work(T* p) { work<T>(p); }

public:
ShenandoahMarkRefsMetadataClosure(ShenandoahObjToScanQueue* q, ShenandoahReferenceProcessor* rp) :
Expand Down
8 changes: 4 additions & 4 deletions src/hotspot/share/gc/shenandoah/shenandoahOopClosures.inline.hpp
View file
Expand Up @@ -30,18 +30,18 @@
#include "gc/shenandoah/shenandoahHeap.inline.hpp"
#include "gc/shenandoah/shenandoahMark.inline.hpp"

template<class T, StringDedupMode STRING_DEDUP>
template<class T>
inline void ShenandoahMarkRefsSuperClosure::work(T* p) {
ShenandoahMark::mark_through_ref<T, STRING_DEDUP>(p, _queue, _mark_context, &_stringDedup_requests, _weak);
ShenandoahMark::mark_through_ref<T>(p, _queue, _mark_context, _weak);
}

template<class T, StringDedupMode STRING_DEDUP>
template<class T>
inline void ShenandoahMarkUpdateRefsSuperClosure::work(T* p) {
// Update the location
_heap->update_with_forwarded(p);

// ...then do the usual thing
ShenandoahMarkRefsSuperClosure::work<T, STRING_DEDUP>(p);
ShenandoahMarkRefsSuperClosure::work<T>(p);
}

template<class T>
Expand Down

1 comment on commit 9a49698

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Review

Issues

Please sign in to comment.