RFR: 8079301: Some command line options not settable via JAVA_TOOL_OPTIONS
Jeremy Manson
jeremymanson at google.com
Thu Jul 16 00:27:17 UTC 2015
New rev:
http://cr.openjdk.java.net/~jmanson/8079301/webrev.05/
Thanks for all the attention!
On Wed, Jul 15, 2015 at 3:39 PM, Jeremy Manson <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> 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/>
>>
>> Jeremy
>>
>> On Fri, Jul 10, 2015 at 5:52 AM, Dmitry Dmitriev <
>> <dmitry.dmitriev at oracle.com>dmitry.dmitriev at oracle.com
>> <mailto:dmitry.dmitriev at oracle.com> <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>
>>
>> Jeremy
>>
>> On Thu, Jul 9, 2015 at 2:52 PM, Dmitry Dmitriev
>> <dmitry.dmitriev at oracle.com
>> <mailto:dmitry.dmitriev at oracle.com>
>> <dmitry.dmitriev at oracle.com>
>> <mailto:dmitry.dmitriev at oracle.com
>> <dmitry.dmitriev at oracle.com>
>> <mailto:dmitry.dmitriev at oracle.com>
>> <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/>
>>
>> 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/>
>>
>> (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>
>> <coleen.phillimore at oracle.com>
>> <mailto:coleen.phillimore at oracle.com
>> <coleen.phillimore at oracle.com>
>> <mailto:coleen.phillimore at oracle.com> <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>
>>
>> "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> <martinrb at google.com> <
>> mailto:martinrb at google.com <martinrb at google.com>
>> <mailto:martinrb at google.com> <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>
>> jeremymanson at google.com
>> <mailto:jeremymanson at google.com> <jeremymanson at google.com>
>> <mailto:jeremymanson at google.com <jeremymanson at google.com>
>> <mailto:jeremymanson at google.com> <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> <jeremymanson at google.com>
>> <mailto:jeremymanson at google.com <jeremymanson at google.com>
>> <mailto:jeremymanson at google.com> <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>
>> <david.holmes at oracle.com>
>> <mailto:david.holmes at oracle.com <david.holmes at oracle.com>
>> <mailto:david.holmes at oracle.com> <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/>
>>
>>
>> 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/>
>>
>> Jeremy
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>
More information about the hotspot-runtime-dev
mailing list