On Fri, 3 Nov 2023 12:32:02 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: > > ayang review - renamings + documentation src/hotspot/share/gc/g1/g1Policy.hpp line 275: > 273: double start, > 274: double end, > 275: bool alloocation_failure = false); Typo. src/hotspot/share/gc/g1/g1Policy.hpp line 314: > 312: // Record the start and end of the actual collection part of the > evacuation pause. > 313: void record_young_collection_start(); > 314: void record_young_collection_end(bool > concurrent_operation_is_full_mark, bool alllocation_failure); Typo. src/hotspot/share/gc/g1/g1YoungCollector.cpp line 87: > 85: GCCause::to_string(_pause_cause), > 86: _collector->evacuation_pinned() ? " (Pinned)" : "", > 87: _collector->evacuation_alloc_failed() ? " (Allocation > Failure)" : ""); > GC(6) Pause Young (Normal) (Pinned) (Evacuation Failure) I wonder if the last two can be merged into one `()`, sth like `(Pinned / ...)`, because they are on the same abstraction level. src/hotspot/share/gc/g1/g1_globals.hpp line 327: > 325: range(1, 256) > \ > 326: > \ > 327: product(uint, G1NumCollectionsKeepPinned, 8, DIAGNOSTIC, > \ Any particular reason this is not `EXPERIMENTAL`? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1381627320 PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1381627700 PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1381628625 PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1381624492
