[Fwd: Re: RFR (M/L): 8010722 assert: failed: heap size is too big for compressed oops]

[David Holmes]

Hi Thomas,

I think it is bad form to call os::init_large_pages() from inside the 
arguments processing code! I presume you can not move it from 
os::init_2() to os::init()? I'd prefer to see the argument processing 
refactored if we have to add in another os::init hook, than to hide this 
inside the argument processing.

David

On 30/08/2013 12:28 AM, Thomas Schatzl wrote:
> Hi all,
>
>    forwarding the RFR for 8010722 here too as it contains changes on
> parts of the runtime too (argument processing, large page
> initialization), and you might be interested.
>
> Note that the current webrev is
> http://cr.openjdk.java.net/~tschatzl/8010722/webrev.3
>
> Sorry for not thinking about cc'ing here earlier,
>    Thomas
>
> -------- Forwarded Message --------
>> From: Thomas Schatzl <thomas.schatzl at oracle.com>
>> To: hotspot-gc-dev at openjdk.java.net
>> Subject: Re: RFR (M/L): 8010722 assert: failed: heap size is too big
>> for compressed oops
>> Date: Wed, 28 Aug 2013 15:32:46 +0200
>>
>> Hi all,
>>
>> On Tue, 2013-05-14 at 16:37 +0200, Thomas Schatzl wrote:
>>> Hi all,
>>>
>> JIRA:
>>> https://jbs.oracle.com/bugs/browse/JDK-8010722
>>
>>   can I have reviews for the following change?
>>
>>>
>>> In argument processing related to ergonomically determining compressed
>>> oops use, there were a few use-before-set issues leading to crashes that
>>> this fix tries to resolve.
>>>
>>> bugs.sun.com
>>> http://bugs.sun.com/view_bug.do?bug_id=8010722
>>>
>>> JIRA:
>>> https://jbs.oracle.com/bugs/browse/JDK-8010722
>>>
>>> webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8010722/webrev.1/
>>
>> There is a new webrev at
>> http://cr.openjdk.java.net/~tschatzl/8010722/webrev.2 ; this has become
>> necessary after the changes in CR 8007074 (Stefan K's large pages
>> changes) and CR 8003424 from Harold.
>>
>> Particularly the latter changed how the class metadata space is
>> allocated, removing a lot of additional checking and alignment issues.
>>
>> However the core issue still persists, the maximum heap size for
>> compressed oops is calculated wrongly as the NULL page is not properly
>> accounted for.
>>
>> testing:
>> jprt, jprt test cases
>>
>> Thomas
>>
>>> Here's a walkthrough of the changes:
>>>
>>> As mentioned, the cause of this CR is that ergonomics for determining
>>> the maximum java heap size usable for compressed oops uses variables
>>> that are later changed ergonomically.
>>>
>>> It is best to look at the changes beginning from
>>> Arguments::set_ergonomics_flags(): the idea of this change is to avoid
>>> later overflow, so the change tries to conservatively estimate sizes of
>>> the non-java heap parts. The complication is that not even the later
>>> effective alignment of these heap parts has been determined at this
>>> point.
>>>
>>> So the change first calculates the maximum possible heap alignment by
>>> calling set_max_heap_alignment(); this size is influenced by OS page
>>> sizes, so the change needs to initialize large pages by calling
>>> os::large_page_init() in Arguments::parse(), before the call to
>>> set_ergonomics_flags(). The maximum possible alignment is then
>>> calculated by asking the active GC for its maximum alignment, as at this
>>> point the GC has already been determined, the maximum page size, and
>>> other requirements, like alignment for card table size etc.
>>>
>>> Now the code can calculate the conservative estimate for actual maximum
>>> heap for compressed oops used in set_use_compressed_oops(), by
>>> subtracting the conservatively aligned sizes of the other heap parts.
>>> (In Arguments::max_heap_for_compressed_oops()) The result is the maximum
>>> possible heap that can use compressed oops, minus the aligned metaspace
>>> size, minus the aligned null page size.
>>>
>>> There is another circular dependency problem here, the metaspace size itself is later ergonomically sized; the change fixes this problem by anticipating that in Arguments::max_heap_for_compressed_oops(), using the same predicate for determining whether to ergonomically size it or not [this is CR8009778 I think].
>>>
>>> The other changes are straightforward: the os_* changes result from that
>>> large page initialization must be done earlier now; the changes in the
>>> collectors themselves are simply about providing the collector's maximum
>>> alignment. The change in Universe::reserve_heap() contains one assertion that checks whether the conservative estimate for alignment has been conservative enough earlier.
>>>
>>> The test case tests test cases from the CR that work now, and additional
>>> border cases related to ergonomically deciding heap size for compressed
>>> oops.
>>>
>>> One side effect of this change is that the ergonomically determined heap size is slighly smaller now (so that it always fits :).
>>>
>>> Thanks,
>>>    Thomas
>>>
>>>
>>>
>>>
>>>
>>
>>
>
>