Review request: 8004710: NPG: jmap could throw sun.jvm.hotspot.types.WrongTypeException after PermGen removal

[Stefan Karlsson]

On 2013-01-31 10:09, Bengt Rutisson wrote:
>
> Stefan,
>
> On 1/31/13 9:40 AM, Stefan Karlsson wrote:
>> On 2013-01-30 17:22, Stefan Karlsson wrote:
>>> On 30 jan 2013, at 17:17, Jon Masamitsu <jon.masamitsu at oracle.com> 
>>> wrote:
>>>
>>>> Comment below.
>>>>
>>>> On 1/29/2013 2:01 PM, Stefan Karlsson wrote:
>>>>> On 29 jan 2013, at 22:53, Jon Masamitsu<jon.masamitsu at oracle.com>  
>>>>> wrote:
>>>>>
>>>>>> Stefan,
>>>>>>
>>>>>> Changes look correct.
>>>>> Thanks.
>>>>>
>>>>>> Did you consider calculating the value for endReserve() all
>>>>>> in the VM (similar to the way ReserveForAllocationPrefetch
>>>>>> is calculated during initialization) and passing the value
>>>>>> to the SA the way ReserveForAllocationPrefetch is passed?
>>>>>> If ReserveForAllocationPrefetch is not used anywhere else,
>>>>>> you could substitute it (TLabEndReserve as a possible name)
>>>>>> for ReserveForAllocationPrefetch.
>>>>> No i didn't think about that. I just tried to mimic the VM code in 
>>>>> the SA code. Would you prefer if I changed it to use your 
>>>>> alternative solution?
>>>> Scrap the alternate fix suggestion but I'd suggest changing the name
>>>>
>>>> intArrayHeaderSize
>>>>
>>>> to
>>>>
>>>> minFillerArraySize
>>>>
>>>> What caught my eye about this change is that it was hard
>>>> to understand the use of intArrayHeaderSize unless you knew
>>>> that tlab's had filler arrays inserted in them to make them
>>>> parsable and hotspot saved room at the end of each tlab so
>>>> that a filler object could always be inserted.  If you
>>>> make the name change that give the reader a clue to that.
>>> Sure, I'll update the name. Thanks again for the review.
>> Updated version:
>> http://cr.openjdk.java.net/~stefank/8004710/webrev.01/
>>
>> I need a second review.
>
> Looks good to me.
>
> One minor thing:
>
> ThreadLocalAllocBuffer:: endReserve(). Was it intended to leave the 
> variable intArrayHeaderSize in there? Looks like it was just left from 
> the renaming.

I left it there on purpose. The SA code hides the 
Array.headerSizeInBytes() function, so I figured I could use the public 
Array.baseOffsetInBytes(BasicType.T_INT) instead. But since it's 
actually the header size I'm after (See: 
ThreadLocalAllocBuffer::end_reserve()), I put the result in a variable 
with a name that hinted about that.

For reference: In src/share/vm/memory/threadLocalAllocBuffer.hpp
   // Reserve space at the end of TLAB
   static size_t end_reserve() {
     int reserve_size = typeArrayOopDesc::header_size(T_INT);
     return MAX2(reserve_size, 
VM_Version::reserve_for_allocation_prefetch());
   }

Do you want me to remove the intArrayHeaderSize variable?

StefanK

>
> Bengt
>
>>
>> thanks,
>> StefanK
>>>
>>> StefanK
>>>
>>>> Ship it.
>>>>
>>>> Jon
>>>>
>>>>
>>>>> thanks,
>>>>> StefanK
>>>>>
>>>>>
>>>>>> Jon
>>>>>>
>>>>>> On 01/29/13 02:58, Stefan Karlsson wrote:
>>>>>>> http://cr.openjdk.java.net/~stefank/8004710/webrev/
>>>>>>>
>>>>>>> This bug is in the GC specific code in the SA agent.
>>>>>>>
>>>>>>>  From the bug report:
>>>>>>> There's a bug in ObjectHeap.collectLiveRegions(), which tries to 
>>>>>>> collect the regions of the heap that contain live objects. It 
>>>>>>> removes the [top, end) regions of a TLAB, since we don't have 
>>>>>>> live objects in those regions. Unfortunately, this doesn't 
>>>>>>> consider the small alignment_reserve() part at the end of the 
>>>>>>> TLAB, and we try to decode an object in that unused memory region.
>>>>>>>
>>>>>>> It's not related to the PermGen removal, but the failure mode 
>>>>>>> was probably changed.
>>>>>>>
>>>>>>> thanks,
>>>>>>> StefanK
>>>>>>>
>>
>