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

[Erik Österlund]

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