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

[Bengt Rutisson]

Hi Marcus,

Nice work!

It looks really good to me.

Some minor comments:

heapRegionManager.cpp


  467   if (Atomic::cmpxchg(Claimed, &_claims[region_index], Unclaimed) 
== Unclaimed) {
  468     return true;
  469   }
  470   return false;

To me this is more readable:

uint old_val = Atomic::cmpxchg(Claimed, &_claims[region_index], Unclaimed);
return old_val == Unclaimed;



heapRegionManager.hpp

  254   ~HeapRegionClaimer() {
  255     if (_claims != NULL) {
  256       FREE_C_HEAP_ARRAY(uint, _claims, mtGC);
  257     }
  258   }

Can we move this in to the cpp file to make NEW_C_HEAP_ARRAY and 
FREE_C_HEAP_ARRAY appear in the same file.


Can we introduce a getter in G1CollectedHeap for this instead of adding 
two friend declarations?

G1CollectedHeap::heap()->_hrm._allocated_heapregions_length


Thanks,
Bengt


On 2014-09-30 11:55, Thomas Schatzl wrote:
> Hi Marcus,
>
> On Tue, 2014-09-30 at 11:43 +0200, Marcus Larsson wrote:
>> Hi Thomas,
>>
>>
>> On 09/25/2014 01:08 PM, Thomas Schatzl wrote:
>>> Hi Marcus,
>>>
>>> On Fri, 2014-09-19 at 13:13 +0200, Marcus Larsson wrote:
>>>> Hi Thomas, thanks for reviewing!
>>>>
>>> [...]
>>>> New webrev:
>>>> http://cr.openjdk.java.net/~brutisso/webrev-8058298v2/
>>>>
>>>> Changes from previous webrev:
>>>> http://cr.openjdk.java.net/~brutisso/webrev-8058298v1-v2/
>>>>
>>>     thanks for the changes, look good.
>>>
>>> One final request :) Is it possible to make the HRClaimer of
>>> G1ParRemoveSelfForwardPtrsTask a member of the task like it is done for
>>> all the other tasks? Then we would also get rid of the parameterless
>>> constructor for HeapRegionClaimer.
>> I can do that. This alone will however not allow us to get rid of the
>> parameterless constructor unfortunately.
>> Some tasks still conditionally use the HRClaimer
>> (G1ParVerifyFinalCountTask, G1ParFinalCountTask for example),
>> initializing the HRClaimer in the task constructor only if
>> use_parallel_gc_threads() is true.
>>
>>> I saw that you are also working on "JDK-6979279: remove special-case
>>> code for ParallelGCThreads==0". With that change,
>>> G1CollectedHeap::use_parallel_gc_threads() would be always true,
>>> allowing to make HeapRegionClaimer::initialize() private.
>> Yes, together with that change it would be possible to remove the empty
>> constructor and just keep
>> the initialization code in the other constructor, removing the need for
>> the initialize function completely.
>> I'm however thinking the above patch could include those changes to the
>> HRClaimer, since it is only possible
>> after making those changes anyway.
>>
>> For now, I moved the claimer to the task as you requested.
>>
> Okay, thanks. Looks okay.
>
>> New webrev:
>> http://cr.openjdk.java.net/~mlarsson/8058298/webrev.02/
>>
>> Incremental webrev:
>> http://cr.openjdk.java.net/~mlarsson/8058298/webrev.01-02/
>>
> Thanks,
>    Thomas
>
>