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

[Bengt Rutisson]

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.

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