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

[Stefan Karlsson]

On 2013-01-31 10:47, Bengt Rutisson wrote:
> 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... :)
http://cr.openjdk.java.net/~stefank/8004710/webrev.02/

I just removed the variable. Now endReserve() looks like this:
   private long endReserve() {
     long minFillerArraySize = Array.baseOffsetInBytes(BasicType.T_INT);
     long reserveForAllocationPrefetch = 
VM.getVM().getReserveForAllocationPrefetch();
     long heapWordSize = VM.getVM().getHeapWordSize();

     return Math.max(minFillerArraySize, reserveForAllocationPrefetch * 
heapWordSize);
   }

I think it's good enough, since it's even more descriptive than the C++ 
code that it tries to mimic.

thanks,
StefanK

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