Fwd: Re: RFR: JDK-8072725: Provide more granular levels for GC verification
thomas.schatzl at oracle.com
Fri Jan 8 14:23:05 UTC 2016
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:
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
- 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.
More information about the hotspot-gc-dev