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

[Poonam Bajaj Parhar]

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
>