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

[Srinivas Ramakrishna]

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.

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   }

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.

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.

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.

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