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

[Marcus Larsson]

Hi,

Updated the change according to Bengt's feedback, but left the friend 
declarations in place. Also, since ParallelGCThreads is not allowed to 
be 0 any longer [1], I simplified the claimer and removed the initialize 
function and the empty constructor.

New webrev:
http://cr.openjdk.java.net/~mlarsson/8058298/webrev.03/

Incremental:
http://cr.openjdk.java.net/~mlarsson/8058298/webrev.02-03/

Thanks,
Marcus

[1]: https://bugs.openjdk.java.net/browse/JDK-8059527


On 06/10/14 13:37, Bengt Rutisson wrote:
>
> Hi Thomas,
>
>
> On 2014-10-06 13:26, Thomas Schatzl wrote:
>> Hi,
>>
>> On Mon, 2014-10-06 at 13:08 +0200, Bengt Rutisson wrote:
>>> Hi Marcus,
>>>
>>> Nice work!
>>>
>>> It looks really good to me.
>>>
>>> Some minor comments:
>>>
>>> heapRegionManager.cpp
>>>
>> [...]
>>> Can we introduce a getter in G1CollectedHeap for this instead of adding
>>> two friend declarations?
>>>
>>> G1CollectedHeap::heap()->_hrm._allocated_heapregions_length
>> I somewhat prefer the friend declaration:
>> _hrm._allocated_heapregions_length is somewhat of a hack and is
>> preferably not available from outside, because potentially people start
>> using it for iteration over the heap again then ;)
>>
>> What could be done to hide this is to create a factory method for the
>> HeapRegionClaimer in HeapRegionManager, or pass the HeapRegionClaimer
>> instance to a HeapRegionManager instance for initialization.
>
> I discussed this a bit with Marcus. I think creating a factory for the
> claimer or passing it to the manager for initialization seems like a
> step in the right direction. However, I think that would make it even
> more obvious that you really want an iterator and not just a claimer. ;)
>
> After some discussion I'm fine with keeping the current friend
> declarations. If we decide to introduce iterators we can clean this up
> then. It is probably not worth going half the way right now.
>
> Thanks,
> Bengt
>
>>
>> Thanks,
>>    Thomas
>>
>>
>