RFR (M): 8136681: Factor out IHOP calculation from G1CollectorPolicy

[Mikael Gerdin]

Hi Thomas,

On 2015-11-05 10:42, Thomas Schatzl wrote:
> Hi all,
>
>    can I have reviews for this preparatory change for adaptive IHOP
> sizing, that factors out the IHOP calculation from G1Collectorpolicy
> into separate classes?
>
> So instead of having this single line at G1CollectorPolicy:944
>
>   944   size_t marking_initiating_used_threshold =
>   945     (_g1->capacity() / 100) * InitiatingHeapOccupancyPercent;
> [...]
>   949   if ((cur_used_bytes + alloc_byte_size) >
> marking_initiating_used_threshold) {
>
> that determines whether marking should start, that decision is
> externalized into a G1IHOPControl class with a
> get_conc_mark_start_threshold() method.
>
> This allows flexible replacement of the IHOP calculation algorithm
> depending on some switch using a factory method.
>
> In addition to that, this change tracks and passes several information
> IHOPControl instances need, like allocation between gcs, mutator time
> from initial mark to the first mixed gc, and others.
>
> The change also adds some test.
>
> It depends on all my recent patches in this area, namely:
> 8140689: Skip last young-only gc if nothing to do in the mixed gc phase
> 8140597: Forcing an initial mark causes G1 to abort mixed collections
> 8140585: PLAB statistics are flushed too late
> 8139874: After G1 Full GC, the next GC is always a young-only GC
> 8138740: Start initial mark right after mixed GC if needed
>
> The problem is that it expects (and verifies) that the GC pauses are
> executed in the "correct" order.
>
> There is no behavioral change in this CR.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8136681
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8136681/webrev/

In g1CollectorPolicy.cpp
  156   _initial_mark_to_mixed()
InitialMarkToMixedTimeTracker has a non-trivial default constructor so 
you don't need to call it explicitly.

  296 G1CollectorPolicy::~G1CollectorPolicy() {
  297   if (_ihop_control != NULL) {
  298     delete _ihop_control;
  299   }
  300 }
There is no need to null check before delete.

  543
  544 uint G1CollectorPolicy::bounded_young_list_target_length(size_t 
rs_lengths, size_t* unbounded_target_length) const {
  545   // Calculate the absolute and desired min bounds.
  546
Would it make sense to split bounded_young_list_target_length into
uint unbounded_young_list_target_length(size_t rs_lengths) const;
uint bound_young_list_target_length(uint unbounded) const;

update_young_list_max_and_target_length could then become

uint unbounded_young = unbounded_young_list_target_length(size_t 
rs_lengths);
_young_list_target_length = bound_young_list_target_length(unbounded_young);
update_max_gclocker_expansion();
return unbounded_young;

this way you could get rid of the output parameter which looks kind of ugly.


  920   // We abort the marking phase.
We skip doing mixed gcs sounds more appropriate here, we've successfully 
completed marking, right?


  987         marking_initiating_used_threshold,
  988         (double) marking_initiating_used_threshold / 
_g1->capacity() * 100,
  989         source);
since G1IHOPControl has an _ihop_percent member, could you add an 
accessor for it and use it in the ergo logging instead of:
(double) marking_initiating_used_threshold / _g1->capacity() * 100,

1038
1039   double app_time_ms = 1.0;
1040
why not just move the calculation of app_time_ms outside the use_stats 
scope as well?


1219   double marking_to_mixed_time = -1.0;
1220   if (!collector_state()->last_gc_was_young() && 
_initial_mark_to_mixed.has_result()) {
1221     marking_to_mixed_time = _initial_mark_to_mixed.last_marking_time();
1222     assert(marking_to_mixed_time > 0.0,
1223            "Initial mark to mixed time must be larger than zero but 
is %.3f",
1224            marking_to_mixed_time);
1225   }
1226   // Only update IHOP information on regular GCs.
1227   if (update_stats) {
1228     update_ihop_statistics(marking_to_mixed_time,

Since you are only setting marking_to_mixed_time at line 1221 -1 is 
passed to update_ihop_statistics fairly regularly.
Would it make sense to break out update_time_to_mixed and move it inside 
the 1220-1225 if-block?



g1CollectorPolicy.hpp
  289   class InitialMarkToMixedTimeTracker {
Would you mind moving this into a separate file?
I would almost prefer if this code was made more general somehow but 
let's skip that for now, but giving it the exposure of its own file 
might make it more discoverable for alternate uses and could trigger 
someone else to reuse the code.

I'm sure a VALUE_OBJ_CLASS_SPEC on the TimeTracker will make someone 
really happy.



Can you file an enhancement to unify G1CollectorPolicy::PauseKind and 
G1YCType/G1YCTypeHelper (g1YCTypes.hpp)


g1IHOPControl.hpp

You never use the forward declared G1Predictions (but I guess it's used 
by adaptive ihop in the next patch?)

g1IHOPControl.cpp

You never use the included g1Predictions.hpp (but I guess it's used by 
adaptive ihop in the next patch?)


   55                 (double) get_conc_mark_start_threshold() / 
_target_occupancy * 100.0,
why not use _ihop_percent here?


/Mikael

> Testing:
> jprt, vm.gc, lots of manual testing, new VM test
>
> Note that SQE will create additional tests.
>
> Thanks,
>    Thomas
>