Jini,
Looks good. One minor comment:
+ public void printG1HeapSummary(G1CollectedHeap heap) {+
G1CollectedHeap g1h = (G1CollectedHeap) heap;
'heap' has been cast to 'G1CollectedHeap' at call site, so seems no need
to convert here again.
Thanks
Yumin
On Mon, Mar 12, 2018 at 8:52 AM, Jini George <jini.geo...@oracle.com> wrote:
> Thank you very much, Stefan. Could one more reviewer please take a look at
> it ?
>
> - Jini.
>
>
> On 3/12/2018 8:52 PM, Stefan Johansson wrote:
>
>> Hi Jini,
>>
>> This looks good. I'm totally fine with skipping metaspace if that isn't
>> displayed for the other GCs.
>>
>> Cheers,
>> Stefan
>>
>> On 2018-03-09 10:29, Jini George wrote:
>>
>>> Here is the revised webrev:
>>>
>>> http://cr.openjdk.java.net/~jgeorge/8175312/webrev.02/
>>>
>>> I have made modifications to have the 'universe' command display details
>>> like:
>>>
>>> hsdb> universe
>>> Heap Parameters:
>>> garbage-first heap [0x0000000725200000, 0x00000007c0000000] region size
>>> 1024K
>>> G1 Heap:
>>> regions = 2478
>>> capacity = 2598371328 (2478.0MB)
>>> used = 5242880 (5.0MB)
>>> free = 2593128448 (2473.0MB)
>>> 0.20177562550443906% used
>>> G1 Young Generation:
>>> Eden Space:
>>> regions = 5
>>> capacity = 8388608 (8.0MB)
>>> used = 5242880 (5.0MB)
>>> free = 3145728 (3.0MB)
>>> 62.5% used
>>> Survivor Space:
>>> regions = 0
>>> capacity = 0 (0.0MB)
>>> used = 0 (0.0MB)
>>> free = 0 (0.0MB)
>>> 0.0% used
>>> G1 Old Generation:
>>> regions = 0
>>> capacity = 155189248 (148.0MB)
>>> used = 0 (0.0MB)
>>> free = 155189248 (148.0MB)
>>> 0.0% used
>>>
>>>
>>> I did not add the metaspace details since that did not seem to be in
>>> line with the 'universe' output for other GCs. I have added a new command
>>> "g1regiondetails" to display the region details, and have modified the
>>> tests accordingly.
>>>
>>> hsdb> g1regiondetails
>>> Region Details:
>>> Region: 0x0000000725200000,0x0000000725200000,0x0000000725300000:Free
>>> Region: 0x0000000725300000,0x0000000725300000,0x0000000725400000:Free
>>> Region: 0x0000000725400000,0x0000000725400000,0x0000000725500000:Free
>>> Region: 0x0000000725500000,0x0000000725500000,0x0000000725600000:Free
>>> Region: 0x0000000725600000,0x0000000725600000,0x0000000725700000:Free
>>> Region: 0x0000000725700000,0x0000000725700000,0x0000000725800000:Free
>>> ...
>>>
>>> Thanks,
>>> Jini.
>>>
>>>
>>> On 2/28/2018 12:56 PM, Jini George wrote:
>>>
>>>> Thank you very much, Stefan. My answers inline.
>>>>
>>>> On 2/27/2018 3:30 PM, Stefan Johansson wrote:
>>>>
>>>>> Hi Jini,
>>>>>
>>>>
>>>> JIRA ID:https://bugs.openjdk.java.net/browse/JDK-8175312
>>>>>>> Webrev: http://cr.openjdk.java.net/~jgeorge/8175312/webrev.00/index.
>>>>>>> html
>>>>>>>
>>>>>>> It looks like a file is missing, did you forget to add it to the
>>>>> changeset?
>>>>>
>>>>
>>>> Indeed, I had missed that! I added the missing file in the following
>>>> webrev:
>>>>
>>>> http://cr.openjdk.java.net/~jgeorge/8175312/webrev.01/
>>>>
>>>> ---
>>>>> open/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/G1CollectedHeap.java:36:
>>>>> error: cannot find symbol
>>>>> import sun.jvm.hotspot.gc.shared.PrintRegionClosure;
>>>>> ---
>>>>>
>>>>> Otherwise the change looks good, but I would like to see the output
>>>>> live. For a big heap this will print a lot of data, just wondering if the
>>>>> universe command is the correct choice for this kind of output. I like
>>>>> having the possibility to print all regions, so I want the change but
>>>>> maybe
>>>>> it should be a different command and 'universe' just prints a little more
>>>>> than before. Something like our logging heap-summary at shutdown:
>>>>> garbage-first heap total 16384K, used 3072K [0x00000000ff000000,
>>>>> 0x0000000100000000)
>>>>> region size 1024K, 4 young (4096K), 0 survivors (0K)
>>>>> Metaspace used 6731K, capacity 6825K, committed 7040K, reserved
>>>>> 1056768K
>>>>> class space used 559K, capacity 594K, committed 640K, reserved
>>>>> 1048576K
>>>>>
>>>>
>>>> Ok, will add this, and could probably have the region details displayed
>>>> under a new command called "g1regiondetails", or some such, and send out a
>>>> new webrev.
>>>>
>>>> Thanks,
>>>> Jini.
>>>>
>>>>
>>>>> Thanks,
>>>>> Stefan
>>>>>
>>>>>> Modifications have been made to display the regions like:
>>>>>>>
>>>>>>> ...
>>>>>>> Region: 0x00000005c5400000,0x00000005c5600000,0x00000005c5600000:Old
>>>>>>> Region: 0x00000005c5600000,0x00000005c5800000,0x00000005c5800000:Old
>>>>>>> Region: 0x00000005c5800000,0x00000005c5a00000,0x00000005c5a00000:Old
>>>>>>> Region: 0x00000005c5a00000,0x00000005c5c00000,0x00000005c5c00000:Old
>>>>>>> Region: 0x00000005c5c00000,0x00000005c5c00000,0x00000005c5e00000:
>>>>>>> Free
>>>>>>> Region: 0x00000005c5e00000,0x00000005c5e00000,0x00000005c6000000:
>>>>>>> Free
>>>>>>> Region: 0x00000005c6000000,0x00000005c6200000,0x00000005c6200000:Old
>>>>>>> ...
>>>>>>>
>>>>>>> The jtreg test at this point does not include any testing for the
>>>>>>> display of archived or pinned regions. The testing for this will be
>>>>>>> added
>>>>>>> once JDK-8174994 is resolved.
>>>>>>>
>>>>>>> The SA tests pass with jprt and Mach5.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Jini.
>>>>>>>
>>>>>>
>>>>>
>>
