Request for review (m) - 8008508: CMS does not correctly reduce heap size after a Full GC

[Jon Masamitsu]

I've integrated the main change (webrev.02)
to gc_baseline.  Still no review on the refactoring
(webrev02.1) so that will not go back.

Jon

On 03/25/13 19:17, Jon Masamitsu wrote:
> I have reviews for the principle part of the change
> (webrev.02) (thanks JohnCu and Ramki) so will push
> that change.  The second part (webrev02.1) is a
> refactoring to share code.  I don't have any reviews
> for that so will not push it.
>
> Jon
>
>
> On 3/13/2013 8:53 PM, Jon Masamitsu wrote:
>> Ramki,
>>
>> I've replied to your coding comments below.
>>
>> All,
>>
>>  I've update the webrev for all the changes so far.
>>
>> http://cr.openjdk.java.net/~jmasa/8008508/webrev.02/
>>
>> I've also added two helper functions for compute_new_size()
>> so that code could be shared as appropriate.  Just the
>> refactoring changes are in this webrev.
>>
>> http://cr.openjdk.java.net/~jmasa/8008508/webrev.02.1/
>>
>> Thanks.
>>
>> Jon
>>
>> On 3/2/2013 12:42 AM, Srinivas Ramakrishna wrote:
>>> Hi Jon --
>>>
>>> I'll give a couple of code-level comments, but there's a high-level
>>> comment at the end, which
>>> might be worth pondering over.
>>>
>>> On Tue, Feb 26, 2013 at 3:47 PM, Jon 
>>> Masamitsu<jon.masamitsu at oracle.com>  wrote:
>>>> I've updated the webrev for review comments (thanks, JohnCu)
>>>>
>>>> http://cr.openjdk.java.net/~jmasa/8008508/webrev.01/
>>>>
>>> concurrent MarkSweepGeneration.cpp:
>>>
>>>   949   // compute expansion delta needed for reaching desired free 
>>> percentage
>>>   950   if (free_percentage < desired_free_percentage) {
>>>   951     size_t desired_capacity = (size_t)(used() / ((double) 1 -
>>> desired_free_percentage));
>>>   952     assert(desired_capacity >= capacity(), "invalid expansion 
>>> size");
>>>   953     size_t expand_bytes = MAX2(desired_capacity - capacity(),
>>> MinHeapDeltaBytes);
>>>
>>>   954     if (expand_bytes > 0) {
>>>
>>> The if test at 954 (in your changed code) will always be true because
>>> MinHeapDeltaBytes > 0
>>> and we are taking the max at line 953.
>>
>> Ok.  MinHeapDeltaBytes is a product flag so it could be set to 0 but 
>> other
>> collectors ignore that possibility so I'll take out the test.
>>
>>
>>> On reading your shrinking code further below at lines 989-994, it
>>> struck me that there's a small
>>> existing bug in the shrink/expand code here. It's probably more of a
>>> factor in the shrink code though
>>> because we can live with more free space than we'd ideally want, but
>>> not less than we ideally want,
>>> and we certainly do not want to chatter around the desired free
>>> percentage (hence the delta).
>>>
>>> Your shrinking arm looks like this:-
>>>
>>>   989   } else {
>>>   990     size_t desired_capacity = (size_t)(used() / ((double) 1 -
>>> desired_free_percentage));
>>>   991     assert(desired_capacity <= capacity(), "invalid expansion 
>>> size");
>>>   992     size_t shrink_bytes = MAX2(capacity() - desired_capacity,
>>> MinHeapDeltaBytes);
>>>   993     shrink_free_list_by(shrink_bytes);
>>>   994   }
>>>
>>> I'd modify it to:-
>>>
>>>   989   } else {
>>>   990     size_t desired_capacity = (size_t)(used() / ((double) 1 -
>>> desired_free_percentage));
>>>   991     assert(desired_capacity <= capacity(), "invalid expansion 
>>> size");
>>>              size_t shrink_bytes = capacity() - desired_capacity;
>>>              // Don't shrink unless the delta is greater than the
>>> minimum shrink we want
>>>   992     if (shrink_bytes >= MinHeapDeltaBytes) {
>>>   993         shrink_free_list_by(shrink_bytes);
>>>              }
>>>   994   }
>>
>> I changed the code as you suggested.  Thanks for catching this.
>>
>>> Note the asymmetry between expansion and shrinking, where we expand by
>>> the min if we feel we are below the desired free, but we don't shrink
>>> even if we are above desired free unless the gap exceeds the min
>>> delta.
>>
>> Seems right.
>>> This seems to be the correct policy.  (Of course none of that is moot
>>> until shrink_free_list_by() starts doing shrinking, but probably best
>>> to fix the policy now, so that it doesn't cause chatter later.
>>>
>>> concurrentMarkSweepGeneration.hpp:
>>>
>>> 1298   virtual void compute_new_size();
>>> 1299   void compute_new_size_free_list();
>>>
>>> I think it would be good to add a 1-line doc comment for each of the
>>> above methods.
>>
>> Added
>>
>>   // Resize the generation after a compacting GC.  The
>>   // generation can be treated as a contiguous space
>>   // after the compaction.
>>   virtual void compute_new_size();
>>   // Resize the generation after a non-compacting
>>   // collection.
>>   void compute_new_size_free_list();
>>
>>
>>> vmStructs.cpp:
>>>
>>> I think you also have to adjust the fields _capacity_at_prologue and
>>> _used_at_prologue into the superclass. I'd also move the decls up to
>>> CardGeneration, since that's how the original code is laid out.
>>
>> Done
>>
>>> Finally, a high level comment. While I see the reason for these
>>> mechanics, I worry a little bit
>>> about the somewhat schizophrenic expansion/shrinkage policy, as it
>>> swings between one policy
>>> used when we are presumably doing well and keeping up with the 
>>> application's
>>> allocation/promotion rates and not running into concurrent mode
>>> failure, but then suddenly switching
>>> to a completely different expansion/shrinking policy when we do a
>>> compaction (presumably because
>>> we had a concurrent mode failure). I'd much rather have divorced the
>>> two, by using a more uniform
>>> policy specifically for CMS, perhaps predicated by whether this was
>>> happening after a
>>> compaction or not, but keeping it separate from the policy that might
>>> be used for stop-world
>>>   kind of collectors (hence TenuredGeneraion).
>>>
>>> I'd appreciate it if you could give some thought to this final high
>>> level comment regarding the
>>> general policy approach taken here. (Also it is somewhat troubling to
>>> push policy so high up
>>> into the class hierarchy, so that every subclass must live with it;
>>> I'd much rather the policy
>>> were separated from the mechanics of what would be done when an
>>> expansion or shrinkage
>>> was needed.)
>>>
>>> -- ramki
>>
>