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
