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