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

Thomas Schatzl thomas.schatzl at oracle.com
Fri Jan 8 14:23:05 UTC 2016


Hi,

On Thu, 2016-01-07 at 06:28 -0800, Poonam Bajaj Parhar wrote:
> Hello Kevin,
> 
> Thanks for looking at the changes and for your feedback.
> 
> On 1/5/2016 1:08 PM, Kevin Walls wrote:
> >  Hi Poonam,
> > 
> > Great, I think it looks good, and really look forward to having
> > this ability to fine-tune what we verify, rather than being so
> > heavyweight about it!
> > 
> > I just noticed: should_verify_subset("heap") and
> > should_verify_subset("c-heap") are going to clash?  If you specify
> > c-heap, you'll trigger both verifications as strstr finds "heap" in
> > there?  Do you want to change heap to java-heap, or maybe use
> > java_heap and c_heap (if you want to use underscores to be
> > consistent with symbol_table, string_table), or something else to
> > make them not clash?
> > 
> Yes, it is a problem with the current code. To avoid this problem and
> such future problems when we may add more subsets, I have updated the
> code to parse the SubSet string once and store the choices in a flag.
> And when we need to verify, we check for the bits in this flag.
> 
> >  
> > (As we discussed over email briefly, another possibility was
> > splitting the given argument string on commas, doing our string
> > comparisons once only during argument processing somewhere, and
> > populating a long of flag bits, so Universe::verify() just checks
> > for presence of bits, rather than comparing lots of characters. 
> > The speed isn't a particular worry right now so your very readable
> > string comparisons do the job!)
> Here's the updated webrev:
> http://cr.openjdk.java.net/~poonam/8072725/webrev.02/

I think this change improves the use quite a bit, thanks :)

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.

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

- could you extract the delimiter string for strtok() into a constant
instead of copy&paste?

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

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

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

Thanks,
  Thomas




More information about the hotspot-gc-dev mailing list