RFR: 8079301: Some command line options not settable via JAVA_TOOL_OPTIONS
Jeremy Manson
jeremymanson at google.com
Wed Jul 15 22:39:31 UTC 2015
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