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

[Bengt Rutisson]

On 1/31/13 10:37 AM, Stefan Karlsson wrote:
> 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?

Thanks for the explanation. I'm fine with leaving it in. But since I 
didn't get it from just reading the code, maybe this is a case where it 
is better to add a comment than using a variable name? Normally I would 
argue the other way around... :)

Bengt


>
> 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
>>>>>>>>
>>>
>>
>