RFR: 8211718: Supporting multiple concurrent OopStorage iterators

[Per Liden]

Looks good!

/Per

On 10/05/2018 12:18 PM, Erik Österlund wrote:
> Hi Per and Kim,
> 
> Thank you both for the reviews. The renaming was applied as suggested 
> (including comments).
> 
> Incremental:
> http://cr.openjdk.java.net/~eosterlund/8211718/webrev.00_01/
> 
> Full:
> http://cr.openjdk.java.net/~eosterlund/8211718/webrev.01/
> 
> Thanks,
> /Erik
> 
> On 2018-10-05 08:18, Per Liden wrote:
>> Hi,
>>
>> On 10/05/2018 01:27 AM, Kim Barrett wrote:
>>>> On Oct 4, 2018, at 9:53 AM, Erik Österlund 
>>>> <erik.osterlund at oracle.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> OopStorage has an artificial restriction of having only one 
>>>> concurrent iterator at a time. This restriction should be removed.
>>>>
>>>> The solution is to turn the bool that tracks concurrent iterations, 
>>>> into an int.
>>>>
>>>> For some background, the reason for the restriction has historic 
>>>> reasons, from a time when opening OopStorage for concurrent 
>>>> iteration was done using a lock-free protocol between GC worker 
>>>> threads, which made it tricky. Since then, the model was simplified 
>>>> to just let the ParState RAII object open and close the concurrent 
>>>> iterations. With the simplified model, there is no need for such a 
>>>> restriction.
>>>>
>>>> I talked to Kim, and long-term there are more plans in this area, 
>>>> such as removing weak_oops_do, and then unifying the iterators. But 
>>>> for now, I would like to just stop disallowing multiple concurrent 
>>>> iterations as one small step along the way.
>>>>
>>>> I updated the relevant OopStorage documentation to reflect the 
>>>> removal of this restriction.
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8211718/webrev.00/
>>
>> Looks good.
>>
>>>
>>> Looks good, so far as it goes.  However, maybe some renames are in 
>>> order:
>>>
>>> _concurrent_iteration_active => _concurrent_iteration_state
>>> or
>>> _concurrent_iteration_active => _concurrent_iteration_count
>>> update_iteration_state => update_[concurrent_]iteration_count
>>>
>>> And perhaps an assertion in the update function that it didn’t get 
>>> updated to negative.
>>
>> I agree, _concurrent_iteration_count and 
>> update_concurrent_iteration_count sounds like good new names.
>>
>> cheers,
>> Per
>