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

[Thomas Schatzl]

Hi,

On Fri, 2015-11-13 at 16:08 +0100, Mikael Gerdin wrote:
> Hi Thomas,
> 
> On 2015-11-05 10:42, Thomas Schatzl wrote:
[...]
> > 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.
> 

I would prefer to just mention it for completeness.

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

Removed.

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

Fixed, after some off-list discussion.

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

Fixed.

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

I changed the member of G1IHOPControl to _initial_ihop_percent, which
hopefully makes it more clear that with adaptive IHOP,
marking_initiating_used_threshold and the _initial_ihop_percent are not
the same any more.

Actually I would like to go away from using IHOP as value throughout the
code for anything but initialization. A percentage value immediately
begs the question, relative to what?

With adaptive IHOP this needs to be relative to the current heap
capacity, not the total capacity as soon as G1 will start to follow the
current heap capacity.

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

Done.

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

Done.

> g1CollectorPolicy.hpp
>   289   class 	 {
> 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 

I did have something like this but I could not find a good use to it. So
I removed the additional complexity my previous implementation had.

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

Done.

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

Okay. Created JDK-8143041.

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

Yes. I will remove it from this patch.

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

Removed.

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

Done.

New webrevs at:
http://cr.openjdk.java.net/~tschatzl/8136681/webrev.1_to_2/
(incremental)
http://cr.openjdk.java.net/~tschatzl/8136681/webrev.2/ (changes)

The webrev also adds much more documentation to G1IHOPControl, thanks to
Jon for reminding me.

Thanks,
  Thomas