Open code review for 8061999 Enhance VM option parsing to allow options to be specified
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Jul 17 18:49:57 UTC 2015
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