RFR(S): 7069863: G1: SIGSEGV running SPECjbb2011 and -UseBiasedLocking

[John Cuthbertson]

Hi Vladimir,

Apologies. I missed that review comment. I'll do it immediately.

JohnC

On 07/29/11 10:15, Vladimir Kozlov wrote:
> John,
>
> Why you did not add failed_to_reserve_as_requested() to 
> ReservedSpace() as I asked? It is missing there after the code which 
> handle wrong alignment.
>
> Thanks,
> Vladimir
>
> John Cuthbertson wrote:
>> Hi Everyone,
>>
>> A new webrev based upon the feed back from Vladimir can be found at: 
>> http://cr.openjdk.java.net/~johnc/7069863/webrev.1/
>>
>> I'm looking for one more reviewer.
>>
>> Thanks,
>>
>> JohnC
>>
>> On 07/27/11 12:06, Vladimir Kozlov wrote:
>>> g1CollectedHeap.cpp:
>>>
>>>   // When compressed oops are used the preferred heap base is 
>>> calculated
>>>   // by subtracting the requested size from the 32Gb boundary and using
>>>   // the result as the base address for heap reservation. If the 
>>> size is
>>>   // not aligned to HeapRegion::GrainBytes passed into the 
>>> ReservedHeapSpace
>>>   // constructor then the base of the actual reserved heap may end up
>>>   // differing from the requested base address. If this happens then we
>>>   // could end up using a non-optimal compressed oops mode.
>>>
>>>
>>> virtualspace.cpp:
>>>
>>>  213   // prefix_align == suffix_align).
>>>                        ^ prefix_align < suffix_align
>>>
>>> ReservedSpace::ReservedSpace() also misses the call to 
>>> failed_to_reserve_as_requested() after the code which handle wrong 
>>> alignment.
>>>
>>> Add assert into ReservedSpace::initialize()
>>>
>>>   // Assert that if noaccess_prefix is used, it is the same as 
>>> alignment.
>>>   assert(noaccess_prefix == 0 ||
>>>          noaccess_prefix == alignment, "noaccess prefix wrong");
>>>
>>> Why we have next code? Should we do this alignment adjustment at the 
>>> beginning of the method?
>>>
>>> 355   _alignment = MAX2(alignment, (size_t) os::vm_page_size());
>>>
>>> Vladimir
>>>
>>> John Cuthbertson wrote:
>>>> Hi Everyone,
>>>>
>>>> Can I have a couple of volunteers look over these changes - the 
>>>> webrev can be found at: 
>>>> http://cr.openjdk.java.net/~johnc/7069863/webrev.0/
>>>>
>>>> The issue was caused by an implicit null check in generated code 
>>>> not firing. The implicit null check did not trap as a result of a 
>>>> mismatch between the compressed oops mode and the calculated heap 
>>>> base. Also the page below heap base was not being protected. The 
>>>> mismatch between the compressed oops mode and heap base was the 
>>>> result of the G1 heap initialization code passing in a total heap 
>>>> size (G1 heap and perm) that was not a multiple of the alignment 
>>>> passed into the ReservedSpace constructor. Hence the preferred heap 
>>>> base address was not correctly aligned causing the ReservedSpace to 
>>>> be allocated at an address other than the preferred heap base. The 
>>>> page below the heap base was not protected because the G1 heap 
>>>> initialization code was calling the wrong ReservedSpace constructor.
>>>>
>>>> Testing: the failing test case; various small tests with various 
>>>> collectors, large heaps, and +PrintCompressedOopsMode.
>>>>
>>>> Many thanks to Vladimir and Igor for helping to diagnose the issue.
>>>>
>>>> Thanks,
>>>>
>>>> JohnC
>>