RFR (S): 8004172: Update jstat counter names to reflect metaspace changes (original) (raw)

Erik Helin erik.helin at oracle.com
Fri Feb 8 00:08:19 PST 2013

On 02/07/2013 07:49 PM, Mandy Chung wrote:

On 2/7/2013 4:36 AM, Erik Helin wrote: Hi Mandy,

thanks for the review! See comments inline. On 02/05/2013 09:14 PM, Mandy Chung wrote:

Looks good. Minor nits:

gcCapacityOutput1.awk - should the last column in L7 be removed? I don't think so, jmap -gccause outputs the GC cause in the last column, which sometimes can be "No GC". Why do you think it should be removed? This fix changes from 4 columns "PGCMN PGCMX PGC PC" to 3 columns "MCMN MCMX MC". Line 7 is an example output that I would think the number of columns is expected to match with line 6. Is that right?

I'm sorry, I misread gcCapacityOutput1.awk as gcCauseOutput1.awk (that is why my answer makes no sense at all). You are completely right, the last column on line 7 should be removed!

New webrev located at: http://cr.openjdk.java.net/~ehelin/8004172/jdk/webrev.05/

On 02/07/2013 07:49 PM, Mandy Chung wrote:

On 02/05/2013 09:14 PM, Mandy Chung wrote:

How are you going to integrate the hotspot and jdk fixes in a synchronized fashion? We shall make sure the TL sun/tools/jstat tests pass at all times.

Thanks for catching this. the tests in tl do passes (I was wrong about this). The change will have to be pushed in several steps: 1. Add new metaspace counters (and keep the old ones). This will be pushed to hotspot-gc. 2. Wait for the change to trickle down to tl. 3. Update the tests and jstat to use the new metaspace counters. 4. Wait for this change to trickle down to hotspot-gc. 5. Remove the old metaspace counters and push this to hotspot-gc. Do you want me to send out an additional webrev for what the hotspot code will look like with both the old and new counters? Jon may want to review the hotspot code if revised. I only review the jdk change. Thanks for considering doing a 2-phase hotspot change. This kind of synchronized change is painful. For this specific change, I'm open to temporarily disable these jstat tests for one integration cycle since the impact is only in jstat counters and well isolated and that'll ease the pain. i.e. (1) your hotspot change + add jstat tests in jdk/test/ProblemList.txt to hotspot-gc and when it's pulled down to TL (2) your jdk jstats change + remove jstat tests in jdk/test/ProblemList.txt in TL. Just another option for you to consider. No issue from me in either option.

Thanks, this is will at least make the push a little bit easier! I will talk to Alejandro to make sure that jdk/test/ProblemList.txt gets pushed all the way to the tl repository.

Thanks, Erik

Mandy

More information about the serviceability-dev mailing list