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

Mikael Gerdin mikael.gerdin at oracle.com
Wed Nov 18 15:57:41 UTC 2015


On 2015-11-16 12:23, Thomas Schatzl wrote:
> 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.

Hm, I was thinking more in the lines of

// Only update IHOP information on regular GCs.
if (update_stats) {
   if (!collector_state()->last_gc_was_young() &&
	_initial_mark_to_mixed.has_result()) {
     double marking_to_mixed_time = 
_initial_mark_to_mixed.last_marking_time();
     assert(marking_to_mixed_time > 0.0,
            "Initial mark to mixed time must be larger than zero but is 
%.3f",
            marking_to_mixed_time);
       _ihop_control->update_marking_length(marking_to_mixed_time);
     }

     update_ihop_statistics(app_time_ms / 1000.0,
                            _last_old_allocated_bytes,
                            last_unrestrained_young_length * 
HeapRegion::GrainBytes);
   }
   _last_old_allocated_bytes = 0;

Otherwise a reader might think that marking_to_mixed_time is actually 
set and updated more than once per gc cycle when it's in fact "-1" most 
of the time and thus ignored in update_ihop_statistics.

/Mikael

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



More information about the hotspot-gc-dev mailing list