RFR: 8079301: Some command line options not settable via JAVA_TOOL_OPTIONS
Dmitry Dmitriev
dmitry.dmitriev at oracle.com
Thu Jul 16 11:06:26 UTC 2015
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