RFR: 8233697: CHT: Iteration parallelization

[Thomas Schatzl]

On Wed, 19 Oct 2022 10:15:46 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

> Hi,
> 
> Please review this change to add parallel iteration of the ConcurrentHashTable. The iteration should be done during a safepoint without concurrent modifications to the ConcurrentHashTable.
> 
> Usecase is in parallelizing the merging of large remsets for G1.
> 
> Some background: The problem is that particularly during (G1) mixed gc it happens that the distribution of contents in the CHT is very unbalanced - young gen regions have a very small remembered set (little work), and old gen regions very large ones (much work).
> 
> Since the current work distribution is based on whole remembered sets (i.e. CHTs), this makes for a very unbalanced merge remsets phase in G1 when you have quite a bit more than the number of old gen regions threads at your disposal.
> This negatively impacts pause time predictions (and obviously pause times are longer than necessary as many threads are idling to wait for the phase to complete).
> 
> This change only adds the infrastructure code in the CHT, there will be a follow-up with G1 changes.
> 
> Testing: tier 1-3

Changes requested by tschatzl (Reviewer).

src/hotspot/share/utilities/concurrentHashTable.hpp line 499:

> 497:   template <typename SCAN_FUNC>
> 498:   void do_safepoint_scan(SCAN_FUNC& scan_f, BucketsClaimer* bucket_claimer);
> 499: 

Suggestion:

  // Visit all items with SCAN_FUNC without any protection.
  // Thread-safe, but must be called at safepoint.
  class BucketsClaimer;
  template <typename SCAN_FUNC>
  void do_safepoint_scan(SCAN_FUNC& scan_f, BucketsClaimer* bucket_claimer);


This is just a suggestion: maybe put the declaration of `BucketsClaimer` close to the only use.

src/hotspot/share/utilities/concurrentHashTable.inline.hpp line 981:

> 979: }
> 980: 
> 981: 

Superfluous whitespace.

src/hotspot/share/utilities/concurrentHashTable.inline.hpp line 1350:

> 1348:   // The table is split into ranges, every increment is one range.
> 1349:   volatile size_t _next_to_claim;
> 1350:   size_t _claim_size_log2; // Log number of buckets in claimed range.

For naming the constants containing log values (if kept), I would prefer if the existing style to put the `log2` in the front were kept.

src/hotspot/share/utilities/concurrentHashTable.inline.hpp line 1359:

> 1357: 
> 1358: public:
> 1359:   BucketsClaimer(ConcurrentHashTable<CONFIG, F>* cht) :

Would it be possible to add a claim size parameter (with default value `DEFAULT_CLAIM_SIZE_LOG2`)?

Particularly I'm not convinced that the default value is good :)

src/hotspot/share/utilities/concurrentHashTable.inline.hpp line 1370:

> 1368:     size_t size_log2 = _cht->_table->_log2_size;
> 1369:     _claim_size_log2 = MIN2(_claim_size_log2, size_log2);
> 1370:     _limit = (size_t)1 << (size_log2 - _claim_size_log2);

Is it really advantageous to keep these values as log2? It seems to me that just storing the actual non-log values would actually require less work everywhere.
I am fine with restricting claim increments to powers of two (but even that seems artificial), there only seems to be no advantage to use logs anywhere (only that every use needs to do the shift to get the actual value).

src/hotspot/share/utilities/concurrentHashTable.inline.hpp line 1393:

> 1391:         return true;
> 1392:       }
> 1393:     }

Would it be useful/feasible to put this block of code into a helper method?

Something like `claim_from_table(&_next_to_claim, _limit, _claim_size_log2, _cht->get_table(), start, stop, table);`, potentially wrapping the first few parameters into a struct?

Also because the second version seems to be missing the early-out, i.e. some copy&paste oversight.

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

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