RFR(S): 8058461: serviceability/dcmd/CodelistTest.java and serviceability/dcmd/CompilerQueueTest.java SIGSEGV

[Vladimir Kozlov]

This looks good.

Thanks,
Vladimir

On 9/18/14 4:52 AM, Nils Eliasson wrote:
> Albert pointed out that there are actually only one compile queue lock share by the queues. That simplified the patch.
>
> Next: http://cr.openjdk.java.net/~neliasso/8058461/webrev.12
>
> Thanks,
> //Nils
>
> On 2014-09-18 12:41, Nils Eliasson wrote:
>> Yes, good catch.
>>
>> Next: http://cr.openjdk.java.net/~neliasso/8058461/webrev.09/
>>
>> //Nils
>>
>> On 2014-09-18 12:21, Albert wrote:
>>> Hi Nils,
>>>
>>>   785 void CompileBroker::print_compile_queues(outputStream* st) {
>>>   786   _c1_compile_queue->print_with_lock(st);
>>>   787   _c2_compile_queue->print_with_lock(st);
>>>   788 }
>>>
>>>
>>> can't it happen that _c1_compile_queue and/or _c2_compile_queue or NULL?
>>>
>>> Best,
>>> Albert
>>>
>>>
>>> On 09/18/2014 10:50 AM, Nils Eliasson wrote:
>>>> Yes. Also good to have the print method lock-free so it can be callled from a debugger without hassle. Adding a
>>>> wrapper that takes the compileQueue lock.
>>>>
>>>> New webrev: http://cr.openjdk.java.net/~neliasso/8058461/webrev.08/
>>>>
>>>> Thanks!
>>>> //Nils
>>>>
>>>> On 2014-09-18 02:59, Vladimir Kozlov wrote:
>>>>> CompileQueue::add() path already has lock acquired.
>>>>> I think you need the lock only in CompileBroker::print_compile_queues().
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 9/17/14 1:54 AM, Nils Eliasson wrote:
>>>>>>
>>>>>> Hi Albert,
>>>>>>
>>>>>> On 2014-09-16 10:45, Albert wrote:
>>>>>>> Hi Nils,
>>>>>>>
>>>>>>> please see comments inline.
>>>>>>>
>>>>>>> On 09/16/2014 10:16 AM, Nils Eliasson wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I would like review of this change that includes three fixes:
>>>>>>>>
>>>>>>>> 1) Let Dcmd Compiler.codelist only print alive-nmethods. We ran into
>>>>>>>> crashes when listing zombies and unloaded too. Alive nmethods
>>>>>>>> includes not-entrants so it still gives a pretty good idea about
>>>>>>>> whats in the code cache and what has been used recently.
>>>>>>>>
>>>>>>> What was the cause of these crashes? Is the reason that
>>>>>>> nmethod->method() is null?
>>>>>>
>>>>>> In one case it crashed when nmethod->method()->constants() was null when
>>>>>> printing the name.
>>>>>>
>>>>>> I see that the segmented code cache push already changed to only
>>>>>> iterating the alive nmethods so I removed that from the patch.
>>>>>>
>>>>>>> Would it make sense to print information about zombie methods that
>>>>>>> don't lead
>>>>>>> to a crash?
>>>>>> Probably not. Not_entrant nmethods may stay around for a while, but once
>>>>>> zombies they are reclaimed pretty fast.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> 2) Take CompileQueue lock when printing queue. It is not enough to be
>>>>>>>> at a safepoint - the compiler threads may still mutate the list
>>>>>>>> causing crashes.
>>>>>>>>
>>>>>>> Are we guaranteed to not run into a deadlock when we acquire the lock?
>>>>>>
>>>>>> Yes. It doesn't use any other lock.
>>>>>>
>>>>>> New webrev: http://cr.openjdk.java.net/~neliasso/8058461/webrev.05/
>>>>>>
>>>>>> Thanks Albert and Christian for your feedback.
>>>>>>
>>>>>> //Nils
>>>>>>
>>>>>>>
>>>>>>> Best,
>>>>>>> Albert
>>>>>>>> 3) Relax the parsing of long hex-numbers in the test of codelist.
>>>>>>>> High addresses (sparc) casues NumberFormatExceptions.
>>>>>>>>
>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8058461
>>>>>>>> webrev: http://cr.openjdk.java.net/~neliasso/8058461/webrev.04/
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>> //Nils
>>>>>>>
>>>>>>
>>>>
>>>
>>
>