<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
  <head>

    <meta http-equiv="content-type" content="text/html; charset=ISO-8859-1">
  </head>
  <body text="#000000" bgcolor="#ffffff">
    Hi all,<br>
    <br>
    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):<br>
    <br>
    <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tonyp/7084509/webrev.0/">http://cr.openjdk.java.net/~tonyp/7084509/webrev.0/</a><br>
    <br>
    This change fixed several issues in the young target length
    calculations:<br>
    <br>
    - 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()).<br>
    <br>
    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.<br>
    <br>
    - 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.<br>
    <br>
    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.<br>
    <br>
    - 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.<br>
    <br>
    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.<br>
    <br>
    Additional fixes:<br>
    <br>
    - 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.<br>
    <br>
    - Refactoring and simplification to make the code easier to follow.
    This should help make the changes for the following two CRs easier:<br>
    <br>
    6929868: <font face="">G1: introduce min / max young gen size
      bounds</font><br>
    7084525: G1: Generate ergonomic decision log records for young gen
    sizing and for pause prediction<br>
    <br>
    The bulk of the changes are in G1CollectorPolicy. It might be easier
    if you looked at the new versions of the following methods:<br>
    <br>
    G1CollectorPolicy::predict_will_fit()<br>
    G1CollectorPolicy::calculate_young_list_desired_min_length()<br>
    G1CollectorPolicy::calculate_young_list_desired_max_length()<br>
    G1CollectorPolicy::update_young_list_target_length()<br>
    G1CollectorPolicy::calculate_young_list_target_length()<br>
    <br>
    and compared them to the previous versions instead of looking at
    their diffs.<br>
    <br>
    Tony<br>
    <br>
  </body>
</html>