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

[Poonam Bajaj Parhar]

Hello Dmitry,

On 1/11/2016 9:00 AM, Dmitry Fazunenko wrote:
> 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
>
Will make these changes. Thanks for the review!

regards,
Poonam

> 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
>>>
>>
>