RFR: 8079301: Some command line options not settable via JAVA_TOOL_OPTIONS
Coleen Phillimore
coleen.phillimore at oracle.com
Thu Jul 16 18:09:58 UTC 2015
On 7/15/15 6:39 PM, Jeremy Manson 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.
lol, nah, just under utilized. Most stuff used to be cleaned up by the
GC or by ResourceMark, which the initial implementers of Hotspot thought
was (is) better.
>
> Some crazy day we might even consider rolling forward to the 21st
> century and starting to use unique_ptr in the source base.
That's going a bit further. We still have some years left before the
end of the 21st century though so you never know.
Coleen
>
> 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