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

[Stefan Karlsson]

On 2020-05-19 14:46, 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.

Looks good.

StefanK

>
> Thanks for reviewing!
>
> cheers,
> Per
>
>>
>> Thanks,
>> StefanK
>>