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

David Holmes david.holmes at oracle.com
Tue Sep 3 17:59:17 PDT 2013


Hi Thomas,

On 3/09/2013 7:59 PM, Thomas Schatzl wrote:
> Hi all,
>
>    I updated the change for 8010722 per the suggestions of David and
> Stefan and Bengt.
>
> Changes:
> - add a new OS class hook called init_ergo() to be called between
> os::init() and os::init_2().

That in itself was okay but you still call this from inside the 
arguments processing code - which is what I object to.

David

> I preferred this more descriptive name rather than using os::init_2()
> and changing all os::init_2() calls to os_init_3().
> - sorted new #include references
> - naming changes:
> All methods using a conservative maximum heap alignment have the name
> "conservative" in them now. Non-"conservative" methods return an actual
> required alignment.
> - test case refactoring:
>    - use specific WhiteBox method returning the maximum COOP size
> directory instead of parsing some text output
>    - use the HotspotDiagnosticMXBean to retrieve the current
> ClassMetaspaceSize for the test, i.e. the test case that uses different
> class meta space size.
>
> There is need to refactor jtreg support routines across the gc component
> for commonly used functionality. I filed JDK-8024157 for this purpose.
>
> The latest version can be viewed at
> http://cr.openjdk.java.net/~tschatzl/8010722/webrev.4
>
> bugs.sun.com
> http://bugs.sun.com/view_bug.do?bug_id=8010722
>
> JIRA:
> https://jbs.oracle.com/bugs/browse/JDK-8010722
>
> Testing:
> jprt, jtreg
>
> Thanks,
>    Thomas
>
> On Fri, 2013-08-30 at 13:28 +1000, David Holmes wrote:
>> 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
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>
>


More information about the hotspot-gc-dev mailing list