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

[David Holmes]

On 5/09/2013 4:54 PM, 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:
>>>>>>> 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.

Ok. There is also the comment before the actual implementations eg:

// this is called _after_ the global arguments have been parsed

but I guess it is okay.

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

It is a grey area because there are some very similar non-ergonomic flag 
processing sections in both parse() and apply_ergonomics().

But lets move on.

Thanks,
David
--------

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