RFR: 8058298: Separate heap region iterator claim values from the data structures iterated over

[Thomas Schatzl]

Hi Marcus,

On Fri, 2014-09-12 at 14:05 +0200, Marcus Larsson wrote:
> Hi,
> 
> I would like reviews for the following refactorization to separate the 
> heap region claiming from the actual heap region data structure. This 
> allows concurrent tasks to claim heap regions independent of each other.
> 
> The patch adds a HeapRegionClaimer that should be used for parallel 
> iteration over heap regions, and will handle the claims for a certain 
> task. The previous claim field and its associated functions in 
> HeapRegion is removed.
> 
> Webrev:
> http://cr.openjdk.java.net/~brutisso/webrev-8058298/
> 
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8058298
> 
> Testing:
> jprt
> local UTE vm.gc.testlist
> SPECjbb2013

  a few notes:

- there is no Atomic::cmpxchg() for uint, only jint (iirc!). Please
change the type for HeapRegionClaimer::_claims accordingly.

- HeapRegionClaimer should inherit from StackObj

- I think you everything but is_region_claimed() and claim_region()
could be moved to the cpp file without problems. I think even moving
those two too would not matter given that a HeapRegion is a relatively
coarse unit of work.

Actually I was not able to compile a patched VM without moving at least
HeapRegionClaimer::initialize() to a cpp file when using
USE_PRECOMPILED_HEADER=0.

- in HeapRegionClaimer::HeapRegionClaimer(uint), the method body is
indented two spaces too many.

- I would see the HeapRegionClaimer as part of the parallel iteration,
so better located with the HeapRegionManager class. Also, the need for
the number of regions (_allocated_heapregions_length) indicates that.
It does not reference HeapRegion at all too.

- maybe in a future RFE we could make the HeapRegionClaimer a real
iterator. I already like that this change removes a lot of code though.

Looks good otherwise.

Thanks,
  Thomas