Review request: JDK-8003539 -- Minimal VM. VM doesn't react to -Dcom.sun.management and -XX:+ManagementServer

[David Holmes]

On 19/11/2012 8:36 AM, Joe Provino wrote:
> On 11/18/2012 5:08 PM, David Holmes wrote:
>> Hi Joe,
>>
>> I think we should have utilized UNSUPPORTED_OPTION for those flags
>> that have to be disabled and will generate a warning (eg all the GC
>> warnings) - even if we have to augment the macro somewhat.
>
> That sounds reasonable. Should I do that now or just make minimal changes?

Just minimal for now. Perhaps take this up with the UseG1GC CR instead.

>> Then we could have a second macro for the vm_exit_on_initialization
>> case (though the distinction between a fatal error and a warning seems
>> to be quite arbitrary :( ).
>
> Yeah, it's not entirely clear when we warn or exit. I do recall a
> discussion about how we should warn if
> the only impact is potentially performance and exit if something is
> likely to fail. Customers may have scripts
> that they'd like to continue to work as long as missing functionality
> won't prevent them from working. Maybe we should revisit this.

No that is the right distinction - I was just failing to make it. :)

>> Otherwise this change seems functionally correct, but the indentation
>> appears wrong for the -Dcom.sun.management change.
>
> Are you referring to line 2440? I looked at other calls to
> vm_exit_during_initialization and sometimes
> it's indented two spaces, other times four spaces. I agree they should
> all be done the same way.

Line 2439 - the call to vm_exit_... is inside the if statement so needs 
indenting as per line 2437.

> Is 2 spaces the standard for something like this?

Hotspot code indent is 2 spaces. How long lines get subsequently 
indented (ie 2440) does seem somewhat arbitrary.

Thanks,
David

> thanks for the review.
>
> joe
>
>>
>> David
>>
>> On 19/11/2012 7:31 AM, Joe Provino wrote:
>>>
>>> How does this look?
>>> http://cr.openjdk.java.net/~jprovino/8003539/webrev.01/
>>>
>>> On 11/16/2012 4:13 PM, BILL PITTORE wrote:
>>>> Where you had it is the right place, you just need to #if it better.
>>>> The -XX: options get process by the code just after line 2788 by
>>>> process_argument(). That deals with all the globals defined in
>>>> globals.hpp so it's not where you can special case one flag.
>>>>
>>>> bill
>>>>
>>>>
>>>>
>>>> On 11/16/2012 3:23 PM, Joe Provino wrote:
>>>>> Dmitri, wait, I think I see what you mean. If INCLUDE_MANAGEMENT is
>>>>> set to true, then there's no code generated.
>>>>>
>>>>> I think the code that's there will work but perhaps there's a better
>>>>> place to check for this option when
>>>>> INCLUDE_MANAGEMENT is false.
>>>>>
>>>>> I seem to recall there's a place were -XX: arguments are parsed But I
>>>>> don't see a separate method to do that.
>>>>> I must be looking in the wrong place.
>>>>>
>>>>> joe
>>>>>
>>>>> On 11/16/2012 3:14 PM, Dmitry Samersoff wrote:
>>>>>> Joe,
>>>>>>
>>>>>> After you fix at 2788 regular hotspot start accepting do-nothing
>>>>>> option
>>>>>>
>>>>>> -XX:+ManagementServer
>>>>>>
>>>>>> Is it intentional?
>>>>>>
>>>>>> -Dmitry
>>>>>>
>>>>>>
>>>>>> On 2012-11-16 23:24, Joe Provino wrote:
>>>>>>> This is a small change to one file -- arguments.cpp -- to print an
>>>>>>> error
>>>>>>> message
>>>>>>> and exit if INCLUDE_MANAGEMENT is false.
>>>>>>>
>>>>>>> Webrev is here:
>>>>>>> http://cr.openjdk.java.net/~jprovino/8003539/webrev.00/
>>>>>>>
>>>>>>> thanks.
>>>>>>>
>>>>>>> joe
>>>>>>
>>>>
>>>>