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

[Jon Masamitsu]

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