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

[Albert]

Looks good to me (not a reviewer).

Best,
Albert

On 09/18/2014 10:40 PM, Vladimir Kozlov wrote:
> 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
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>