RFR(L) 8154154: Separate G1 specific policy code from the CollectorPolicy class hierarchy

Kim Barrett kim.barrett at oracle.com
Fri Apr 15 17:27:18 UTC 2016


> On Apr 15, 2016, at 3:37 AM, Mikael Gerdin <mikael.gerdin at oracle.com> wrote:
> On 2016-04-15 01:06, Kim Barrett wrote:
>>> On Apr 14, 2016, at 7:23 AM, Mikael Gerdin <mikael.gerdin at oracle.com> wrote:
>>> Testing: RBT GC Testing, JPRT, Performance testing on aurora.
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8154154
>>> Webrev: http://cr.openjdk.java.net/~mgerdin/8154154/webrev.0/
>> 
>> OK, this review was *much* easier than I expected.
>> 
>> Looks good, with just a few very minor comments.
> 
> Thanks.
> I'd like to pick up on your offer of deferring the cleanups to new RFEs since this change feels large enough as-is and I've tried to avoid changing too much of the semantics in this change.

OK.

>> src/share/vm/gc/g1/concurrentG1Refine.cpp
>>   30 #include "gc/g1/g1Policy.hpp"
>>   59   G1Policy* policy = g1h->g1_policy();
>> 
>> These changes will be obsoleted by my in-review changes for 8133051.
>> One of use is going to have a (minor) merge collision to deal with.
> 
> Yep, I don't mind taking the merge hit since your change already has a bunch of reviews.

Well, you have two completed reviews, where I think I only have one so far.
But the repos are closed right now anyway.  I don’t expect it to be a big deal
for whichever of us goes second, so long as we’re not both trying to push at
the same time and the second gets bounced for merge conflicts.

>> src/share/vm/gc/g1/g1YoungGenSizer.hpp
>>   88   // Calculate the maximum length of the young gen given the number of regions
>>   89   // depending on the sizing algorithm.
>>   90   void compute_max_new_size(uint number_of_heap_regions);
>> 
>> A name of calculate_xxx suggests to me that it computes and returns a
>> value.  This function is called for side effect, e.g. it updates
>> MaxNewSize.
> 
> I agree, how about record_max_new_size?

adjust_max_new_size is my current favorite, but you decide.  update_xxx is another option.




More information about the hotspot-gc-dev mailing list