RFR (XS): 6843375: Debuggee VM crashes performing mark-sweep-compact

Jon Masamitsu jon.masamitsu at oracle.com
Mon May 20 17:36:51 UTC 2013


Erik,

Change looks good.

Jon

On 5/14/13 3:54 PM, Jon Masamitsu wrote:
> Refactoring turned out well.
>
> Thanks.
>
> Jon
>
> On 5/14/13 2:46 AM, Erik Helin wrote:
>> Jon,
>>
>> On 05/09/2013 04:49 PM, Jon Masamitsu wrote:
>>>
>>> On 5/9/13 12:55 AM, Erik Helin wrote:
>>>> Hi Jon,
>>>>
>>>> thanks for reviewing!
>>>>
>>>> On 05/08/2013 08:21 PM, Jon Masamitsu wrote:
>>>>> Erik,
>>>>>
>>>>> Change looks good.
>>>>>
>>>>> Would you consider changing the name of _breakpoints
>>>>>
>>>>>> class VM_ChangeBreakpoints : public VM_Operation {
>>>>>> private:
>>>>>>   JvmtiBreakpoints* _breakpoints;
>>>>>
>>>>> to jvmtiExport_breakpoints?
>>>>
>>>> Would _jvmti_breakpoints make more sense? The VM_ChangeBreakpoints
>>>> constructor always gets passed
>>>> JvmtiCurrentBreakpoints::_jvmti_breakpoints as argument.
>>>
>>> I like the second suggestion.
>>>>
>>>> On 05/08/2013 08:21 PM, Jon Masamitsu wrote:
>>>>> I'm looking for a way to alert
>>>>> the reader of the code that this list of breakpoints is part of
>>>>> (owned if you will) by some other variable so that the reader
>>>>> will not think it is part of the responsibility of 
>>>>> VM_ChangeBreakpoints
>>>>> to update it (as it was erroneously done before you fix).
>>>>> Does that make sense?  Is the name change enough?  I was
>>>>> trying to come up with some refactoring of the code to make
>>>>> it more obvious that _breakpoints should not be processes
>>>>> by VM_ChangeBreakpoints but this is the best I could do.
>>>>
>>>> Another suggestion would be to remove the JvmtiBreakpoints* arguments
>>>> to VM_ChangeBreakpoints and instead directly call
>>>> JvmtiCurrentBreakpoints::get_jvmti_breakpoints to show the reader of
>>>> the code that VM_ChangeBreakpoints always make use of
>>>> JvmtiCurrentBreakpoints::_jvmti_breakpoints.
>>>>
>>>> That way, one would (hopefully) see the JvmtiExport::oops_do calls
>>>> JvmtiCurrentBreakpoints::oops_do and realize that it is not
>>>> VM_ChangeBreakpoints responsibility to call oops_do.
>>>>
>>>> What do you think?
>>>
>>> I like this idea.  I was hoping for something like this but the
>>> idea wouldn't crystallize in my brain.
>>
>> I've uploaded a new webrev at:
>> http://cr.openjdk.java.net/~ehelin/6843375/webrev.01/
>>
>> Sorry for the noise in jvmtiImpl.hpp, but I had to move the 
>> VM_ChangeBreakpoints class below JvmtiCurrentBreakpoints since 
>> VM_ChangeBreakpoints now uses JvmtiCurrentBreakpoints.
>>
>> I also removed the method JvmtiBreakpoints::clearall since it is not 
>> being used anywhere.
>>
>> How do you think the refactoring turned out?
>>
>> Thanks,
>> Erik
>>
>>> Thanks.
>>>
>>> Jon
>>>>
>>>> Thanks,
>>>> Erik
>>>>
>>>>> This is a throw away suggestion if you like.
>>>>>
>>>>> Jon
>>>>>
>>>>>
>>>>>
>>>>> On 5/8/2013 8:51 AM, Erik Helin wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> this change fixes a problem when a VM_ChangeBreakpoint operation is
>>>>>> enqueued on the VMOperationQueue while at the same time as a GC is
>>>>>> running. The GC will process the strong roots and will call both
>>>>>> VMOperationQueue::oops_do (via Threads::oops_do) and
>>>>>> JvmtiExport::oops_do.
>>>>>>
>>>>>> VMOperationQueue::oops_do will call VM_ChangeBreakpoint::oops_do 
>>>>>> which
>>>>>> calls JvmtiBreakpoints::oops_do. The problem is that
>>>>>> JvmtiExport::oops_do calls JvmtiCurrentBreakpoints::oops_do which
>>>>>> points to the same JvmtiBreakpoints as the one VM_ChangeBreakpoint
>>>>>> just called oops_do on.
>>>>>>
>>>>>> This change remove the code VM_ChangeBreakpoint that calls
>>>>>> JvmtiBreakpoints::oops_do. All the GCs will visit the breakpoints 
>>>>>> via
>>>>>> JvmtiExport.
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~ehelin/6843375/webrev/
>>>>>>
>>>>>> Testing:
>>>>>> JPRT
>>>>>>
>>>>>> Bug:
>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6843375
>>>>>>
>>>>>> Thanks,
>>>>>> Erik
>>>>>
>>>>
>>>
>>
>




More information about the hotspot-gc-dev mailing list