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.

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

Jump to bottom

8274196: Crashes in VM_HeapDumper::work after JDK-8252842 #5681

Closed
wants to merge 6 commits into from
Closed

8274196: Crashes in VM_HeapDumper::work after JDK-8252842 #5681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fca03cb
8274196: Crashes in VM_HeapDumper::work after JDK-8252842
linzang Sep 24, 2021
2ccc4d2
Merge branch 'master' into pd-fix
linzang Sep 26, 2021
91f1c34
un-ProblemList BasicJMapTest.java
linzang Sep 26, 2021
c1658e1
Add missing read barrier
Sep 27, 2021
f6cb212
remove redundant empty line
Sep 27, 2021
db77cda
remove load barrier for JNI local roots
linzang Sep 27, 2021
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
18 changes: 9 additions & 9 deletions src/hotspot/share/services/heapDumper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -748,7 +748,7 @@ class ParDumpWriter : public AbstractDumpWriter {

static void before_work() {
assert(_lock == NULL, "ParDumpWriter lock must be initialized only once");
_lock = new (std::nothrow) PaddedMonitor(Mutex::leaf, "Parallel HProf writer lock", Mutex::_safepoint_check_never);
_lock = new (std::nothrow) PaddedMonitor(Mutex::leaf, "ParallelHProfWriter_lock", Mutex::_safepoint_check_always);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you change these locks to _safepoint_check_always, you have to acquire them without the Mutex::_no_safepoint_check flags so I don't know why you don't get that assert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it may be because this is actually not a JavaThread. So the assert in Mutex::check_no_safepoint_state would pass.
Moreover, I have tried to use PaddedMonitor(Mutex::nosafepoint, "ParallelHProfWriter_lock", Mutex::_safepoint_check_never); here, but the slowdebug would report errors as you mentioned in JDK-8274245.

Copy link
Contributor Author

@linzang linzang Sep 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I agree the flag here and at the place of lock acquiring seems problematic. I will try to see whether I can use Mutex::_safepoint_check_never here and get rid of the assert.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes
void Mutex::check_no_safepoint_state(Thread* thread) {
check_block_state(thread);
assert(!thread->is_active_Java_thread() || _safepoint_check_required != _safepoint_check_always,
"This lock should always have a safepoint check for Java threads: %s",
name());
}

yes, we exclude the check for a non-java thread, which I thought was an odd exclusion last time I looked. I pass the tests in sun/tools/jmap/BasicJMapTest.java so maybe leave it for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, maybe a new issue could be created for tracking this.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

}

static void after_work() {
Expand Down Expand Up @@ -1626,7 +1626,7 @@ class JNIGlobalsDumper : public OopClosure {
};

void JNIGlobalsDumper::do_oop(oop* obj_p) {
oop o = *obj_p;
oop o = NativeAccess<AS_NO_KEEPALIVE>::oop_load(obj_p);

// ignore these
if (o == NULL) return;
Expand Down Expand Up @@ -1814,8 +1814,8 @@ class DumperController : public CHeapObj<mtInternal> {
public:
DumperController(uint number) :
_started(false),
_lock(new (std::nothrow) PaddedMonitor(Mutex::leaf, "Dumper Controller lock",
Mutex::_safepoint_check_never)),
_lock(new (std::nothrow) PaddedMonitor(Mutex::leaf, "DumperController_lock",
Mutex::_safepoint_check_always)),
_dumper_number(number),
_complete_number(0) { }

Expand Down Expand Up @@ -1910,11 +1910,11 @@ class VM_HeapDumper : public VM_GC_Operation, public AbstractGangTask {
_num_writer_threads = 1;
_num_dumper_threads = num_total - _num_writer_threads;
}
// Number of dumper threads that only iterate heap.
uint _heap_only_dumper_threads = _num_dumper_threads - 1 /* VMDumper thread */;
// Prepare parallel writer.
if (_num_dumper_threads > 1) {
ParDumpWriter::before_work();
// Number of dumper threads that only iterate heap.
uint _heap_only_dumper_threads = _num_dumper_threads - 1 /* VMDumper thread */;
_dumper_controller = new (std::nothrow) DumperController(_heap_only_dumper_threads);
_poi = Universe::heap()->parallel_object_iterator(_num_dumper_threads);
}
Expand Down Expand Up @@ -2252,6 +2252,9 @@ void VM_HeapDumper::doit() {
WorkGang* gang = ch->safepoint_workers();

if (gang == NULL) {
// Use serial dump, set dumper threads and writer threads number to 1.
_num_dumper_threads=1;
_num_writer_threads=1;
work(0);
} else {
prepare_parallel_dump(gang->active_workers());
Expand Down Expand Up @@ -2315,7 +2318,6 @@ void VM_HeapDumper::work(uint worker_id) {
// technically not jni roots, but global roots
// for things like preallocated throwable backtraces
Universe::vm_global()->oops_do(&jni_dumper);

// HPROF_GC_ROOT_STICKY_CLASS
// These should be classes in the NULL class loader data, and not all classes
// if !ClassUnloading
Expand Down Expand Up @@ -2353,10 +2355,8 @@ void VM_HeapDumper::work(uint worker_id) {
_dumper_controller->wait_all_dumpers_complete();
// clear internal buffer;
pw.finish_dump_segment(true);

// refresh the global_writer's buffer and position;
writer()->refresh();

} else {
pw.finish_dump_segment(true);
_dumper_controller->dumper_complete();
Expand Down
3 changes: 0 additions & 3 deletions test/hotspot/jtreg/ProblemList-zgc.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,3 @@ serviceability/sa/TestJmapCoreMetaspace.java 8268722,8268636
serviceability/sa/TestJhsdbJstackMixed.java 8248912 generic-all
serviceability/sa/ClhsdbPstack.java#process 8248912 generic-all
serviceability/sa/ClhsdbPstack.java#core 8248912 generic-all

serviceability/dcmd/gc/HeapDumpAllTest.java 8274196 generic-all
serviceability/dcmd/gc/HeapDumpTest.java 8274196 generic-all
5 changes: 0 additions & 5 deletions test/jdk/ProblemList.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -818,11 +818,6 @@ sun/tools/jstat/jstatLineCounts2.sh 8268211 linux-aa
sun/tools/jstat/jstatLineCounts3.sh 8268211 linux-aarch64
sun/tools/jstat/jstatLineCounts4.sh 8268211 linux-aarch64

sun/tools/jmap/BasicJMapTest.java#G1 8274245 generic-all
sun/tools/jmap/BasicJMapTest.java#Parallel 8274245 generic-all
sun/tools/jmap/BasicJMapTest.java#Serial 8274245 generic-all
sun/tools/jmap/BasicJMapTest.java#Z 8274245 generic-all

############################################################################

# jdk_other
Expand Down