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

Jon Masamitsu jon.masamitsu at oracle.com
Mon Mar 4 10:21:50 PST 2013


Ramki,

Thanks for taking a look.  Let's talk about the high level
comment before the details.

Are you saying that the amount of desired free space in the
cms generation after a collection should be independent of
whether the collection was a concurrent collection or a
full STW collection?

Jon


On 03/02/13 00:42, 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.
>
> 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


More information about the hotspot-gc-dev mailing list