Skip to content

Commit 7212561

Browse files
zhengyu123zhengyu123
committed
8267188: gc/stringdedup/TestStringDeduplicationInterned.java fails with Shenandoah
Reviewed-by: rkennke, shade
1 parent e36cbd8 commit 7212561

File tree

8 files changed

+84
-77
lines changed

8 files changed

+84
-77
lines changed

‎src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -86,9 +86,11 @@ class ShenandoahConcurrentMarkingTask : public AbstractGangTask {
8686
ShenandoahObjToScanQueue* q = _cm->get_queue(worker_id);
8787
ShenandoahReferenceProcessor* rp = heap->ref_processor();
8888
assert(rp != NULL, "need reference processor");
89+
StringDedup::Requests requests;
8990
_cm->mark_loop(worker_id, _terminator, rp,
9091
true /*cancellable*/,
91-
ShenandoahStringDedup::is_enabled() ? ENQUEUE_DEDUP : NO_DEDUP);
92+
ShenandoahStringDedup::is_enabled() ? ENQUEUE_DEDUP : NO_DEDUP,
93+
&requests);
9294
}
9395
};
9496

@@ -134,6 +136,7 @@ class ShenandoahFinalMarkingTask : public AbstractGangTask {
134136

135137
ShenandoahParallelWorkerSession worker_session(worker_id);
136138
ShenandoahReferenceProcessor* rp = heap->ref_processor();
139+
StringDedup::Requests requests;
137140

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

147-
ShenandoahMarkRefsClosure<NO_DEDUP> mark_cl(q, rp);
150+
ShenandoahMarkRefsClosure mark_cl(q, rp);
148151
ShenandoahSATBAndRemarkThreadsClosure tc(satb_mq_set,
149152
ShenandoahIUBarrier ? &mark_cl : NULL);
150153
Threads::threads_do(&tc);
151154
}
152155
_cm->mark_loop(worker_id, _terminator, rp,
153156
false /*not cancellable*/,
154-
_dedup_string ? ENQUEUE_DEDUP : NO_DEDUP);
157+
_dedup_string ? ENQUEUE_DEDUP : NO_DEDUP,
158+
&requests);
155159
assert(_cm->task_queues()->is_empty(), "Should be empty");
156160
}
157161
};
@@ -189,9 +193,7 @@ ShenandoahMarkConcurrentRootsTask::ShenandoahMarkConcurrentRootsTask(ShenandoahO
189193
void ShenandoahMarkConcurrentRootsTask::work(uint worker_id) {
190194
ShenandoahConcurrentWorkerSession worker_session(worker_id);
191195
ShenandoahObjToScanQueue* q = _queue_set->queue(worker_id);
192-
// Cannot enable string deduplication during root scanning. Otherwise,
193-
// may result lock inversion between stack watermark and string dedup queue lock.
194-
ShenandoahMarkRefsClosure<NO_DEDUP> cl(q, _rp);
196+
ShenandoahMarkRefsClosure cl(q, _rp);
195197
_root_scanner.roots_do(&cl, worker_id);
196198
}
197199

‎src/hotspot/share/gc/shenandoah/shenandoahMark.cpp

Lines changed: 20 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@
3636

3737
ShenandoahMarkRefsSuperClosure::ShenandoahMarkRefsSuperClosure(ShenandoahObjToScanQueue* q, ShenandoahReferenceProcessor* rp) :
3838
MetadataVisitingOopIterateClosure(rp),
39-
_stringDedup_requests(),
4039
_queue(q),
4140
_mark_context(ShenandoahHeap::heap()->marking_context()),
4241
_weak(false)
@@ -56,7 +55,7 @@ void ShenandoahMark::clear() {
5655
}
5756

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

6261
ShenandoahHeap* const heap = ShenandoahHeap::heap();
@@ -66,60 +65,60 @@ void ShenandoahMark::mark_loop_prework(uint w, TaskTerminator *t, ShenandoahRefe
6665
// play nice with specialized_oop_iterators.
6766
if (heap->unload_classes()) {
6867
if (heap->has_forwarded_objects()) {
69-
using Closure = ShenandoahMarkUpdateRefsMetadataClosure<STRING_DEDUP>;
68+
using Closure = ShenandoahMarkUpdateRefsMetadataClosure;
7069
Closure cl(q, rp);
71-
mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
70+
mark_loop_work<Closure, CANCELLABLE, STRING_DEDUP>(&cl, ld, w, t, req);
7271
} else {
73-
using Closure = ShenandoahMarkRefsMetadataClosure<STRING_DEDUP>;
72+
using Closure = ShenandoahMarkRefsMetadataClosure;
7473
Closure cl(q, rp);
75-
mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
74+
mark_loop_work<Closure, CANCELLABLE, STRING_DEDUP>(&cl, ld, w, t, req);
7675
}
7776
} else {
7877
if (heap->has_forwarded_objects()) {
79-
using Closure = ShenandoahMarkUpdateRefsClosure<STRING_DEDUP>;
78+
using Closure = ShenandoahMarkUpdateRefsClosure;
8079
Closure cl(q, rp);
81-
mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
80+
mark_loop_work<Closure, CANCELLABLE, STRING_DEDUP>(&cl, ld, w, t, req);
8281
} else {
83-
using Closure = ShenandoahMarkRefsClosure<STRING_DEDUP>;
82+
using Closure = ShenandoahMarkRefsClosure;
8483
Closure cl(q, rp);
85-
mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
84+
mark_loop_work<Closure, CANCELLABLE, STRING_DEDUP>(&cl, ld, w, t, req);
8685
}
8786
}
8887

8988
heap->flush_liveness_cache(w);
9089
}
9190

9291
void ShenandoahMark::mark_loop(uint worker_id, TaskTerminator* terminator, ShenandoahReferenceProcessor *rp,
93-
bool cancellable, StringDedupMode dedup_mode) {
92+
bool cancellable, StringDedupMode dedup_mode, StringDedup::Requests* const req) {
9493
if (cancellable) {
9594
switch(dedup_mode) {
9695
case NO_DEDUP:
97-
mark_loop_prework<true, NO_DEDUP>(worker_id, terminator, rp);
96+
mark_loop_prework<true, NO_DEDUP>(worker_id, terminator, rp, req);
9897
break;
9998
case ENQUEUE_DEDUP:
100-
mark_loop_prework<true, ENQUEUE_DEDUP>(worker_id, terminator, rp);
99+
mark_loop_prework<true, ENQUEUE_DEDUP>(worker_id, terminator, rp, req);
101100
break;
102101
case ALWAYS_DEDUP:
103-
mark_loop_prework<true, ALWAYS_DEDUP>(worker_id, terminator, rp);
102+
mark_loop_prework<true, ALWAYS_DEDUP>(worker_id, terminator, rp, req);
104103
break;
105104
}
106105
} else {
107106
switch(dedup_mode) {
108107
case NO_DEDUP:
109-
mark_loop_prework<false, NO_DEDUP>(worker_id, terminator, rp);
108+
mark_loop_prework<false, NO_DEDUP>(worker_id, terminator, rp, req);
110109
break;
111110
case ENQUEUE_DEDUP:
112-
mark_loop_prework<false, ENQUEUE_DEDUP>(worker_id, terminator, rp);
111+
mark_loop_prework<false, ENQUEUE_DEDUP>(worker_id, terminator, rp, req);
113112
break;
114113
case ALWAYS_DEDUP:
115-
mark_loop_prework<false, ALWAYS_DEDUP>(worker_id, terminator, rp);
114+
mark_loop_prework<false, ALWAYS_DEDUP>(worker_id, terminator, rp, req);
116115
break;
117116
}
118117
}
119118
}
120119

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

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

148147
for (uint i = 0; i < stride; i++) {
149148
if (q->pop(t)) {
150-
do_task<T>(q, cl, live_data, &t);
149+
do_task<T, STRING_DEDUP>(q, cl, live_data, req, &t);
151150
} else {
152151
assert(q->is_empty(), "Must be empty");
153152
q = queues->claim_next();
@@ -176,7 +175,7 @@ void ShenandoahMark::mark_loop_work(T* cl, ShenandoahLiveData* live_data, uint w
176175
for (uint i = 0; i < stride; i++) {
177176
if (q->pop(t) ||
178177
queues->steal(worker_id, t)) {
179-
do_task<T>(q, cl, live_data, &t);
178+
do_task<T, STRING_DEDUP>(q, cl, live_data, req, &t);
180179
work++;
181180
} else {
182181
break;

‎src/hotspot/share/gc/shenandoah/shenandoahMark.hpp

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,8 @@ class ShenandoahMark: public StackObj {
4545
ShenandoahMark();
4646

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

5151
static void clear();
5252

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

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

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

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

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

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

76+
template <StringDedupMode STRING_DEDUP>
77+
inline void dedup_string(oop obj, StringDedup::Requests* const req);
7678
protected:
7779
void mark_loop(uint worker_id, TaskTerminator* terminator, ShenandoahReferenceProcessor *rp,
78-
bool cancellable, StringDedupMode dedup_mode);
80+
bool cancellable, StringDedupMode dedup_mode, StringDedup::Requests* const req);
7981
};
8082

8183
#endif // SHARE_GC_SHENANDOAH_SHENANDOAHMARK_HPP

‎src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp

Lines changed: 20 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,22 @@
4040
#include "runtime/prefetch.inline.hpp"
4141
#include "utilities/powerOfTwo.hpp"
4242

43-
template <class T>
44-
void ShenandoahMark::do_task(ShenandoahObjToScanQueue* q, T* cl, ShenandoahLiveData* live_data, ShenandoahMarkTask* task) {
43+
template <StringDedupMode STRING_DEDUP>
44+
void ShenandoahMark::dedup_string(oop obj, StringDedup::Requests* const req) {
45+
if (STRING_DEDUP == ENQUEUE_DEDUP) {
46+
if (ShenandoahStringDedup::is_candidate(obj)) {
47+
req->add(obj);
48+
}
49+
} else if (STRING_DEDUP == ALWAYS_DEDUP) {
50+
if (ShenandoahStringDedup::is_string_candidate(obj) &&
51+
!ShenandoahStringDedup::dedup_requested(obj)) {
52+
req->add(obj);
53+
}
54+
}
55+
}
56+
57+
template <class T, StringDedupMode STRING_DEDUP>
58+
void ShenandoahMark::do_task(ShenandoahObjToScanQueue* q, T* cl, ShenandoahLiveData* live_data, StringDedup::Requests* const req, ShenandoahMarkTask* task) {
4559
oop obj = task->obj();
4660

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

209224
class ShenandoahSATBBufferClosure : public SATBBufferClosure {
210225
private:
211-
StringDedup::Requests _stringdedup_requests;
212226
ShenandoahObjToScanQueue* _queue;
213227
ShenandoahHeap* _heap;
214228
ShenandoahMarkingContext* const _mark_context;
@@ -222,24 +236,15 @@ class ShenandoahSATBBufferClosure : public SATBBufferClosure {
222236

223237
void do_buffer(void **buffer, size_t size) {
224238
assert(size == 0 || !_heap->has_forwarded_objects(), "Forwarded objects are not expected here");
225-
if (ShenandoahStringDedup::is_enabled()) {
226-
do_buffer_impl<ENQUEUE_DEDUP>(buffer, size);
227-
} else {
228-
do_buffer_impl<NO_DEDUP>(buffer, size);
229-
}
230-
}
231-
232-
template<StringDedupMode STRING_DEDUP>
233-
void do_buffer_impl(void **buffer, size_t size) {
234239
for (size_t i = 0; i < size; ++i) {
235240
oop *p = (oop *) &buffer[i];
236-
ShenandoahMark::mark_through_ref<oop, STRING_DEDUP>(p, _queue, _mark_context, &_stringdedup_requests, false);
241+
ShenandoahMark::mark_through_ref<oop>(p, _queue, _mark_context, false);
237242
}
238243
}
239244
};
240245

241-
template<class T, StringDedupMode STRING_DEDUP>
242-
inline void ShenandoahMark::mark_through_ref(T* p, ShenandoahObjToScanQueue* q, ShenandoahMarkingContext* const mark_context, StringDedup::Requests* const req, bool weak) {
246+
template<class T>
247+
inline void ShenandoahMark::mark_through_ref(T* p, ShenandoahObjToScanQueue* q, ShenandoahMarkingContext* const mark_context, bool weak) {
243248
T o = RawAccess<>::oop_load(p);
244249
if (!CompressedOops::is_null(o)) {
245250
oop obj = CompressedOops::decode_not_null(o);
@@ -257,16 +262,6 @@ inline void ShenandoahMark::mark_through_ref(T* p, ShenandoahObjToScanQueue* q,
257262
if (marked) {
258263
bool pushed = q->push(ShenandoahMarkTask(obj, skip_live, weak));
259264
assert(pushed, "overflow queue should always succeed pushing");
260-
261-
if ((STRING_DEDUP == ENQUEUE_DEDUP) && ShenandoahStringDedup::is_candidate(obj)) {
262-
assert(ShenandoahStringDedup::is_enabled(), "Must be enabled");
263-
req->add(obj);
264-
} else if ((STRING_DEDUP == ALWAYS_DEDUP) &&
265-
ShenandoahStringDedup::is_string_candidate(obj) &&
266-
!ShenandoahStringDedup::dedup_requested(obj)) {
267-
assert(ShenandoahStringDedup::is_enabled(), "Must be enabled");
268-
req->add(obj);
269-
}
270265
}
271266

272267
shenandoah_assert_marked(p, obj);

‎src/hotspot/share/gc/shenandoah/shenandoahOopClosures.hpp

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -40,13 +40,12 @@ enum StringDedupMode {
4040

4141
class ShenandoahMarkRefsSuperClosure : public MetadataVisitingOopIterateClosure {
4242
private:
43-
StringDedup::Requests _stringDedup_requests;
4443
ShenandoahObjToScanQueue* _queue;
4544
ShenandoahMarkingContext* const _mark_context;
4645
bool _weak;
4746

4847
protected:
49-
template <class T, StringDedupMode STRING_DEDUP>
48+
template <class T>
5049
void work(T *p);
5150

5251
public:
@@ -65,7 +64,7 @@ class ShenandoahMarkUpdateRefsSuperClosure : public ShenandoahMarkRefsSuperClosu
6564
protected:
6665
ShenandoahHeap* const _heap;
6766

68-
template <class T, StringDedupMode STRING_DEDUP>
67+
template <class T>
6968
inline void work(T* p);
7069

7170
public:
@@ -76,11 +75,10 @@ class ShenandoahMarkUpdateRefsSuperClosure : public ShenandoahMarkRefsSuperClosu
7675
};
7776
};
7877

79-
template <StringDedupMode STRING_DEDUP>
8078
class ShenandoahMarkUpdateRefsClosure : public ShenandoahMarkUpdateRefsSuperClosure {
8179
private:
8280
template <class T>
83-
inline void do_oop_work(T* p) { work<T, STRING_DEDUP>(p); }
81+
inline void do_oop_work(T* p) { work<T>(p); }
8482

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

94-
template <StringDedupMode STRING_DEDUP>
9592
class ShenandoahMarkUpdateRefsMetadataClosure : public ShenandoahMarkUpdateRefsSuperClosure {
9693
private:
9794
template <class T>
98-
inline void do_oop_work(T* p) { work<T, STRING_DEDUP>(p); }
95+
inline void do_oop_work(T* p) { work<T>(p); }
9996

10097
public:
10198
ShenandoahMarkUpdateRefsMetadataClosure(ShenandoahObjToScanQueue* q, ShenandoahReferenceProcessor* rp) :
@@ -107,11 +104,10 @@ class ShenandoahMarkUpdateRefsMetadataClosure : public ShenandoahMarkUpdateRefsS
107104
};
108105

109106

110-
template <StringDedupMode STRING_DEDUP>
111107
class ShenandoahMarkRefsClosure : public ShenandoahMarkRefsSuperClosure {
112108
private:
113109
template <class T>
114-
inline void do_oop_work(T* p) { work<T, STRING_DEDUP>(p); }
110+
inline void do_oop_work(T* p) { work<T>(p); }
115111

116112
public:
117113
ShenandoahMarkRefsClosure(ShenandoahObjToScanQueue* q, ShenandoahReferenceProcessor* rp) :
@@ -123,11 +119,10 @@ class ShenandoahMarkRefsClosure : public ShenandoahMarkRefsSuperClosure {
123119
};
124120

125121

126-
template <StringDedupMode STRING_DEDUP>
127122
class ShenandoahMarkRefsMetadataClosure : public ShenandoahMarkRefsSuperClosure {
128123
private:
129124
template <class T>
130-
inline void do_oop_work(T* p) { work<T, STRING_DEDUP>(p); }
125+
inline void do_oop_work(T* p) { work<T>(p); }
131126

132127
public:
133128
ShenandoahMarkRefsMetadataClosure(ShenandoahObjToScanQueue* q, ShenandoahReferenceProcessor* rp) :

‎src/hotspot/share/gc/shenandoah/shenandoahOopClosures.inline.hpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,18 +30,18 @@
3030
#include "gc/shenandoah/shenandoahHeap.inline.hpp"
3131
#include "gc/shenandoah/shenandoahMark.inline.hpp"
3232

33-
template<class T, StringDedupMode STRING_DEDUP>
33+
template<class T>
3434
inline void ShenandoahMarkRefsSuperClosure::work(T* p) {
35-
ShenandoahMark::mark_through_ref<T, STRING_DEDUP>(p, _queue, _mark_context, &_stringDedup_requests, _weak);
35+
ShenandoahMark::mark_through_ref<T>(p, _queue, _mark_context, _weak);
3636
}
3737

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

4343
// ...then do the usual thing
44-
ShenandoahMarkRefsSuperClosure::work<T, STRING_DEDUP>(p);
44+
ShenandoahMarkRefsSuperClosure::work<T>(p);
4545
}
4646

4747
template<class T>

0 commit comments

Comments
 (0)