RFR: 8079301: Some command line options not settable via JAVA_TOOL_OPTIONS

Dmitry Dmitriev dmitry.dmitriev at oracle.com
Thu Jul 16 17:27:31 UTC 2015


Jeremy, thank you! Looks good to me, but I am not a reviewer.

Dmitry

On 16.07.2015 20:20, Jeremy Manson wrote:
> Thanks for the thorough review.
>
> http://cr.openjdk.java.net/~jmanson/8079301/webrev.06/ 
> <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.06/>
>
> As for other jint->int opportunities: the only ones I found were in 
> loop bounds, and I decided it didn't improve the code to change those.
>
> Jeremy
>
> On Thu, Jul 16, 2015 at 4:06 AM, Dmitry Dmitriev 
> <dmitry.dmitriev at oracle.com <mailto:dmitry.dmitriev at oracle.com>> wrote:
>
>     Hello Jeremy,
>
>     Thank you for doing this! Looks good, just few comments:
>     1) Latest code still not check return value of
>     'match_special_option_and_act' new function on lines 3702-3704.
>     I repeat comment here from one of the e-mail below just in case if
>     you missed it: In one case it can return error(JNI_ERR) when
>     "-XX:NativeMemoryTracking" is specified and INCLUDE_NMT is not
>     defined. Thus, I think that check for return value should be added.
>     2) There are some mix with int/jint variables and return values. I
>     think that all these should be converted to jint.
>     Here what I see:
>       -change return value of new 'set_args' method on line 3415 to
>     'jint'. 'set_args' method called only once(on line 3515) and it
>     return value is set to variable 'status' with 'jint' type.
>       -change return value of new 'match_special_option_and_act'
>     function on line 3602 to 'jint'
>       -change type of 'code' variable on line 3691 to 'jint'. In some
>     cases 'code' can be returned from Arguments::parse function, but
>     return value of Arguments::parse function is 'jint'.
>
>     Thank you,
>     Dmitry
>
>     On 16.07.2015 3:27, Jeremy Manson wrote:
>>     New rev:
>>
>>     http://cr.openjdk.java.net/~jmanson/8079301/webrev.05/
>>     <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.05/>
>>
>>     Thanks for all the attention!
>>
>>     On Wed, Jul 15, 2015 at 3:39 PM, Jeremy Manson
>>     <jeremymanson at google.com <mailto:jeremymanson at google.com>> wrote:
>>
>>         Sorry about the delay.  We were finally able to get JDK8 to
>>         the point where we could use it without our services falling
>>         apart, so we rolled it forward at Google. I've spent a lot of
>>         the last week putting out fires.  Also, house renovations. :/
>>
>>         I think that it is long past time for Hotspot to start using
>>         the RAII classes everyone else in the world adopted years
>>         ago, and will be happy to implement one for this case.  It
>>         was pretty hard for me to avoid the temptation when I was
>>         writing this code in the first place, but the general lack of
>>         them throughout the codebase made me think that they were
>>         unpopular.
>>
>>         Some crazy day we might even consider rolling forward to the
>>         21st century and starting to use unique_ptr in the source base.
>>
>>         I'll ping the thread when there's a new rev.
>>
>>         Jeremy
>>
>>         On Tue, Jul 14, 2015 at 1:26 PM, Dmitry Dmitriev
>>         <dmitry.dmitriev at oracle.com
>>         <mailto:dmitry.dmitriev at oracle.com>> wrote:
>>
>>             Hello Coleen,
>>
>>             Yes, I like your approach. It seems more accurate. Thus,
>>             desctructor can be something like that:
>>             ~EnvInitArgs() {
>>                if( _args.options != NULL ) {
>>                   for (int i = 0; i < _args.nOptions; i++) {
>>             os::free(_args.options[i].optionString);
>>                   }
>>             FREE_C_HEAP_ARRAY(JavaVMOption, _args.options);
>>                }
>>             }
>>
>>             Jeremy, what you think about that?
>>
>>             Thank you,
>>             Dmitry
>>
>>
>>             On 14.07.2015 22:46, Coleen Phillimore wrote:
>>>
>>>
>>>             On 7/12/15 10:27 AM, Dmitry Dmitriev wrote:
>>>>             Hello Coleen,
>>>>
>>>>             I think that adding destructor not help with this,
>>>>             since all this code with free_memory_for_vm_init_args
>>>>             is a special workaround for processing options in env
>>>>             variables and not applied to the options from command
>>>>             line which passed in 'args' to the Arguments::parse
>>>>             function.
>>>
>>>             I see - JavaVMInitArgs is a C struct in jni.h
>>>
>>>             If you wrap JavaVMInitArgs for the options for
>>>             environment variables in a scoped object that calls the
>>>             destructor to clean up JavaVMInitArgs, this would
>>>             eliminate all these extra calls that you have to make to
>>>             free_memory_for_vm_init_args:
>>>
>>>             class EnvInitArgs : public StackObj {
>>>               JavaVMInitArgs _args;
>>>              public:
>>>               ~EnvInitArgs() {
>>>                     // Code to conditionally delete option memory
>>>             that you have in free_memory_for_vm_init_args
>>>               }
>>>               JavaVMInitArgs* init_args() const { return &_args; }
>>>             }
>>>
>>>             You can put this in the constructor:
>>>
>>>             + vm_args->version = JNI_VERSION_1_2;
>>>             + vm_args->options = NULL;
>>>             + vm_args->nOptions = 0;
>>>             + vm_args->ignoreUnrecognized = false;
>>>             I think this would be neater than having to check for
>>>             JNI_OK and call free everywhere.
>>>
>>>             Coleen
>>>
>>>>
>>>>             About match_special_option_and_act. Probably it can be
>>>>             processed in the following way(raw idea):
>>>>             code =
>>>>             match_special_option_and_act(&java_tool_options_args,
>>>>             &flags_file);
>>>>             if (code == JNI_OK) {
>>>>                // call next
>>>>                code = match_special_option_and_act(args, &flags_file);
>>>>             }
>>>>
>>>>             if (code == JNI_OK) {
>>>>                // call next
>>>>                code =
>>>>             match_special_option_and_act(&java_options_args,
>>>>             &flags_file);
>>>>             }
>>>>
>>>>             if (code != JNI_OK) {
>>>>             free_memory_for_vm_init_args(&java_tool_options_args);
>>>>             free_memory_for_vm_init_args(&java_options_args);
>>>>                return code;
>>>>             }
>>>>
>>>>             Thanks,
>>>>             Dmitry
>>>>
>>>>             On 11.07.2015 0:16, Coleen Phillimore wrote:
>>>>>
>>>>>             Hi, I have a couple of suggestions.
>>>>>
>>>>>             Can you add a destructor to JavaVMInitArgs which will
>>>>>             delete the options things if they exist?   Then you
>>>>>             won't need these free_memory_for_vm_init_args
>>>>>             functions.   I think this would work.
>>>>>
>>>>>             Secondly, I was also going to comment about
>>>>>             match_special_option_and_act not using it's return
>>>>>             value.   The destructor code would make this less
>>>>>             cumbersome.  I like the refactoring though.
>>>>>
>>>>>             Thanks,
>>>>>             Coleen
>>>>>
>>>>>             On 7/10/15 2:40 PM, Dmitry Dmitriev wrote:
>>>>>>             Jeremy, thank you for fixing that! Please, see my
>>>>>>             comments inline for 2 item and one more comment.
>>>>>>
>>>>>>             On 10.07.2015 20:15, Jeremy Manson wrote:
>>>>>>>             Thanks for the detailed look, Dmitry.  I have taken
>>>>>>>             your advice, with a couple of exceptions:
>>>>>>>
>>>>>>>             1) I named the method free_memory_for_vm_init_args
>>>>>>>             instead of free_memory_for_environment_variables.
>>>>>>>             2) I did not free the memory in 3661-3663 or in
>>>>>>>             3679-3681, because those are error paths where the
>>>>>>>             memory may not have been allocated in the first
>>>>>>>             place.  So freeing it might lead to a crash.
>>>>>>             Yes, you are right. But I think we can guard from
>>>>>>             this by adding check for args->options in new
>>>>>>             free_memory_for_vm_init_args function and free memory
>>>>>>             in all execution paths. If args->options is not NULL,
>>>>>>             then memory is allocated and must be freed:
>>>>>>
>>>>>>             3485 static void
>>>>>>             free_memory_for_vm_init_args(JavaVMInitArgs* args) {
>>>>>>             3486   if( args->options != NULL ) {
>>>>>>             3487 for (int i = 0; i < args->nOptions; i++) {
>>>>>>             3488 os::free(args->options[i].optionString);
>>>>>>             3489 }
>>>>>>             3490 FREE_C_HEAP_ARRAY(JavaVMOption, args->options);
>>>>>>             3491   }
>>>>>>             3492 }
>>>>>>
>>>>>>             As I see you add free memory for error paths at
>>>>>>             lines(webrev.04) 3668-3671 and 3687-3691. Thus, with
>>>>>>             that guard you can add two calls to
>>>>>>             free_memory_for_vm_init_args function before "return
>>>>>>             JNI_EINVAL;" on line 3696:
>>>>>>
>>>>>>             3693 #ifdef ASSERT
>>>>>>             3694     // Parse default .hotspotrc settings file
>>>>>>             3695     if (!process_settings_file(".hotspotrc",
>>>>>>             false, args->ignoreUnrecognized)) {
>>>>>>             3696       return JNI_EINVAL;
>>>>>>             3697     }
>>>>>>             3698 #else
>>>>>>
>>>>>>
>>>>>>             And one more comment. Unfortunately I oversight this
>>>>>>             before... You not check return value from
>>>>>>             'match_special_option_and_act' new function on lines
>>>>>>             3673-3675, but in one case it can return
>>>>>>             error(JNI_ERR) when "-XX:NativeMemoryTracking" is
>>>>>>             specified and INCLUDE_NMT is not defined. Thus, I
>>>>>>             think that check for return value should be
>>>>>>             added(with freeing memory for env variables in case
>>>>>>             of error).
>>>>>>
>>>>>>             Thank you,
>>>>>>             Dmitry
>>>>>>
>>>>>>>
>>>>>>>             New Rev:
>>>>>>>
>>>>>>>             http://cr.openjdk.java.net/~jmanson/8079301/webrev.04/
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.04/>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.04/>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.04/>
>>>>>>>
>>>>>>>
>>>>>>>             Jeremy
>>>>>>>
>>>>>>>             On Fri, Jul 10, 2015 at 5:52 AM, Dmitry Dmitriev
>>>>>>>             <dmitry.dmitriev at oracle.com
>>>>>>>             <mailto:dmitry.dmitriev at oracle.com>
>>>>>>>             <mailto:dmitry.dmitriev at oracle.com>
>>>>>>>             <mailto:dmitry.dmitriev at oracle.com>> wrote:
>>>>>>>
>>>>>>>                 Jeremy,
>>>>>>>
>>>>>>>                 Two additional comments which I didn't catch
>>>>>>>             earlier...
>>>>>>>
>>>>>>>                 1) Order of processing command line options is
>>>>>>>             following(Arguments::parse_vm_init_args function):
>>>>>>>             options from
>>>>>>>             JAVA_TOOL_OPTIONS environment variable, then options
>>>>>>>             from command
>>>>>>>                 line and then options from _JAVA_OPTIONS
>>>>>>>             environment variable. And
>>>>>>>                 last argument wins!
>>>>>>>
>>>>>>>                 You not change this order in
>>>>>>>             Arguments::parse_vm_init_args
>>>>>>>                 function, but it seems that order in several
>>>>>>>             other places is changed.
>>>>>>>
>>>>>>>                 Lines 3665-3667:
>>>>>>>
>>>>>>>                 3665 match_special_option_and_act(args,
>>>>>>>             &flags_file);
>>>>>>>                 3666
>>>>>>>             match_special_option_and_act(&java_tool_options_args,
>>>>>>>             &flags_file);
>>>>>>>                 3667
>>>>>>>             match_special_option_and_act(&java_options_args,
>>>>>>>             &flags_file);
>>>>>>>
>>>>>>>                 Here we see that order is following: options
>>>>>>>             from command line,
>>>>>>>                 then options from JAVA_TOOL_OPTIONS environment
>>>>>>>             variable and then
>>>>>>>                 options from _JAVA_OPTIONS environment variable.
>>>>>>>             Thus, in this
>>>>>>>                 case "-XX:+IgnoreUnrecognizedVMOptions" option in
>>>>>>>             JAVA_TOOL_OPTIONS environment variable will have
>>>>>>>             bigger priority
>>>>>>>                 than in command line(last argument wins), but
>>>>>>>             this is differ from
>>>>>>>                 processing options in
>>>>>>>             Arguments::parse_vm_init_args function.
>>>>>>>                 Thus, I think that you need to swap 3665 and
>>>>>>>             3666 lines:
>>>>>>>
>>>>>>>                 3665
>>>>>>>             match_special_option_and_act(&java_tool_options_args,
>>>>>>>             &flags_file);
>>>>>>>                 3666 match_special_option_and_act(args,
>>>>>>>             &flags_file);
>>>>>>>                 3667
>>>>>>>             match_special_option_and_act(&java_options_args,
>>>>>>>             &flags_file);
>>>>>>>
>>>>>>>                 Similar situation on lines 3696-3700:
>>>>>>>
>>>>>>>                 3696   if (PrintVMOptions) {
>>>>>>>                 3697 print_options(args);
>>>>>>>                 3698 print_options(&java_tool_options_args);
>>>>>>>                 3699 print_options(&java_options_args);
>>>>>>>                 3700   }
>>>>>>>
>>>>>>>                 Command line options are printed before options
>>>>>>>             from
>>>>>>>             JAVA_TOOL_OPTIONS environment variable. Thus, I
>>>>>>>             think that you
>>>>>>>                 need to swap 3697 and 3698 lines:
>>>>>>>
>>>>>>>                 3696   if (PrintVMOptions) {
>>>>>>>                 3697 print_options(&java_tool_options_args);
>>>>>>>                 3698 print_options(args);
>>>>>>>                 3699 print_options(&java_options_args);
>>>>>>>                 3700   }
>>>>>>>
>>>>>>>
>>>>>>>                 2) One more thing about error processing :)
>>>>>>>                 I think it will be great to free allocated
>>>>>>>             memory for environment
>>>>>>>                 variables when error occured on lines 3661-3663,
>>>>>>>             3679-3681 and
>>>>>>>                 3685-3687.
>>>>>>>                 My suggestion is to add some static function
>>>>>>>             which free memory for
>>>>>>>                 env variables, something like that:
>>>>>>>                 static void
>>>>>>>             free_memory_for_environment_variables( JavaVMInitArgs
>>>>>>>             *options_args ) {
>>>>>>>                    for (int i = 0; i < options_args->nOptions;
>>>>>>>             i++) {
>>>>>>>             os::free(options_args->options[i].optionString);
>>>>>>>                    }
>>>>>>>             FREE_C_HEAP_ARRAY(JavaVMOption, options_args->options);
>>>>>>>                 }
>>>>>>>
>>>>>>>                 And change lines 3661-3663 to following:
>>>>>>>
>>>>>>>                 3661 if (code != JNI_OK) {
>>>>>>>                 3662
>>>>>>>             free_memory_for_environment_variables(&java_tool_options_args);
>>>>>>>
>>>>>>>                 3663 return code;
>>>>>>>                 3664 }
>>>>>>>
>>>>>>>                 Lines 3679-3681 to following:
>>>>>>>
>>>>>>>                 3679     if (!process_settings_file(flags_file,
>>>>>>>             true,
>>>>>>>             args->ignoreUnrecognized)) {
>>>>>>>                 3680
>>>>>>>             free_memory_for_environment_variables(&java_tool_options_args);
>>>>>>>
>>>>>>>                 3681
>>>>>>>             free_memory_for_environment_variables(&java_options_args);
>>>>>>>
>>>>>>>                 3682 return JNI_EINVAL;
>>>>>>>                 3683     }
>>>>>>>
>>>>>>>                 Lines 3685-3687 to following:
>>>>>>>
>>>>>>>                 3685     if
>>>>>>>             (!process_settings_file(".hotspotrc", false,
>>>>>>>             args->ignoreUnrecognized)) {
>>>>>>>                 3680
>>>>>>>             free_memory_for_environment_variables(&java_tool_options_args);
>>>>>>>
>>>>>>>                 3681
>>>>>>>             free_memory_for_environment_variables(&java_options_args);
>>>>>>>
>>>>>>>                 3686 return JNI_EINVAL;
>>>>>>>                 3687     }
>>>>>>>
>>>>>>>                 And lines 3706-3715 to following:
>>>>>>>
>>>>>>>                 3706 // Done with the envvars
>>>>>>>                 3707
>>>>>>>             free_memory_for_environment_variables(&java_tool_options_args);
>>>>>>>
>>>>>>>                 3708
>>>>>>>             free_memory_for_environment_variables(&java_options_args);
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>                 Thank you,
>>>>>>>                 Dmitry
>>>>>>>
>>>>>>>
>>>>>>>                 On 10.07.2015 14:36, Dmitry Dmitriev wrote:
>>>>>>>
>>>>>>>                     Hello Jeremy,
>>>>>>>
>>>>>>>                     Thank you for fixing that! But in this case
>>>>>>>             we still have
>>>>>>>                     minor issue when quotes processing
>>>>>>>             failed(lines 3438-3444).
>>>>>>>                     Here is my suggestion: leave 'while'
>>>>>>>             cycle(lines 3423-3462) as
>>>>>>>                     I was in your previous webrev(without
>>>>>>>             'start' and etc.) and
>>>>>>>                     call strdup when copy 'options' to
>>>>>>>             'options_arr' on lines
>>>>>>>             3472-3474, i.e. make lines 3472-3474 like this:
>>>>>>>
>>>>>>>                     3472 for (int i = 0; i < options->length();
>>>>>>>             i++) {
>>>>>>>                     3473 options_arr[i] = options->at(i);
>>>>>>>                     3474 options_arr[i].optionString =
>>>>>>>             os::strdup(options_arr[i].optionString);
>>>>>>>                     3475   }
>>>>>>>
>>>>>>>                     What you think about that?
>>>>>>>
>>>>>>>                     Regards,
>>>>>>>                     Dmitry
>>>>>>>
>>>>>>>                     On 10.07.2015 3:23, Jeremy Manson wrote:
>>>>>>>
>>>>>>>                         Hey Dmitry,
>>>>>>>
>>>>>>>             Thanks for looking at it. The leak was deliberate
>>>>>>>             (under
>>>>>>>                         the theory that the command line flags
>>>>>>>             were leaked, too),
>>>>>>>                         but it was silly that I didn't fix it.
>>>>>>>
>>>>>>>             Anyway, fixed. See:
>>>>>>>
>>>>>>>             http://cr.openjdk.java.net/~jmanson/8079301/webrev.03 <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.03>
>>>>>>>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.03>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.03>
>>>>>>>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.03>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.03>
>>>>>>>
>>>>>>>
>>>>>>>             Jeremy
>>>>>>>
>>>>>>>                         On Thu, Jul 9, 2015 at 2:52 PM, Dmitry
>>>>>>>             Dmitriev
>>>>>>>                         <dmitry.dmitriev at oracle.com
>>>>>>>             <mailto:dmitry.dmitriev at oracle.com>
>>>>>>>             <mailto:dmitry.dmitriev at oracle.com>
>>>>>>>             <mailto:dmitry.dmitriev at oracle.com>
>>>>>>>                         <mailto:dmitry.dmitriev at oracle.com
>>>>>>>             <mailto:dmitry.dmitriev at oracle.com>
>>>>>>>             <mailto:dmitry.dmitriev at oracle.com>>> wrote:
>>>>>>>
>>>>>>>             Hello Jeremy,
>>>>>>>
>>>>>>>             Correct me if I am wrong, but I see small memory leak
>>>>>>>                         in your
>>>>>>>             implementation.
>>>>>>>             In Arguments::parse_options_environment_variable
>>>>>>>             function memory
>>>>>>>             for 'buffer' allocated by strdup function on line
>>>>>>>             3415, but now it
>>>>>>>             not freed in this function because data from this
>>>>>>>             buffer is used
>>>>>>>             later. On the other hand when we are done with env
>>>>>>>             variables, we
>>>>>>>             free only options array(lines 3703-3705) and not free
>>>>>>>             previously
>>>>>>>             allocated memory for 'buffer' because we lost track
>>>>>>>                         for it.
>>>>>>>
>>>>>>>             Thanks,
>>>>>>>             Dmitry
>>>>>>>
>>>>>>>
>>>>>>>             On 09.07.2015 21:20, Jeremy Manson wrote:
>>>>>>>
>>>>>>>             Thanks for looking at this, Coleen.
>>>>>>>
>>>>>>>             I merged this with the latest version of hs-rt,
>>>>>>>                         but Ron is or
>>>>>>>             I am going to
>>>>>>>             have to do some merging for 8061999.  The changes
>>>>>>>                         are relatively
>>>>>>>             orthogonal, but they touch a couple of the same
>>>>>>>             places. How
>>>>>>>             far away is
>>>>>>>             Ron's checkin?
>>>>>>>
>>>>>>>             Updated webrev:
>>>>>>>
>>>>>>>             http://cr.openjdk.java.net/~jmanson/8079301/webrev.01/
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.01/>
>>>>>>>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.01/>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.01/>
>>>>>>>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.01/>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.01/>
>>>>>>>
>>>>>>>
>>>>>>>             Updated test:
>>>>>>>
>>>>>>>             http://cr.openjdk.java.net/~jmanson/8079301t/webrev.01/
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.01/>
>>>>>>>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.01/>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.01/>
>>>>>>>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.01/>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.01/>
>>>>>>>
>>>>>>>
>>>>>>>             (Mild complaining on my part: the conflicts
>>>>>>>             wouldn't have been
>>>>>>>             an issue if
>>>>>>>             someone had looked at this two months ago, when I
>>>>>>>             first
>>>>>>>             asked.  I suspect
>>>>>>>             Ron hadn't even started his change at that point.
>>>>>>>             And if external
>>>>>>>             developers were able to submit changes to Hotspot,
>>>>>>>                         we wouldn't
>>>>>>>             have had to
>>>>>>>             bother Oracle engineers at all.  Neither of these
>>>>>>>                         is your
>>>>>>>             fault or your
>>>>>>>             problem, of course - I'm grateful you were able to
>>>>>>>                         take a look.)
>>>>>>>
>>>>>>>             Jeremy
>>>>>>>
>>>>>>>             On Wed, Jul 8, 2015 at 6:19 AM, Coleen Phillimore <
>>>>>>>             coleen.phillimore at oracle.com
>>>>>>>             <mailto:coleen.phillimore at oracle.com>
>>>>>>>             <mailto:coleen.phillimore at oracle.com>
>>>>>>>             <mailto:coleen.phillimore at oracle.com>
>>>>>>>             <mailto:coleen.phillimore at oracle.com
>>>>>>>             <mailto:coleen.phillimore at oracle.com>
>>>>>>>             <mailto:coleen.phillimore at oracle.com>>> wrote:
>>>>>>>
>>>>>>>             I'll sponsor it but can you repost the RFR?
>>>>>>>
>>>>>>>             There has been a fair bit of work to command line
>>>>>>>             processing lately so I'd
>>>>>>>             like to have the others look at it too. Have
>>>>>>>                         you merged
>>>>>>>             this up with the
>>>>>>>             latest version of hs-rt repository and were there
>>>>>>>             conflicts? Also, have
>>>>>>>             you looked at the RFR:
>>>>>>>
>>>>>>>             "Open code review for 8061999 Enhance VM
>>>>>>>             option parsing to
>>>>>>>             allow options
>>>>>>>             to be specified"
>>>>>>>
>>>>>>>             and are there conflicts with this?
>>>>>>>
>>>>>>>             The hotspot change looks good to me.
>>>>>>>
>>>>>>>
>>>>>>>             http://cr.openjdk.java.net/~jmanson/8079301t/webrev.00/test/com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java.udiff.html
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.00/test/com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java.udiff.html>
>>>>>>>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.00/test/com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java.udiff.html>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.00/test/com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java.udiff.html>
>>>>>>>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.00/test/com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java.udiff.html>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.00/test/com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java.udiff.html>
>>>>>>>
>>>>>>>
>>>>>>>             "UseCerealGC" LOL
>>>>>>>             Can you use a different option than
>>>>>>>             PrintOopAddress in
>>>>>>>             this test?  I don't
>>>>>>>             know why this command line option exists or
>>>>>>>             would be
>>>>>>>             useful for, and seems
>>>>>>>             to be a good candidate for removal. If you
>>>>>>>                         need a valid
>>>>>>>             argument, that is
>>>>>>>             unlikely to go away, TraceExceptions would be
>>>>>>>             good.
>>>>>>>
>>>>>>>             Thanks,
>>>>>>>             Coleen
>>>>>>>
>>>>>>>
>>>>>>>             On 7/7/15 8:00 PM, Jeremy Manson wrote:
>>>>>>>
>>>>>>>             No love?  Is there a code owner here I
>>>>>>>             should pester
>>>>>>>             more directly?
>>>>>>>
>>>>>>>             Jeremy
>>>>>>>
>>>>>>>             On Fri, Jun 26, 2015 at 3:00 PM, Martin
>>>>>>>             Buchholz
>>>>>>>             <martinrb at google.com <mailto:martinrb at google.com>
>>>>>>>             <mailto:martinrb at google.com>
>>>>>>>             <mailto:martinrb at google.com>
>>>>>>>             <mailto:martinrb at google.com
>>>>>>>             <mailto:martinrb at google.com>
>>>>>>>             <mailto:martinrb at google.com>>>
>>>>>>>             wrote:
>>>>>>>
>>>>>>>             As usual with Google patches, they are
>>>>>>>                         in use
>>>>>>>             locally.  This one has been
>>>>>>>
>>>>>>>             stable for a while without complaints.
>>>>>>>                         Please
>>>>>>>             sponsor.
>>>>>>>
>>>>>>>             On Fri, Jun 26, 2015 at 1:53 PM,
>>>>>>>                         Jeremy Manson
>>>>>>>             <jeremymanson at google.com
>>>>>>>             <mailto:jeremymanson at google.com>
>>>>>>>             <mailto:jeremymanson at google.com>
>>>>>>>             <mailto:jeremymanson at google.com>
>>>>>>>             <mailto:jeremymanson at google.com
>>>>>>>             <mailto:jeremymanson at google.com>
>>>>>>>             <mailto:jeremymanson at google.com>>>
>>>>>>>             wrote:
>>>>>>>
>>>>>>>             All this talk about
>>>>>>>             IgnoreUnrecognizedVMOptions
>>>>>>>             reminded me that this
>>>>>>>
>>>>>>>             review is still outstanding. Any
>>>>>>>             takers?
>>>>>>>
>>>>>>>             Jeremy
>>>>>>>
>>>>>>>             On Tue, May 5, 2015 at 10:01 AM,
>>>>>>>                         Jeremy Manson
>>>>>>>             <jeremymanson at google.com
>>>>>>>             <mailto:jeremymanson at google.com>
>>>>>>>             <mailto:jeremymanson at google.com>
>>>>>>>             <mailto:jeremymanson at google.com>
>>>>>>>             <mailto:jeremymanson at google.com
>>>>>>>             <mailto:jeremymanson at google.com>
>>>>>>>             <mailto:jeremymanson at google.com>>
>>>>>>>             wrote:
>>>>>>>
>>>>>>>             Hi David,
>>>>>>>
>>>>>>>             Thanks for taking a look.
>>>>>>>
>>>>>>>             On Mon, May 4, 2015 at 8:32
>>>>>>>                         PM, David
>>>>>>>             Holmes
>>>>>>>             <david.holmes at oracle.com
>>>>>>>             <mailto:david.holmes at oracle.com>
>>>>>>>             <mailto:david.holmes at oracle.com>
>>>>>>>             <mailto:david.holmes at oracle.com>
>>>>>>>             <mailto:david.holmes at oracle.com
>>>>>>>             <mailto:david.holmes at oracle.com>
>>>>>>>             <mailto:david.holmes at oracle.com>>>
>>>>>>>
>>>>>>>             wrote:
>>>>>>>
>>>>>>>             Hi Jeremy,
>>>>>>>
>>>>>>>             On 5/05/2015 6:51 AM,
>>>>>>>             Jeremy Manson wrote:
>>>>>>>
>>>>>>>             Not sure who might be
>>>>>>>             willing to
>>>>>>>             sponsor this. David?  You
>>>>>>>                         did the
>>>>>>>
>>>>>>>             last
>>>>>>>             one.  :)
>>>>>>>
>>>>>>>             Might be a while
>>>>>>>             before I can
>>>>>>>             get to it. I did have
>>>>>>>                         a quick look
>>>>>>>
>>>>>>>             (and
>>>>>>>             will need a longer one).
>>>>>>>
>>>>>>>             I understand. I'm just happy it's
>>>>>>>             possible to upstream this stuff at
>>>>>>>             all[1].
>>>>>>>
>>>>>>>             [1] With the eternal caveat
>>>>>>>                         that I'd be
>>>>>>>             happier if we didn't *have* to
>>>>>>>             go through you folks, but
>>>>>>>             we've learned to
>>>>>>>             live with it.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>             Context: A number of
>>>>>>>             command line
>>>>>>>             options are not settable via
>>>>>>>
>>>>>>>             JAVA_TOOL_OPTIONS and _JAVA_OPTIONS:
>>>>>>>
>>>>>>>             - PrintVMOptions
>>>>>>>
>>>>>>>             So you have changed the
>>>>>>>             semantics of this to
>>>>>>>             print out the
>>>>>>>             options
>>>>>>>
>>>>>>>             from
>>>>>>>             the command-line and each
>>>>>>>                         of the two
>>>>>>>             env vars. Seems reasonable but
>>>>>>>             may be
>>>>>>>             better to know which
>>>>>>>             option came from
>>>>>>>             where as we can now see
>>>>>>>                         the same
>>>>>>>             option (or conflicting
>>>>>>>             variants
>>>>>>>             thereof) multiple times.
>>>>>>>
>>>>>>>             I'd be happy to do
>>>>>>>             this.  Any
>>>>>>>             preferred syntax? Does someone
>>>>>>>
>>>>>>>             actually
>>>>>>>             own this feature?
>>>>>>>
>>>>>>>             I'm unclear what the current
>>>>>>>             processing
>>>>>>>             order is for the different
>>>>>>>
>>>>>>>             sources of options, and
>>>>>>>             whether the
>>>>>>>             changes affect that order?
>>>>>>>
>>>>>>>             Nope.  I go through sad
>>>>>>>             contortions
>>>>>>>             to keep the processing
>>>>>>>             order the
>>>>>>>
>>>>>>>             same.  It's JAVA_TOOL_OPTIONS,
>>>>>>>                         then
>>>>>>>             command line, then _JAVA_OPTIONS.
>>>>>>>             See
>>>>>>>             lines 2549-2567.
>>>>>>>
>>>>>>>
>>>>>>>             -
>>>>>>>             IgnoreUnrecognizedVMOptions
>>>>>>>
>>>>>>>             - PrintFlagsInitial
>>>>>>>
>>>>>>>             Unclear what
>>>>>>>             "initial" actually
>>>>>>>             means - is it the default?
>>>>>>>
>>>>>>>             More or less. If you stick,
>>>>>>>                         for example,
>>>>>>>             IgnoreUnrecognizedVMOptions
>>>>>>>             in
>>>>>>>             there first, it will get
>>>>>>>             printed out as
>>>>>>>             "true".  I haven't really
>>>>>>>             changed
>>>>>>>             the semantics here either -
>>>>>>>                         I've just
>>>>>>>             added processing of the envvars.
>>>>>>>
>>>>>>>             - NativeMemoryTracking
>>>>>>>
>>>>>>>             - PrintFlagsWithComments
>>>>>>>
>>>>>>>             This is because these
>>>>>>>             flags have
>>>>>>>             to be processed before
>>>>>>>             processing
>>>>>>>             other
>>>>>>>             flags, and no one ever
>>>>>>>             bothered to
>>>>>>>             do that for the flags
>>>>>>>             coming from
>>>>>>>             environment
>>>>>>>             variables.  This patch
>>>>>>>             fixes that problem.
>>>>>>>
>>>>>>>             I have a test, but it is a
>>>>>>>             modification to a JDK
>>>>>>>                         test instead
>>>>>>>             of a HS
>>>>>>>             test,
>>>>>>>             so it can go in
>>>>>>>             separately (I guess).
>>>>>>>
>>>>>>>             They can be pushed
>>>>>>>             together to
>>>>>>>             different repos in the
>>>>>>>                         same forest.
>>>>>>>
>>>>>>>             Okay.  Here's the test
>>>>>>>             change:
>>>>>>>
>>>>>>>             http://cr.openjdk.java.net/~jmanson/8079301t/webrev.00/
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.00/>
>>>>>>>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.00/>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.00/>
>>>>>>>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.00/>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.00/>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>             As I said I'll have to get
>>>>>>>                         back to this.
>>>>>>>             And hopefully someone else in
>>>>>>>
>>>>>>>             runtime will take a good
>>>>>>>                         look as well ;-)
>>>>>>>
>>>>>>>             I'd be happy to toss it
>>>>>>>                         over to
>>>>>>>             whomever owns this, if anyone.
>>>>>>>
>>>>>>>             Thanks!
>>>>>>>
>>>>>>>             Jeremy
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>             Bug:
>>>>>>>
>>>>>>>             https://bugs.openjdk.java.net/browse/JDK-8079301
>>>>>>>
>>>>>>>             Webrev:
>>>>>>>             http://cr.openjdk.java.net/~jmanson/8079301/webrev.00/
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.00/>
>>>>>>>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.00/>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.00/>
>>>>>>>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.00/>
>>>>>>>             <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.00/>
>>>>>>>
>>>>>>>
>>>>>>>             Jeremy
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>>
>
>



More information about the hotspot-runtime-dev mailing list