RFR: 8211718: Supporting multiple concurrent OopStorage iterators

[Erik Osterlund]

Hi Per,

Thanks for the review!

/Erik

> On 5 Oct 2018, at 12:29, Per Liden <per.liden at oracle.com> wrote:
> 
> 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