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

[Jon Masamitsu]

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
>