On Tue, 24 Oct 2023 09:56:57 GMT, Thomas Schatzl <[email protected]> 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 addition to something like:
>
> `GC(6) P...
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 444:
> 442: }
> 443:
> 444: if (succeeded) {
Can these two `if`s can be merged into one, `if (succeeded) { return result; }`?
src/hotspot/share/gc/g1/g1EvacFailureRegions.hpp line 36:
> 34: class HeapRegionClaimer;
> 35:
> 36: // This class records for every region on the heap whether it has to be
> retained
I feel the term "retain" has two diff meanings in this PR:
1. retain == pinned or evac-fail
2. should_retain_evac_failed_region
1 is during scavenging while 2 is after scavenging.
src/hotspot/share/gc/g1/g1FullGCPrepareTask.inline.hpp line 82:
> 80: } else {
> 81: assert(hr->containing_set() == nullptr, "already cleared by
> PrepareRegionsClosure");
> 82: if (hr->has_pinned_objects() ||
This `do_heap_region` method is hard to follow; there multiple occurrences of
same predicates. I wonder if one can reorganize these if-else a bit. Inlining
`should_compact` should make all `if` on the same level at least.
src/hotspot/share/gc/g1/g1ParScanThreadState.cpp line 494:
> 492: // undo_allocation() method too.
> 493: undo_allocation(dest_attr, obj_ptr, word_sz, node_index);
> 494: return handle_evacuation_failure_par(old, old_mark, word_sz, true /*
> cause_pinned */);
Why is this `cause_pinned == true`? This obj can be arbitrary, not necessarily
type-array.
src/hotspot/share/gc/g1/heapRegion.cpp line 734:
> 732: // ranges passed in here corresponding to the space between live
> objects, it is
> 733: // possible that there is a pinned object that is not any more
> referenced by
> 734: // Java code (only by native).
Can such obj becomes referenced by java again later on? IOW, a pointer passed
from native to java.
src/hotspot/share/gc/g1/heapRegion.inline.hpp line 262:
> 260: }
> 261:
> 262: inline bool HeapRegion::can_reclaim() const {
I'd suggest inline this method to callers, because "can reclaim" is sth caller
context sensitive.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1374835226
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1375035713
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1375023324
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1375009476
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1375304500
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1375030685
