Does CollectedHeap::print_on() need Heap_lock?

[Yasumasa Suenaga]

Hi Thomas,

Thank you for replying.

> It is not required. The methods are only ever called at a safepoint.

No, it is called from Attach Listener thread.
I run JDK 9 EA b112 x64 Linux on GDB, and I got call stack as below:

----------------
Breakpoint 2, 0x00007ffff659b970 in HeapInfoDCmd::execute(DCmdSource, Thread*)
     () from /usr/local/jdk-9/lib/amd64/server/libjvm.so
(gdb) bt
#0  0x00007ffff659b970 in HeapInfoDCmd::execute(DCmdSource, Thread*) ()
    from /usr/local/jdk-9/lib/amd64/server/libjvm.so
#1  0x00007ffff65a764c in DCmd::parse_and_execute(DCmdSource, outputStream*, char const*, char, Thread*) () from /usr/local/jdk-9/lib/amd64/server/libjvm.so
#2  0x00007ffff633bf80 in jcmd(AttachOperation*, outputStream*) ()
    from /usr/local/jdk-9/lib/amd64/server/libjvm.so
#3  0x00007ffff633cca9 in attach_listener_thread_entry(JavaThread*, Thread*) ()
    from /usr/local/jdk-9/lib/amd64/server/libjvm.so
#4  0x00007ffff6b6f876 in JavaThread::thread_main_inner() ()
    from /usr/local/jdk-9/lib/amd64/server/libjvm.so
#5  0x00007ffff6b6f982 in JavaThread::run() ()
    from /usr/local/jdk-9/lib/amd64/server/libjvm.so
#6  0x00007ffff69faad2 in java_start(Thread*) ()
    from /usr/local/jdk-9/lib/amd64/server/libjvm.so
#7  0x00007ffff79b060a in start_thread (arg=0x7fffa490c700)
     at pthread_create.c:334
#8  0x00007ffff72d8a4d in clone ()
     at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
----------------

According to you, I guess HeapInfoDCmd should enter safepoint or get Heap_lock.
Is it correct?


Thanks,

Yasumasa


On 2016/04/29 17:54, Thomas Schatzl wrote:
> Hi Yasumasa,
>
>   sorry for the long wait on this...
>
> On Tue, 2016-04-12 at 22:56 +0900, Yasumasa Suenaga wrote:
>> Hi all,
>>
>> I have a question about Heap_lock in Universe::print_*().
>>
>> Universe::print_heap_at_SIGBREAK() gets Heap_lock.
>> However, HeapInfoDCmd::execute() and Universe::print_on() do not get
>> Heap_lock.
>>
>> Should we get Heap_lock when we print heap information?
>
> It is not required. The methods are only ever called at a safepoint.
>
>> I guess that we should get this lock for consistency.
>
> That would be something that could be done, or just adding
> asserts/guarantees about the intended places to call (i.e. either at
> safepoint or heap locked - there should be some method like that
> somewhere actually).
>
>>
>> --------------------
>> diff -r 87215e99d945 src/share/vm/memory/universe.cpp
>> --- a/src/share/vm/memory/universe.cpp  Wed Apr 06 23:42:52 2016
>> +0000
>> +++ b/src/share/vm/memory/universe.cpp  Tue Apr 12 22:23:17 2016
>> +0900
>> @@ -1069,6 +1069,7 @@
>>  }
>>
>>  void Universe::print_on(outputStream* st) {
>> +  MutexLocker hl(Heap_lock);
>>    st->print_cr("Heap");
>>    heap()->print_on(st);
>>  }
>> diff -r 87215e99d945 src/share/vm/services/diagnosticCommand.cpp
>> --- a/src/share/vm/services/diagnosticCommand.cpp       Wed Apr 06
>> 23:42:52 2016 +0000
>> +++ b/src/share/vm/services/diagnosticCommand.cpp       Tue Apr 12
>> 22:23:17 2016 +0900
>> @@ -413,6 +413,7 @@
>>  }
>>
>>  void HeapInfoDCmd::execute(DCmdSource source, TRAPS) {
>> +  MutexLocker hl(Heap_lock);
>>    Universe::heap()->print_on(output());
>>  }
>>
>> --------------------
>>
>> If it is correct, I file it to JBS and send review request.
>>
>
> Thanks,
>   Thomas
>
>