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

[Per Liden]

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.

Thanks for reviewing!

cheers,
Per

> 
> Thanks,
> StefanK
>