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

[Thomas Schatzl]

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