RFR: 8079301: Some command line options not settable via JAVA_TOOL_OPTIONS
Coleen Phillimore
coleen.phillimore at oracle.com
Thu Jul 16 18:02:20 UTC 2015
I think this looks good. I will sponsor it. Please send me the hg
export of the changeset.
Thanks!
Coleen
On 7/16/15 1:20 PM, 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