RFR: 8076584: Parallelism used for redirty logged cards needs better control.

[Thomas Schatzl]

On Thu, 22 Feb 2024 13:02:13 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

> Please review this change that provides better scaling of the `RedirtyLoggedCardsTask` with high number of worker threads. In the current implementation, the threads  contend for access to the Log Buffers through a single BufferNode resulting in a bottleneck as we increase the threads. The cost per card is pretty low, thus the work distribution overhead dominates the task. 
> 
> The new approach preserves the BufferNodeList states from G1ParScanThreadState, which effectively act as fingers (short cuts) into the list of buffers within `G1RedirtyCardsQueueSet`. We use these  BufferNodeList states to distribute BufferNodes for "redirtying" to the worker threads. By creating multiple points of access to the buffers, this method significantly reduces synchronization overheads and eliminates the bottleneck.
> 
> I have attached results from the Big Ram Tester microbenchmark on a server that spins up 163 worker threads for the `RedirtyLoggedCardsTask`
> 
> ![prepare_mixed](https://github.com/openjdk/jdk/assets/69453999/7120aa59-9b84-4e32-ac58-129ff31bb668)
> ![normal](https://github.com/openjdk/jdk/assets/69453999/7aafb72e-8f18-410d-9f1b-0c0adcd78b03)
> ![mixed](https://github.com/openjdk/jdk/assets/69453999/8a115d9c-56d3-4654-931e-1fa84987915d)
> ![concurrent_start](https://github.com/openjdk/jdk/assets/69453999/783d8804-b7b8-42a7-976c-4c217636157b)
> 
> 
> Testing: Tier 1-3

Change looks good apart from that documentation issue with `G1RedirtyCardsLocalQueueSet::flush()` I think. Please update the old or create a new CR. There is already so much old outdated information in it that makes this PR based on the CR incomprehensible (or the other way around).

src/hotspot/share/gc/g1/g1RedirtyCardsQueue.hpp line 59:

> 57: 
> 58:   // Transfer all completed buffers to the shared qset.
> 59:   void flush(BufferNodeList* buffer_log);

Please update the comment that the given pointer also gets returned the `BufferNodeList` describing the completed buffers as a slice of the shared qset.
Also not completely sure it's common for `flush` to return that and kind of add this here so the naming is not very good imo, but an improved comment should suffice.

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

PR Review: https://git.openjdk.org/jdk/pull/17963#pullrequestreview-1908267786
PR Review Comment: https://git.openjdk.org/jdk/pull/17963#discussion_r1507200764