Open code review for 8061999 Enhance VM option parsing to allow options to be specified
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Jul 20 15:42:31 UTC 2015
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