<!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>