On Thu, 2 Nov 2023 15:51:35 GMT, Thomas Schatzl <tscha...@openjdk.org> wrote:
>> The JEP covers the idea very well, so I'm only covering some implementation
>> details here:
>>
>> * regions get a "pin count" (reference count). As long as it is non-zero, we
>> conservatively never reclaim that region even if there is no reference in
>> there. JNI code might have references to it.
>>
>> * the JNI spec only requires us to provide pinning support for typeArrays,
>> nothing else. This implementation uses this in various ways:
>>
>> * when evacuating from a pinned region, we evacuate everything live but
>> the typeArrays to get more empty regions to clean up later.
>>
>> * when formatting dead space within pinned regions we use filler objects.
>> Pinned regions may be referenced by JNI code only, so we can't overwrite
>> contents of any dead typeArray either. These dead but referenced typeArrays
>> luckily have the same header size of our filler objects, so we can use their
>> headers for our fillers. The problem is that previously there has been that
>> restriction that filler objects are half a region size at most, so we can
>> end up with the need for placing a filler object header inside a typeArray.
>> The code could be clever and handle this situation by splitting the to be
>> filled area so that this can't happen, but the solution taken here is
>> allowing filler arrays to cover a whole region. They are not referenced by
>> Java code anyway, so there is no harm in doing so (i.e. gc code never
>> touches them anyway).
>>
>> * G1 currently only ever actually evacuates young pinned regions. Old pinned
>> regions of any kind are never put into the collection set and automatically
>> skipped. However assuming that the pinning is of short length, we put them
>> into the candidates when we can.
>>
>> * there is the problem that if an applications pins a region for a long
>> time g1 will skip evacuating that region over and over. that may lead to
>> issues with the current policy in marking regions (only exit mixed phase
>> when there are no marking candidates) and just waste of processing time
>> (when the candidate stays in the retained candidates)
>>
>> The cop-out chosen here is to "age out" the regions from the candidates
>> and wait until the next marking happens.
>>
>> I.e. pinned marking candidates are immediately moved to retained
>> candidates, and if in total the region has been pinned for
>> `G1NumCollectionsKeepUnreclaimable` collections it is dropped from the
>> candidates. Its current value is fairly random.
>>
>> * G1 pauses got a new tag if there were pinned regions in the collection
>> set. I.e. in a...
>
> Thomas Schatzl has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add documentation about why and how we handle pinned regions in the
> young/old generation.
src/hotspot/share/gc/g1/g1FullGCPrepareTask.inline.hpp line 76:
> 74: }
> 75:
> 76: inline bool
> G1DetermineCompactionQueueClosure::has_pinned_objects(HeapRegion* hr) const {
Could this be a static-local function so that it doesn't appear in the header
file? (Its name is the same as the public API in heap-region.)
src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp line 482:
> 480: }
> 481:
> 482: double G1GCPhaseTimes::print_post_evacuate_collection_set(bool
> evacuation_retained) const {
Why the renaming here?
src/hotspot/share/gc/g1/g1GCPhaseTimes.hpp line 150:
> 148:
> 149: enum RestoreRetainedRegionsWorkItems {
> 150: RestoreRetainedRegionsEvacFailedNum, // How many regions
> experienced an evacuation failure (pinned or allocation failure)
Kind of a preexisting issue. "retained" here seems to mean evac-fail, not "kept
in retain-list".
src/hotspot/share/gc/g1/g1HeapRegionAttr.hpp line 43:
> 41: remset_is_tracked_t _remset_is_tracked;
> 42: region_type_t _type;
> 43: bool _is_pinned;
Maybe `uint8_t` as documented above?
src/hotspot/share/gc/g1/g1Policy.cpp line 547:
> 545: }
> 546:
> 547: log_trace(gc, ergo, heap)("Selected %u of %u retained candidates
> (unreclaimable %u) taking %1.3fms additional time",
I actually think calling it "pinned", instead of "unreclaimable", is more
informative (to users/dev). (And other places when it is shown in logs.)
src/hotspot/share/gc/g1/g1YoungCollector.cpp line 1102:
> 1100:
> jtm.report_pause_type(collector_state()->young_gc_pause_type(_concurrent_operation_is_full_mark));
> 1101:
> 1102:
> policy()->record_young_collection_end(_concurrent_operation_is_full_mark,
> evacuation_alloc_failed());
The arg name (where this method is defined) should be updated to sth like
`evac_alloc_failed` from `evacuation_failure`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1381415671
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1381418678
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1381440184
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1381419525
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1381423626
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1381433739
