Re: RFR: 8236073: G1: Use SoftMaxHeapSize to guide GC heuristics
Liang Mao
maoliang.ml at alibaba-inc.com
Tue Feb 11 11:46:21 UTC 2020
Hi Thomas,
>
>> ....
>> size_t G1Policy::minimum_desired_bytes(size_t used_bytes) const {
>> return _ihop_control->unrestrained_young_size() != 0 ?
>> _ihop_control->unrestrained_young_size() :
>> _young_list_max_length * HeapRegion::GrainBytes
>> + _reserve_regions * HeapRegion::GrainBytes + used_bytes;
>> }
> I think G1IHOPControl::_target_occupancy (add a getter) is what you want
> to use here (untested).
I'm not looking for _target_occupancy which is current heap capacity
because the minimum bytes may exceed it. Since the memory
usage is almost at peak in remark,
old_use_bytes + promoted_bytes_in_next_gc + unrestrained_young_bytes
can be the minimum desired bytes.
> There is a cleaned up version of my earlier change that implements the
> latter at http://cr.openjdk.java.net/~tschatzl/8236073/webrev.1/ .
I have a question that heap size can be shrinked even commit size is not
changed so it could cause a waste of committed free regions.
Thanks,
Liang
------------------------------------------------------------------
From:Thomas Schatzl <thomas.schatzl at oracle.com>
Send Time:2020 Feb. 11 (Tue.) 18:42
To:"MAO, Liang" <maoliang.ml at alibaba-inc.com>; hotspot-gc-dev <hotspot-gc-dev at openjdk.java.net>
Subject:Re: RFR: 8236073: G1: Use SoftMaxHeapSize to guide GC heuristics
Hi,
On 10.02.20 12:47, Liang Mao wrote:
> Hi Thomas,
>
> In my testing, I didn't change the value of Min/MaxHeapFreeRatio.
>
> The heap had already shrinked to 5GB but in remark it expand to 6644M.
> The fault value of MinHeapFreeRatio is 40, so the minimal commit size
> after remark is the heap size * 1.67 (3979M * 1.67 = 6644M).
> 1.67 = 100/(100 - 40)
>
>
> [1031.322s][info][gc
> ] GC(741) Pause Young (Concurrent Start) (G1 Evacuation Pause) 4724M->4506M(5120M) 10.607ms
> [1031.322s][info][gc,cpu ] GC(741) User=0.42s Sys=0.00s Real=0.01s
> [1031.322s][info][gc ] GC(742) Concurrent Cycle
> [1031.322s][info][gc,marking ] GC(742) Concurrent Clear Claimed Marks
> [1031.322s][info][gc,marking ] GC(742) Concurrent Clear Claimed Marks 0.066ms
> [1031.322s][info][gc,marking ] GC(742) Concurrent Scan Root Regions
> [1031.322s][info][gc,stringdedup ] Concurrent String Deduplication (1031.322s)
> [1031.323s][info][gc,stringdedup ] Concurrent String Deduplication 14224.0B->0.0B(14224.0B) avg 51.1% (1031.322s, 1031.323s) 0.514ms
> [1031.326s][info][gc,marking ] GC(742) Concurrent Scan Root Regions 3.939ms
> [1031.326s][info][gc,marking ] GC(742) Concurrent Mark (1031.326s)
> [1031.326s][info][gc,marking ] GC(742) Concurrent Mark From Roots
> [1031.326s][info][gc,task ] GC(742) Using 16 workers of 16 for marking
> [1031.483s][info][gc,marking ] GC(742) Concurrent Mark From Roots 157.144ms
> [1031.483s][info][gc,marking ] GC(742) Concurrent Preclean
> [1031.484s][info][gc,marking ] GC(742) Concurrent Preclean 0.404ms
> [1031.484s][info][gc,marking ] GC(742) Concurrent Mark (1031.326s, 1031.484s) 157.587ms
> [1031.485s][info][gc,start ] GC(742) Pause Remark
> [1031.496s][info][gc ] GC(742) Pause Remark 4625M->3979M(6644M) 10.953ms
> [1031.496s][info][gc,cpu ] GC(742) User=0.22s Sys=0.04s Real=0.01s
>
>
> In our production environment, we never use JEP 346 mainly because of
> JDK version.
> So I cannot tell how if it would work. I agree the "idle" issue is not
> our main focus for now.
>
> Using SoftMaxHeapSize to guide adaptive IHOP to make desicion of concurrent
> mark GC cycle can work well with JEP 346 and the resize logic in remark.
> I don't stick to shrink the heap in every GC.
>
> The capacity in resize_heap_if_necessary will be
> Max2(min_desire_capacity_by_MinHeapFreeRatio, Min2(soft_max_capacity(),
> max_desire_capacity_by_MaxHeapFreeRatio))
>
> But both 2 approaches have the problem that default MinHeapFreeRatio is
> too large
> in remark comparing to full gc. As resize_heap_if_necessary
> will keep a minimal heap size as 1.667X of used heap size. After remark,
> the used size could be large that not only include those old regions
> with garbages but
> also the used young regions.
>
> #############################
> void G1CollectedHeap::resize_heap_if_necessary() {
> ...
> const size_t capacity_after_gc = capacity();
> const size_t used_after_gc = capacity_after_gc - unused_committed_regions_in_bytes();
> #############################
>
> The used_after_gc is reasonable for full gc but it can contains young
> regions in remark.
> Do you think it should be changed like this?
> #############################
> const size_t used_after_gc = capacity_after_gc - unused_committed_regions_in_bytes()
> - young_regions_count() * HeapRegion::GrainWords;
> // young_regions_count is 0 after full GC
> #############################
Apart from naming ("used_after_gc") which has been wrong since that
method has been in use for Remark, this seems reasonable.
Maybe "old_used_after_gc"? I think the comments need changes to reflect
that we apply the Min/MaxHeapFreeRatio on the old gen occupancy now
(which is the same as total occupancy after full gc) because it may be
called with young regions active.
I also think the whole code that calculates the expansion and shrinking
amount should be moved to the policy (and g1collectedheap code just
calling that and then only react on the return value), but that can be
done separately.
>
> Besides this, as you suggested, a lower MinHeapFreeRatio would be good.
> But arbitrarily setting a fixed number seems is not a good way that the
> small number may not meet pause time goal in later young GC. I tried to use
> following number in resize_heap_if_necessary:
>
> ##############################
> void G1CollectedHeap::resize_heap_if_necessary() {
> ...
> // We can now safely turn them into size_t's.
> size_t minimum_desired_capacity = (size_t) minimum_desired_capacity_d;
> size_t maximum_desired_capacity = (size_t) maximum_desired_capacity_d;
>
> if (!collector_state()->in_full_gc()) {
> minimum_desired_capacity = MIN2(minimum_desired_capacity, policy()->minimum_desired_bytes(used_after_gc));
> }
That looks a bit hacky... :) But I do not have a better policy for
sizing after full gc either. Did you try always using the
minimum_desired_bytes()?
>
> ....
> size_t G1Policy::minimum_desired_bytes(size_t used_bytes) const {
> return _ihop_control->unrestrained_young_size() != 0 ?
> _ihop_control->unrestrained_young_size() :
> _young_list_max_length * HeapRegion::GrainBytes
> + _reserve_regions * HeapRegion::GrainBytes + used_bytes;
> }
I think G1IHOPControl::_target_occupancy (add a getter) is what you want
to use here (untested).
> #############################
>
> I made the minimum_desired_capacity small enough based on adaptive IHOP's
> _last_unrestrained_young_size. Even without SoftMaxHeapSize, the test can
> keep the memory under 3GB. It's a rough example and I didn't predict the
> promotion bytes of next young gc yet. Do you think
> a proper value of minimum_desired_capacity in remark resize
> +
> G1AdaptiveIHOPControl::actual_target_threshold according to
> soft_max_capacity> is enough?
Yes, both fixing the resizing logic and changing the IHOP target (and
young gen size) according to SoftMaxHeapSize should be sufficient to let
G1 keep that goal without too many commit activity.
The resizing logic change could be handled under JDK-8238686, although
this change does not modify the use of MaxHeapFreeRatio.
There is a cleaned up version of my earlier change that implements the
latter at http://cr.openjdk.java.net/~tschatzl/8236073/webrev.1/ .
I will test your suggested changes and see its impact on our perf suite.
Thanks a lot,
Thomas
P.S: it would be nice to send diffs of suggested changes for easier
application too.
More information about the hotspot-gc-dev
mailing list