RFR: 8296419: [REDO] JDK-8295319: pending_cards_at_gc_start doesn't include cards in thread buffers [v2]

[Thomas Schatzl]

On Tue, 22 Nov 2022 04:02:24 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Let's try this again.  This is the original JDK-8295319 change, plus a couple
>> of small additions to fix problems with that original change.
>> 
>> The description of the original change was:
>> 
>> -----
>> 
>> Please review this change to G1 to include the per-thread buffers in the
>> number of pending cards at the start of a young GC.
>> 
>> DCQS::concatenate_logs has been renamed to concatenate_logs_and_stats, and now
>> also merges the per-thread refinement stats during the thread walk to flush
>> buffers. That replaces the separate thread walk to merge and record these
>> stats earlier in the GC. The merged stats and related info don't seem to be
>> needed until after the buffer flushing.
>> 
>> Also, when abandoning dirty card buffers and stats because of a full GC, fixed
>> to also abandon any buffers in the paused buffers lists.
>> 
>> -----
>> 
>> The problem was that by moving log concatenation earlier, it no longer
>> followed the call to retire_tlabs().  Among other things, that function
>> calls flush_deferred_card_mark_barrier() on each thread, possibly adding cards
>> to the thread's dirty card queue.  Doing that after the log concatenation may
>> lead to unprocessed cards in thread queues, with chaos ensuing.
>> 
>> To fix this, the call to retire_tlabs has also been moved, so that it once
>> again precedes the log concatenation.  Also added (debug-only) verification
>> that all thread dirty card queues are empty at the end of
>> pre_evacuate_collection_set.
>> 
>> A possible followup is to refactor those two operations, which each do a
>> (single-threaded) walk of all threads.  We could combine that into a single
>> walk.  We could also parallelize it if that seems warranted, though the
>> per-thread work is usually pretty small, so might not be worth parallelizing.
>> 
>> There might be an opportunity to do some similar refactoring for fullgc and
>> log abandonment, though there the two operations are done far away from each
>> other.
>> 
>> Testing:
>> mach5 tier1-6
>
> Kim Barrett has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - fix usage of _preserved_marks_set
>  - rearrange pre-evacuation
>  - Merge branch 'master' into cards-in-thread-buffers
>  - retire tlabs before processing thread dcq
>  - verify empty dirty card logs
>  - Revert "8296414: [BACKOUT] JDK-8295319: pending_cards_at_gc_start doesn't include cards in thread buffers"
>    
>    This reverts commit b847fb687735ae5dff56d12d221556a5218b5bba.

One more typo to fix before pushing.

src/hotspot/share/gc/g1/g1YoungCollector.cpp line 506:

> 504: void G1YoungCollector::pre_evacuate_collection_set(G1EvacInfo* evacuation_info) {
> 505:   // Flush early, so later phases don't need to account for per-thread stuff.
> 506:   // Flushes deferred card marks, so must precede concatenting logs.

Suggestion:

  // Flushes deferred card marks, so must precede concatenating logs.

-------------

Marked as reviewed by tschatzl (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11053