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

[Marcus Larsson]

Hi Thomas, thanks for reviewing!


On 09/16/2014 03:06 PM, Thomas Schatzl wrote:
> 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

Fixed.

> - 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.

Moved them all to the cpp.
Also verified that it compiles without precompiled header.

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

Fixed.

> - 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.

Moved to  HeapRegionManager.hpp/cpp

> - 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
>
>

New webrev:
http://cr.openjdk.java.net/~brutisso/webrev-8058298v2/

Changes from previous webrev:
http://cr.openjdk.java.net/~brutisso/webrev-8058298v1-v2/

Regards,
Marcus