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

[Thomas Schatzl]

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.

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

> 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