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:21:35 UTC 2015
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