Fwd: Re: RFR: JDK-8072725: Provide more granular levels for GC verification

[Dmitry Fazunenko]

Hi Poonam,

A few boring comments from SQE:

1) Copyrights:
    src/share/vm/*   should be: 1997, 2016,
    test/gc/TestVerifySubSet.java: should be: 2016,

2) test/gc/TestVerifySubSet.java
    would you use 4-space indents to meet the Java Coding style, please

Thanks,
Dima




On 11.01.2016 19:09, Poonam Bajaj Parhar wrote:
> Hello Thomas,
>
>> Sorry for not finding this earlier, one more issue: the description
>> string of the flag in globals.hpp needs closing quotation marks at the
>> end of every line. Otherwise it will contain lots of additional
>> unnecessary whitespace at the end of every "line" when printed.
>>
>> It might be useful to add a failing test run covering specifying an
>> invalid subset.
>
> Fixed both:
> http://cr.openjdk.java.net/~poonam/8072725/webrev.03/
>
> Please take a look.
>
> Thanks,
> Poonam
>
> On 1/11/2016 1:46 AM, Thomas Schatzl wrote:
>> Hi Poonam,
>>
>> On Fri, 2016-01-08 at 14:03 -0800, Poonam Bajaj Parhar wrote:
>>> Hello Thomas,
>>>
>>> Here's the updated webrev:
>>> http://cr.openjdk.java.net/~poonam/8072725/webrev.03/
>>>
>>> Comments inline..
>>>
>>> On 1/8/2016 6:23 AM, Thomas Schatzl wrote:
>> [...]
>>>> I still have questions:
>>>>
>>>> - why does Universe::initialize_verify_flags() require to allocate
>>>> a
>>>> new buffer for tokenizing? Maybe there is something about strtok
>>>> that I
>>>> am not aware of.
>>> We need to copy the original string because strtok modifies the
>>> string
>>> that is passed to it.
>> Okay. I did not read that far when looking at the documentation. Sorry.
>>
>>>> - I think os::malloc() allocations must be matched by os::free(),
>>>> otherwise some NMT related management may go crazy.
>>>>
>>>> - also I do not think that the error checking makes much sense
>>>> here: if
>>>> the C-heap is so small at startup you may as well exit immediately.
>>>> I
>>>> would actually prefer the code used any of the
>>>> NEW_C_HEAP_ARRAY/FREE_C_HEAP_ARRAY macros because then stuff like
>>>> PrintMallocFree etc will work.
>>> Changed it to NEW_C_HEAP_ARRAY/FREE_C_HEAP_ARRAY.
>>>
>>>> - could you extract the delimiter string for strtok() into a
>>>> constant
>>>> instead of copy&paste?
>>> Done.
>>>
>>>> - maybe the check whether to call Universe::initialize_verify_flags
>>>> could simply be changed into a strlen() call instead of checking
>>>> for a
>>>> NUL character.
>>> Done.
>> Thanks for the changes.
>>
>>>> - the call to Universe::initialize_verify_flags() seems to be
>>>> placed
>>>> very late, maybe somewhere closer to other argument parsing. E.g.
>>>> something like Arguments::parse_arguments()/apply_ergo() seems to
>>>> be
>>>> more appropriate. I won't insist on that.
>>> Verification is done in Universe. So I thought it would be good to
>>> initialize the
>>> Verify option when Universe is being initialized.
>>>
>>> I haven't changed it in the current webrev. But if you strongly feel
>>> that the
>>> call to initialize should be made from Arguments, let me know.
>> No, I guess it is okay.
>>
>>>> - I would also prefer if specifying an unknown flag would cause a
>>>> VM
>>>> shutdown instead of a warning message. This would make its handling
>>>> similar to other flags with typos. Also a single warning message
>>>> will
>>>> often get lost in other output.
>>> Fixed it.
>> Sorry for not finding this earlier, one more issue: the description
>> string of the flag in globals.hpp needs closing quotation marks at the
>> end of every line. Otherwise it will contain lots of additional
>> unnecessary whitespace at the end of every "line" when printed.
>>
>> It might be useful to add a failing test run covering specifying an
>> invalid subset.
>>
>> Thanks,
>>    Thomas
>>
>