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

[Tony Printezis]

Hi all,

I fixed a couple of minor issues in the changes below, here's the 
updated webrev:

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

If you've already started looking at the previous one, here are the two 
(small) fixes:

src/share/vm/gc_implementation/g1/g1CollectorPolicy.hpp
<  double _reserve_regions;
---
 >  size_t _reserve_regions;

src/share/vm/gc_implementation/g1/g1CollectorPolicy.cpp
<            "it's been updated to %u", G1ReservePercent);
---
 >            "it's been updated to %u", reserve_perc);

(with thanks to Bengt for pointing me to the first one)

Tony

On 08/30/2011 05:22 AM, Tony Printezis wrote:
> 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/20110901/c3bec146/attachment.htm>