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

[Joe Provino]

On 11/18/2012 06:03 PM, David Holmes wrote:
> 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.

Okay, I fixed the indentation.

joe

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