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

[Thomas Schatzl]

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:
> >>>>> Hi all,
> >>>>>
> >>>>>      I updated the change for 8010722 per the suggestions of David and
> >>>>> Stefan and Bengt.
[...]
> >>>> That in itself was okay but you still call this from inside the
> >>>> arguments processing code - which is what I object to.
> >>>
> >>> Sorry, I misunderstood then. You suggest something like:
> >>>
> >>>     os::init()
> >>>     Arguments::parse()
> >>>     os::init_ergo();
> >>>     Arguments::do_ergonomics()
> >>>     os::init_2();
> >>>
> >>> in Threads::create_vm() where the os:init() methods are called?
> >>
> >> Yes - if that is feasible.
> >
> > Sure. I like the idea.
> >
> > 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.

Thomas