[Fwd: Re: Review request: 6581734 CMS Old Gen's collection usage is zero after GC which is incorrect]

[Mandy Chung]

Kevin Walls wrote:
>
> Great, thanks - respun as 
> http://cr.openjdk.java.net/~kevinw/6581734/webrev.02/
>

This looks better.

Test6581734.java:
   All methods/fields in this class should have indented (currently not 
indented).  Also line 118 has 3 extra spaces.

Thanks
Mandy
>
>
> Mandy Chung wrote:
>> Hi Kevin,
>>
>> Thanks for fixing this.  Looks good with some minor comments:
>>
>> memoryManager.hpp line 181-182:
>>   get_last_gc_stat() returns gc_index but the comment says it returns 
>> the size of the array pool.
>>
>> management.cpp line 1908:
>>   if (!mgr->get_last_gc_stat(stat) == 0) {
>>
>> You forgot to take out "!" when you changed the get_last_gc_stat() to 
>> return size_t instead of bool.
>>
>> test/gc/6581734/Test6581734.java
>>   @run main/othervm -server -Xmx512m -verbose:gc ...
>>
>> Why do you need "-server"?  We also want client VM to be tested, right?
>>
>> You can drop -verbose:gc and use MemoryMXBean.setVerbose(true) to 
>> turn on gc tracing if you like.
>>
>> The test source isn't formatted properly, e.g. line 49-56, 69-75, 
>> 79-124, 127, 131.  Can you fix the formatting - using 4 spaces as 
>> indentation (not tab or 2 spaces)?
>>
>> Thanks
>> Mandy
>>
>> Kevin Walls wrote:
>>>
>>> Hi Mandy,
>>>
>>> Yes, thanks for the discussions we've had so far. 8-)
>>>
>>> Here's an updated JDK7 version:
>>>
>>> http://cr.openjdk.java.net/~kevinw/6581734/webrev.01/
>>>
>>> Thanks
>>> Kevin
>>>
>>> Mandy Chung wrote:
>>>> Kevin,
>>>>
>>>> As I know, you're revising the fix for the 6 update release.   I'll 
>>>> review your revised webrev when it's ready.
>>>>
>>>> Thanks
>>>> Mandy
>>>>
>>>> Kevin Walls wrote:
>>>>> Hi -
>>>>>
>>>>> This is a review request for the issue where cms doesn't update 
>>>>> the statistics used by the MemoryPoolMXBean system: only 
>>>>> concurrent mode failures update the stats.  We just aren't 
>>>>> prepared for concurrent collections, so it takes a fair amount of 
>>>>> customisation to add that flexibility...
>>>>>
>>>>> http://cr.openjdk.java.net/~kevinw/6581734/webrev/
>>>>>
>>>>> Thanks
>>>>> Kevin
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>