RFR: 8245203/8245204/8245208: ZGC: Don't hold the ZPageAllocator lock while committing/uncommitting memory

[Per Liden]

Ok, thanks Erik!

/Per

On 5/20/20 9:55 AM, Erik Österlund wrote:
> Hi Per,
> 
> Looks good. Still find the remove iterator rather niche and weird, but 
> I'm okay with it.
> 
> Thanks,
> /Erik
> 
> On 2020-05-19 14:52, Per Liden wrote:
>>
>>
>> On 5/19/20 2:46 PM, Per Liden wrote:
>>> Hi,
>>>
>>> On 5/19/20 11:59 AM, Stefan Karlsson wrote:
>>>> On 2020-05-18 23:23, Per Liden wrote:
>>>>> Please review this series of three patches to rework the page 
>>>>> allocation path so that we don't hold the ZPageAllocator lock while 
>>>>> committing/uncommitting memory. Patch 1 & 2 are small and 
>>>>> preparatory. Patch 3 is the main patch and it's unfortunately 
>>>>> fairly large as it was hard to break up in a sensible way.
>>>>>
>>>> ...
>>>>>
>>>>> 2) 8245204: ZGC: Introduce ZListRemoveIterator
>>>>>
>>>>> Add ZListRemoveIterator, which unlike ZListIterator, iterates over 
>>>>> a non-const "ZList<T>" and each call to "next()" also removes the 
>>>>> returned element from the list.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8245204
>>>>> Webrev: http://cr.openjdk.java.net/~pliden/8245204/webrev.0
>>>>
>>>> I find it a bit odd that the iterator removes an element in the 
>>>> constructor. Would it be possible to get rid of the _next field, and 
>>>> change the next(...) function to do:
>>>>
>>>> if (!_list->is_empty()) {
>>>> *elem = Forward ? _list->remove_first() : _list->remove_last(); 
>>>> return true;
>>>>    }
>>>>
>>>>    return false;
>>>>
>>>> or, since remove_first/last already checks is_empty:
>>>>
>>>> *elem = Forward ? _list->remove_first() : _list->remove_last(); 
>>>> return *elem != NULL;
>>>
>>> You're right. I "blindly" based it on ZListIterator, but as you point 
>>> out, there's no need for a _next field here, so it can be simplified. 
>>> Changed it to:
>>>
>>> diff --git a/src/hotspot/share/gc/z/zList.hpp 
>>> b/src/hotspot/share/gc/z/zList.hpp
>>> --- a/src/hotspot/share/gc/z/zList.hpp
>>> +++ b/src/hotspot/share/gc/z/zList.hpp
>>> @@ -104,7 +104,6 @@
>>>   class ZListRemoveIteratorImpl : public StackObj {
>>>   private:
>>>     ZList<T>* const _list;
>>> -  T*              _next;
>>>
>>>   public:
>>>     ZListRemoveIteratorImpl(ZList<T>* list);
>>> diff --git a/src/hotspot/share/gc/z/zList.inline.hpp 
>>> b/src/hotspot/share/gc/z/zList.inline.hpp
>>> --- a/src/hotspot/share/gc/z/zList.inline.hpp
>>> +++ b/src/hotspot/share/gc/z/zList.inline.hpp
>>> @@ -224,19 +224,12 @@
>>>
>>>   template <typename T, bool Forward>
>>>   inline ZListRemoveIteratorImpl<T, 
>>> Forward>::ZListRemoveIteratorImpl(ZList<T>* list) :
>>> -    _list(list),
>>> -    _next(Forward ? list->remove_first() : list->remove_last()) {}
>>> +    _list(list) {}
>>>
>>>   template <typename T, bool Forward>
>>>   inline bool ZListRemoveIteratorImpl<T, Forward>::next(T** elem) {
>>> -  if (_next != NULL) {
>>> -    *elem = _next;
>>> -    _next = Forward ? _list->remove_first() : _list->remove_last();
>>> -    return true;
>>> -  }
>>> -
>>> -  // No more elements
>>> -  return false;
>>> +  *elem = Forward ? _list->remove_first() : _list->remove_last();
>>> +  return *elem != NULL;
>>>   }
>>>
>>>   template <typename T>
>>>
>>>
>>>>
>>>> One could argue if there's a real need for the iterator. This
>>>>
>>>> +  ZListRemoveIterator<ZPage> iter(&pages);
>>>> +  for (ZPage* page; iter.next(&page);) {
>>>>
>>>> could simply be:
>>>>
>>>> for (ZPage* page; page = pages.remove_first();) {
>>>>
>>>> but I'm fine with an iterator if you like that.
>>>
>>> I think an iterator is kind of nice, but I agree that it's a border 
>>> line case. Unless someone objects, I think I'll keep it for now.
>>
>> Just a side note. Since we typically don't let pointers auto convert 
>> to bool, I'm thinking a for-loop version would look like this:
>>
>>   for (ZPage* page; (page = pages.remove_first()) != NULL;) {
>>
>> or
>>
>>   for (ZPage* page = pages.remove_first(); page != NULL; page = 
>> pages.remove_first()) {
>>
>> which in my view makes it a little bit less attractive, compared to an 
>> iterator.
>>
>> /Per
>>
>>>
>>> Thanks for reviewing!
>>>
>>> cheers,
>>> Per
>>>
>>>>
>>>> Thanks,
>>>> StefanK
>>>>
>