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

[Per Liden]

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
>>