CRR (M): 7084509: G1: fix inconsistencies and mistakes in the young list target length calculations

[Tony Printezis]

Hi all,

I'd like one more review for this change (Bengt already reviewed it; I 
accidentally "stepped on his toes" with this refactoring, so we reviewed 
each other's changes to see how to move forward):

http://cr.openjdk.java.net/~tonyp/7084509/webrev.0/

This change fixed several issues in the young target length calculations:

- There are two entry points to the calculation: 
calculate_young_list_target_length() and 
calculate_young_list_target_length(size_t rs_lengths). The former calls 
the latter, but in some cases the latter can be called by itself too. 
But there are some extra calculations (max survivor size, max GC locker 
expansion) that were not done when the latter was called by itself. 
Additionally, when calculate_young_list_target_length() was called it 
also required for another method to also be called beforehand 
(calculate_min_young_list_length()).

Fix: replace the above with a single method, 
update_young_list_target_length() which takes an optional rs_lengths 
parameter. Everything is done inside it to ensure that everything that 
needs to be calculated, it is calculated and no other methods need to be 
called beforehand. This also ensures that if we want to apply any min / 
max bounds to the young target length, we can do so in a single place.

- The max survivor size is done with an integer division. This means 
that, if the resulting value is between 0.0 and 1.0, the max survivor 
size will be 0 which effectively tenures everything during the next GC. 
It'd be better if it was 1.

Fix: use double division and ceiling in order for the max survivor size 
to be 1 in the above case. Additionally, I now calculate the survivor 
parameters at the beginning of a pause instead of when the young target 
length is calculated / recalculated. Since those parameters only affect 
the next GC it's pointless to calculate / recalculate them earlier.

- The code that calculates the optimal young target length (i.e., the 
max young length predicted to be within the required pause time) is 
embarrassingly incorrect. It uses binary search to yield the optimal 
length, but unfortunately exits early and in many situations returns a 
young target length that is shorter than it could be.

Fix: updated the binary search algorithm to do the right thing. I 
compared the before / after calculations and the after calculation 
consistently yielded longer young target lengths which still fit within 
the required pause time.

Additional fixes:

- I now calculate the heap reserve every time the heap is resized (as it 
stays the same for a given heap size). There's no point in recalculating 
it every time we do the young target length calculations.

- Refactoring and simplification to make the code easier to follow. This 
should help make the changes for the following two CRs easier:

6929868: G1: introduce min / max young gen size bounds
7084525: G1: Generate ergonomic decision log records for young gen 
sizing and for pause prediction

The bulk of the changes are in G1CollectorPolicy. It might be easier if 
you looked at the new versions of the following methods:

G1CollectorPolicy::predict_will_fit()
G1CollectorPolicy::calculate_young_list_desired_min_length()
G1CollectorPolicy::calculate_young_list_desired_max_length()
G1CollectorPolicy::update_young_list_target_length()
G1CollectorPolicy::calculate_young_list_target_length()

and compared them to the previous versions instead of looking at their 
diffs.

Tony

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20110830/7813549c/attachment.htm>