Open code review for 8061999 Enhance VM option parsing to allow options to be specified
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Jul 20 15:43:13 UTC 2015
Yes, this sounds like a good plan.
Thanks for your help in addressing my concerns.
Coleen
On 7/20/15 11:42 AM, Daniel D. Daugherty wrote:
> Coleen,
>
> Thanks for taking the time to look at how Ron's work conflicts
> with Jeremy's work. We knew there would be some collision and
> having another pair of eyes check this out is a Good Thing (TM).
>
> Rather than clog the alias with a lot of back-and-forth about
> how to merge Ron's work with Jeremy's work, let's take it off
> alias until we get closure on another webrev.
>
> Ron is out of the office today. I believe Dmitry is on vacation
> all week so we won't have his lightning fast test updates until
> he gets back. I still need to spin up on Jeremy's changes.
>
> Does this sound like an OK plan?
>
> Dan
>
>
> On 7/20/15 9:21 AM, Coleen Phillimore wrote:
>>
>> Hi Dan,
>>
>> I have comments about the round 1 of this change. In light of
>> Jeremy Manson's refactoring, I don't think much of the code that was
>> added for this RFE needs to be added. There are functions it can
>> call instead and ScopeVMInitArgs destructor can replace the free
>> calls that were added. This code also had duplicates parsing that is
>> done for environment variable. Further I think the logging added in
>> this change is sandbox debugging and not worthy of adding a Logging
>> flag, and should be removed. I can send more specific comments but
>> the summary is that I would like to see this change integrated better
>> with existing code, rather than checked in.
>>
>> Thanks,
>> Coleen
>>
>>
>> On 7/20/15 10:49 AM, Daniel D. Daugherty wrote:
>>> Coleen,
>>>
>>> Ron's out of the office today (Monday) so I'll chime in.
>>>
>>> We asked Ron to finish addressing the round 0 code review comments
>>> before rebasing his project to the current RT_Baseline in order to
>>> preserve the proper context for those replies and adjustments.
>>>
>>> Ron's project was last rebased just before Gerard's awesome cmd
>>> line option validation work. We also knew that Jeremy's env var
>>> changes were coming and that we likely had conflicts there. Lots
>>> of dust is flying in the cmd line options arena lately!
>>>
>>> Dan
>>>
>>>
>>> On 7/17/15 12:49 PM, Coleen Phillimore wrote:
>>>>
>>>> Sorry, it's a RAII class. I got the acronym confused with Robert
>>>> Strout :)
>>>>
>>>> Coleen
>>>>
>>>> On 7/17/15 2:47 PM, Coleen Phillimore wrote:
>>>>>
>>>>> Ron,
>>>>>
>>>>> The code that we've just reviewed and I'm sponsoring from Jeremy
>>>>> Manson directly conflicts with this code. Jeremy has added a
>>>>> RESII class for JavaVMInitArgs that you should use. I have other
>>>>> initial comments but I haven't gotten to reading all of this
>>>>> thread yet. Please wait for my code review.
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>
>>>>> On 7/16/15 4:00 PM, Ron Durbin wrote:
>>>>>> I will file it and thanks for the "r" review.
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: gerard ziemski
>>>>>>> Sent: Thursday, July 16, 2015 1:48 PM
>>>>>>> To: Ron Durbin; hotspot-runtime-dev at openjdk.java.net
>>>>>>> Subject: Re: Open code review for 8061999 Enhance VM option
>>>>>>> parsing to allow options to be specified
>>>>>>>
>>>>>>> hi Ron,
>>>>>>>
>>>>>>> I have no more comments, just one question: you say "A future
>>>>>>> enhancement will be to refactor the options file processing to grow
>>>>>>> memory like the environment variables do now." - is that
>>>>>>> enhancement
>>>>>>> filed yet?
>>>>>>>
>>>>>>> Please consider this reviewed with small "r".
>>>>>>>
>>>>>>>
>>>>>>> cheers
>>>>>>>
>>>>>>>
>>>>>>> On 07/14/2015 05:21 PM, Ron Durbin wrote:
>>>>>>>> Gerard,
>>>>>>>>
>>>>>>>> Thanks for your time in reviewing the code and providing comments.
>>>>>>>> My responses to your comments inline below:
>>>>>>>>
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Gerard Ziemski
>>>>>>>>> Sent: Tuesday, June 23, 2015 1:53 PM
>>>>>>>>> To: hotspot-runtime-dev at openjdk.java.net
>>>>>>>>> Cc: Ron Durbin
>>>>>>>>> Subject: Re: Open code review for 8061999 Enhance VM option
>>>>>>>>> parsing to allow options to be specified
>>>>>>>>>
>>>>>>>>> hi Ron,
>>>>>>>>>
>>>>>>>>> I'm sending you partial feedback, since I am starting to get a
>>>>>>>>> bit tired. I will spend more time reviewing
>>>>>>> your
>>>>>>>>> webrev tomorrow, but here is what I have so far:
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>> src/share/vm/utilities/globalDefinitions.hp
>>>>>>>>>
>>>>>>>>> 1. Should we name the method "is_white()", not "iswhite()"
>>>>>>>>> since it's part of our code base? But then since
>>>>>>> it's
>>>>>>>>> a macro, shouldn't it actually be "IS_WHITE()" ?
>>>>>>>>> ---
>>>>>>>>> src/share/vm/runtime/arguments.hpp
>>>>>>>>> [
>>>>>>>> [Ron] While researching my answer to this comment, I realized
>>>>>>>> that I should have used the already existing isspace()
>>>>>>>> function
>>>>>>>> instead of creating the iswhite() macro. Will fix.
>>>>>>>>
>>>>>>>>> 1.
>>>>>>>>>
>>>>>>>>> All the arguments in alloc_JVM_options_list(),
>>>>>>>>> copy_JVM_options_from_buf() and parse_JVM_options_file() use
>>>>>>>>> underscores in the arguments names except for
>>>>>>>>> merge_JVM_options_file(). I think the merge_JVM_options_file()
>>>>>>>>> should be:
>>>>>>>>>
>>>>>>>>> + static jint merge_JVM_options_file(const struct
>>>>>>>>> JavaVMInitArgs *args_in,
>>>>>>>>> + struct JavaVMInitArgs
>>>>>>>>> **args_out);
>>>>>>>>>
>>>>>>>> [Ron] This one is a reasonable clean up
>>>>>>>>
>>>>>>>>> ---
>>>>>>>>> src/share/vm/runtime/arguments.cpp
>>>>>>>>>
>>>>>>>>> 1. Why do FreeVMOptions, DumpVMOptions, DumpVMOption and
>>>>>>>>> DumpOption start with capital letters? Shouldn't
>>>>>>> their
>>>>>>>>> names start with a lower case letter?
>>>>>>>>>
>>>>>>>> [Ron] This one is a reasonable clean up
>>>>>>>>
>>>>>>>>> 2. Line 4306. The pattern in Arguments::parse() seems to be to
>>>>>>>>> print out error message and return the error
>>>>>>>>> value if something goes wrong, but we do vm_exit(1) instead?
>>>>>>>>>
>>>>>>>> [Ron ] The cases that will trigger this exit on fail are extreme:
>>>>>>>> - missing or otherwise in accessible options files
>>>>>>>> - Un-parsable options file, too big, too many options,
>>>>>>>> nonwhite space terminated options
>>>>>>>> - Unable to allocate memory for options files processing
>>>>>>>>
>>>>>>>>> 3. Line 4309. I was told that when it comes to NULL pointer
>>>>>>>>> check we should do (NULL == args), not the other
>>>>>>> way
>>>>>>>>> around.
>>>>>>>>>
>>>>>>>> [Ron] This one is a reasonable clean up
>>>>>>>>
>>>>>>>>> 4. Line 4375. Don't we need FreeVMOptions() here? Line 4382 as
>>>>>>>>> well?
>>>>>>>>
>>>>>>>> [Ron] Gerard this one is a reasonable clean up
>>>>>>>>
>>>>>>>>> 5. Question. Why do we need N_MAX_OPTIONS?
>>>>>>>> [Ron] Until recently N_MAX_OPTIONS applied to environment
>>>>>>>> variables too
>>>>>>>> N_MAX_OTIIONS is used to limit the number of options
>>>>>>>> and thus the memory allocated.
>>>>>>>> A future enhancement will be to refactor the options
>>>>>>>> file processing to grow memory like the
>>>>>>>> environment variables do now.
>>>>>>>>
>>>>>>>>> 6. Question. Why do we need OPTION_BUFFER_SIZE? Can't we use
>>>>>>>>> "int bytes_needed = fseek(stream, 0, SEEK_END);
>>>>>>>>> rewind(stream)" and have the code dynamically allocate memory
>>>>>>>>> without hard coded limits?
>>>>>>>>>
>>>>>>>> [Ron>] Until recently OPTION_BUFFER_SIZE applied to environment
>>>>>>>> variables too. \
>>>>>>>> OPTION_BUFFER_SIZE is used to limit the memory
>>>>>>>> allocated for reading the options.
>>>>>>>> A future enhancement will be to refactor the options
>>>>>>>> file processing to grow memory like the
>>>>>>>> environment variables do now.
>>>>>>>>
>>>>>>>>
>>>>>>>>> 7. Line 3965. Can that comparison ever succeed? "read()" will
>>>>>>>>> not read more bytes (only less) than as
>>>>>>> specified
>>>>>>>>> by "bytes_alloc" and if it did, we would overwrite memory
>>>>>>>>> since our buf is only "bytes_alloc" big.
>>>>>>>>>
>>>>>>>> [Ron>] Yes that comparison can succeed. We only support an
>>>>>>>> options file that is <= OPTION_BUFFER_SIZE bytes in
>>>>>>>> size.
>>>>>>>>
>>>>>>>> We allocate a read buffer that is OPTION_BUFFER_SIZE + 1
>>>>>>>> bytes in size for two reasons:
>>>>>>>>
>>>>>>>> 1) to have space for a NULL terminator when the
>>>>>>>> options file is the maximum number of bytes
>>>>>>>> in length
>>>>>>>> 2) easy detection of an options file that is too large;
>>>>>>>> we try to read OPTION_BUFFER_SIZE + 1 bytes. If we
>>>>>>>> succeed, then the file is too big.
>>>>>>>>
>>>>>>>>
>>>>>>>>> cheers
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 6/22/2015 7:52 AM, Ron Durbin wrote:
>>>>>>>>> Webrev URL:
>>>>>>>>> http://cr.openjdk.java.net/~rdurbin/8061999_OCR0_JDK9_webrev
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> RFE request:
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8061999
>>>>>>>>>
>>>>>>>>> This RFE allows a file to be specified that holds VM Options that
>>>>>>>>> would otherwise be specified on the command line or in an
>>>>>>>>> environment variable.
>>>>>>>>> Only one options file may be specified on the command line and
>>>>>>>>> no options file
>>>>>>>>> may be specified in either of the following environment variables
>>>>>>>>> "JAVA_TOOL_OPTIONS" or "_JAVA_OPTIONS".
>>>>>>>>>
>>>>>>>>> The options file feature supports all VM options currently
>>>>>>>>> supported on
>>>>>>>>> the command line, except the options file option. The option
>>>>>>>>> to specify an
>>>>>>>>> options file is "-XX:VMOptionsFile=<Filename>".
>>>>>>>>> The options file feature supports an options file up to 1024
>>>>>>>>> bytes in size
>>>>>>>>> and up to 64 options.
>>>>>>>>>
>>>>>>>>> This feature has been tested on:
>>>>>>>>> OS:
>>>>>>>>> Solaris, MAC, Windows, Linux
>>>>>>>>> Tests:
>>>>>>>>> Manual unit tests
>>>>>>>>> JPRT with -testset hotspot (including the SQE proposed
>>>>>>>>> test coverage for this feature.)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list