RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v8]

Fri Nov 3 10:19:10 UTC 2023

On Thu, 2 Nov 2023 15:51:35 GMT, Thomas Schatzl <tschatzl at 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