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

Poonam Bajaj Parhar poonam.bajaj at oracle.com
Fri Jan 8 22:03:08 UTC 2016


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:
> 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.
We need to copy the original string because strtok modifies the string 
that is passed to it.

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

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

Thanks,
Poonam

> Thanks,
>    Thomas
>




More information about the hotspot-gc-dev mailing list