RFR: 8211718: Supporting multiple concurrent OopStorage iterators

[Erik Österlund]

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