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

[Bengt Rutisson]

Hi Marcus,

On 2014-10-06 14:17, Marcus Larsson wrote:
> 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/

Looks good.

Thanks,
Bengt


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