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

[Stefan Karlsson]

On 9/11/13 4:03 PM, Thomas Schatzl wrote:
> Hi,
>
>
> On Tue, 2013-09-10 at 11:36 +0200, Stefan Karlsson wrote:
>> On 09/05/2013 08:54 AM, Thomas Schatzl wrote:
>>> Hi,
>>>
>>> On Thu, 2013-09-05 at 12:00 +1000, David Holmes wrote:
>>>> On 5/09/2013 1:19 AM, Thomas Schatzl wrote:
>>>>> On Wed, 2013-09-04 at 18:22 +1000, David Holmes wrote:
>>>>>> On 4/09/2013 4:27 PM, Thomas Schatzl wrote:
>>>>>>> On Wed, 2013-09-04 at 10:59 +1000, David Holmes wrote:
>>>>>>>> On 3/09/2013 7:59 PM, Thomas Schatzl wrote:
>>>>> New webrev at http://cr.openjdk.java.net/~tschatzl/8010722/webrev.5/
>>>> So you have just split parse() in two at the point where you need to
>>>> inject the os::init_large_pages. That's okay but I don't think the name
>>>> apply_ergonomics accurately reflects everything that happens in that
>>>> second function. Further, given that os::init2 is specifically defined
>>>> as being invoked after arguments have been parsed, it would now be
>>> I changed the comment for init_2 to "Called after command line parsing
>>> and VM ergonomics processing" already.
>>>
>>>> unclear as to whether it could come before Arguments::apply_ergonomics
>>>> (it can't!). In that light I'd prefer see very generic naming eg parse()
>>>> and parse2(), or parse_phase1(), parse_phase2().
>>> Imo almost all code in the apply_ergonomics() method is about
>>> configuring/setting VM flags depending on the environment (i.e. what I
>>> loosely consider ergonomics), so I still think it is appropriate.
>>>
>>> If you really think otherwise I will change that, I have no overly
>>> strong opinion here - but the code does not do any command line parsing
>>> in the apply_ergonomics()/parse_phase2() method so it seems an even
>>> bigger misnomer.
>> I'm not sure I like the way the naming turned out. Arguments::parse()
>> still does "ergonomics" and os::init_ergo() is just misleading. However,
>> if you can't figure out better names I'll not block this.
> There is a new webrev at
> http://cr.openjdk.java.net/~tschatzl/8010722/webrev.6 .
>
> As talked about in the private email, I chose names so that the main
> method calling all these looks as follows:
>
> [...]
> os::init()
> Arguments::parse()
> os::init_before_ergo()
> Arguments::apply_ergo()
> os::init2()
> [...]
>
> We know that the Arguments::parse()/Arguments::apply_ergo() method names
> are not the best, and both methods do some ergonomics, but it should be
> okay for now.
>
>> Here are some style comments:
>>
>> http://cr.openjdk.java.net/~tschatzl/8010722/webrev.5/src/share/vm/gc_implementation/g1/heapRegion.hpp.frames.html
>>
>> Could you add an empty line between 358 and 359.
>>    358   static size_t max_region_size();
> [...]
>>
>> http://cr.openjdk.java.net/~tschatzl/8010722/webrev.5/src/share/vm/gc_implementation/parallelScavenge/parallelScavengeHeap.hpp.frames.html
>>
>> Can you start your comments with capitals? You need to go through the
>> entire patch and look at your comments. One example:
> [...]
>> Why remove the const qualifier? Can you cast a way the constness in the
>> caller instead?
>>    130   static size_t intra_heap_alignment() { return 64 * K *
>> HeapWordSize; }
> The method is static now, and this is required because it is called by
> conservative_max_heap_alignment(); it never depended on any members
> before either.

OK. I missed that.

>
>> http://cr.openjdk.java.net/~tschatzl/8010722/webrev.5/src/share/vm/memory/collectorPolicy.hpp.udiff.html
>>
>> Just a comment. After your changes
>> GenCollectorPolicy::compute_max_alignment() is now a static function
>> that doesn't do any kind of policy work anymore.
> Any suggestions to move that somewhere more fitting?
>
> It was not really doing policy work before either. The rem_set_name()
> method always ever returned GenRemSet::CardTable.

No. I just wanted to bring up that point for future consideration.

thanks,
StefanK

>
>> http://cr.openjdk.java.net/~tschatzl/8010722/webrev.5/src/share/vm/memory/genCollectedHeap.hpp.udiff.html
>>
>> I think this should be named conservative_max_heap_alignment().
>> +  // return the (conservative) maximum heap alignment
>> +  static size_t max_heap_alignment() {
>> +    return Generation::GenGrain;
>> +  }
> Done.
>
>> http://cr.openjdk.java.net/~tschatzl/8010722/webrev.5/src/share/vm/runtime/arguments.cpp.udiff.html
>>
>> I think you should leave the INCLUDE_ALL_GCS block at the end of the
>> include lists, just to be consistent with includes in most of our other
>> files.
>> +#if INCLUDE_ALL_GCS
>> +#include
> Done.
>
>> http://cr.openjdk.java.net/~tschatzl/8010722/webrev.5/src/share/vm/runtime/arguments.hpp.frames.html
>>
>> Excessive amount of spaces in line 437.
>>    437   static size_t conservative_max_heap_alignment()       { return
>> _conservative_max_heap_alignment; }
> Done.
>
>
>> http://cr.openjdk.java.net/~tschatzl/8010722/webrev.5/test/gc/arguments/TestUseCompressedOopsErgoTools.java.html
>>
>> I think it would be easier to understand the matcher logic if it was
>> written as: return m.group(1).equals("true")
>>
>>    165     String match = m.group();
>>    166     return match.substring(match.lastIndexOf(" ") + 1, match.length()).equals("true");
> Done. Thanks!
>
>> Otherwise this looks good as far as I can see. It's a step in the right
>> direction to resolve our circular dependencies in the initialization code.
> I agree.
>
> Thanks for your suggestions,
>    Thomas
>
>