RFR: 8245203/8245204/8245208: ZGC: Don't hold the ZPageAllocator lock while committing/uncommitting memory
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. --------------------- 1) 8245203: ZGC: Don't track size in ZPhysicalMemoryBacking To make it easier to allow for more concurrent commit/uncommit we should not track the backing "size" in ZPhysicalMemoryBacking. Instead we can add the full physical memory range (from 0 to max_capacity) to the list of uncommitted memory at startup. This removes the need to track and update the backing size (which otherwise needs to be properly synchronized). Bug: https://bugs.openjdk.java.net/browse/JDK-8245203 Webrev: http://cr.openjdk.java.net/~pliden/8245203/webrev.0 --------------------- 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 --------------------- 3) 8245208: ZGC: Don't hold the ZPageAllocator lock while committing/uncommitting memory We're currently holding the ZPageAllocator lock while performing a number of expensive operations, such as committing and uncommitting memory. This can have a very negative impact on latency, for example, when a Java thread is trying to allocate a page from the page cache while the ZUncommitter thread is uncommitting a portion of the heap. Bug: https://bugs.openjdk.java.net/browse/JDK-8245208 Webrev: http://cr.openjdk.java.net/~pliden/8245208/webrev.0 --------------------- Testing: More than 10 runs of Tier1-7 on all ZGC supported platforms. Manual testing to provoke various error conditions on Linux. This patchset also solves https://bugs.openjdk.java.net/browse/JDK-8242527 as a side-effect. /Per
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.
---------------------
1) 8245203: ZGC: Don't track size in ZPhysicalMemoryBacking
To make it easier to allow for more concurrent commit/uncommit we should not track the backing "size" in ZPhysicalMemoryBacking. Instead we can add the full physical memory range (from 0 to max_capacity) to the list of uncommitted memory at startup. This removes the need to track and update the backing size (which otherwise needs to be properly synchronized).
Bug: https://bugs.openjdk.java.net/browse/JDK-8245203 Webrev: http://cr.openjdk.java.net/~pliden/8245203/webrev.0
Most of this looks good. You removed the EINTR while loop from the ftruncate call. Is that not needed anymore? Thanks, StefanK
On 5/19/20 11:10 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.
---------------------
1) 8245203: ZGC: Don't track size in ZPhysicalMemoryBacking
To make it easier to allow for more concurrent commit/uncommit we should not track the backing "size" in ZPhysicalMemoryBacking. Instead we can add the full physical memory range (from 0 to max_capacity) to the list of uncommitted memory at startup. This removes the need to track and update the backing size (which otherwise needs to be properly synchronized).
Bug: https://bugs.openjdk.java.net/browse/JDK-8245203 Webrev: http://cr.openjdk.java.net/~pliden/8245203/webrev.0
Most of this looks good. You removed the EINTR while loop from the ftruncate call. Is that not needed anymore?
That was an oversight on my part. Added it back. diff --git a/src/hotspot/os/linux/gc/z/zPhysicalMemoryBacking_linux.cpp b/src/hotspot/os/linux/gc/z/zPhysicalMemoryBacking_linux.cpp --- a/src/hotspot/os/linux/gc/z/zPhysicalMemoryBacking_linux.cpp +++ b/src/hotspot/os/linux/gc/z/zPhysicalMemoryBacking_linux.cpp @@ -126,10 +126,12 @@ } // Truncate backing file - if (ftruncate(_fd, max_capacity) == -1) { - ZErrno err; - log_error(gc)("Failed to truncate backing file (%s)", err.to_string()); - return; + while (ftruncate(_fd, max_capacity) == -1) { + if (errno != EINTR) { + ZErrno err; + log_error(gc)("Failed to truncate backing file (%s)", err.to_string()); + return; + } } // Get filesystem statistics Thanks for reviewing! cheers, Per
On 2020-05-19 14:37, Per Liden wrote:
On 5/19/20 11:10 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.
---------------------
1) 8245203: ZGC: Don't track size in ZPhysicalMemoryBacking
To make it easier to allow for more concurrent commit/uncommit we should not track the backing "size" in ZPhysicalMemoryBacking. Instead we can add the full physical memory range (from 0 to max_capacity) to the list of uncommitted memory at startup. This removes the need to track and update the backing size (which otherwise needs to be properly synchronized).
Bug: https://bugs.openjdk.java.net/browse/JDK-8245203 Webrev: http://cr.openjdk.java.net/~pliden/8245203/webrev.0
Most of this looks good. You removed the EINTR while loop from the ftruncate call. Is that not needed anymore?
That was an oversight on my part. Added it back.
diff --git a/src/hotspot/os/linux/gc/z/zPhysicalMemoryBacking_linux.cpp b/src/hotspot/os/linux/gc/z/zPhysicalMemoryBacking_linux.cpp --- a/src/hotspot/os/linux/gc/z/zPhysicalMemoryBacking_linux.cpp +++ b/src/hotspot/os/linux/gc/z/zPhysicalMemoryBacking_linux.cpp @@ -126,10 +126,12 @@ }
// Truncate backing file - if (ftruncate(_fd, max_capacity) == -1) { - ZErrno err; - log_error(gc)("Failed to truncate backing file (%s)", err.to_string()); - return; + while (ftruncate(_fd, max_capacity) == -1) { + if (errno != EINTR) { + ZErrno err; + log_error(gc)("Failed to truncate backing file (%s)", err.to_string()); + return; + } }
// Get filesystem statistics
Thanks for reviewing!
Looks good. StefanK
cheers, Per
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; 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. Thanks, StefanK
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
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
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
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
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
On 5/18/20 11:23 PM, Per Liden wrote: [...]
3) 8245208: ZGC: Don't hold the ZPageAllocator lock while committing/uncommitting memory
We're currently holding the ZPageAllocator lock while performing a number of expensive operations, such as committing and uncommitting memory. This can have a very negative impact on latency, for example, when a Java thread is trying to allocate a page from the page cache while the ZUncommitter thread is uncommitting a portion of the heap.
Bug: https://bugs.openjdk.java.net/browse/JDK-8245208 Webrev: http://cr.openjdk.java.net/~pliden/8245208/webrev.0
Updated webrev based on feedback received so far. I've split the updates into many smaller ones for easier reviewing. --------------- * Misc. smaller adjustments. http://cr.openjdk.java.net/~pliden/8245208/webrev.1-misc/ * Don't use CollectedHeap::total_collections() in ZPageAllocation. http://cr.openjdk.java.net/~pliden/8245208/webrev.1-total_collections/ * Don't fiddle with MinHeapSize/InitialHeapSize after initialization. http://cr.openjdk.java.net/~pliden/8245208/webrev.1-min_initial_heap_size/ * Introduce "claimed" instead of fiddling with "used" when uncommitting. http://cr.openjdk.java.net/~pliden/8245208/webrev.1-claimed/ * Adjust JFR events. http://cr.openjdk.java.net/~pliden/8245208/webrev.1-events/ * Introduce ZConditionLock. http://cr.openjdk.java.net/~pliden/8245208/webrev.1-conditionlock/ * Restructure the ZUncommitter. http://cr.openjdk.java.net/~pliden/8245208/webrev.1-zuncommitter/ * Introduce ZUnmapper to asynchronous unmap pages (broken out into a separate bug, JDK-8246220) http://cr.openjdk.java.net/~pliden/8246220/webrev.0/ --------------- And finally, here's a combined diff, with all of the above patches: http://cr.openjdk.java.net/~pliden/8245208/webrev.1 --------------- Testing: Multiple runs of Tier1-6, multiple iterations of gc-test-suite. cheers, Per
On 6/1/20 7:32 AM, Per Liden wrote:
On 5/18/20 11:23 PM, Per Liden wrote: [...]
3) 8245208: ZGC: Don't hold the ZPageAllocator lock while committing/uncommitting memory
We're currently holding the ZPageAllocator lock while performing a number of expensive operations, such as committing and uncommitting memory. This can have a very negative impact on latency, for example, when a Java thread is trying to allocate a page from the page cache while the ZUncommitter thread is uncommitting a portion of the heap.
Bug: https://bugs.openjdk.java.net/browse/JDK-8245208 Webrev: http://cr.openjdk.java.net/~pliden/8245208/webrev.0
Another update, after receiving some more comments from Stefan and Erik: * 8245208: ZGC: Don't hold the ZPageAllocator lock while committing/uncommitting memory Full: http://cr.openjdk.java.net/~pliden/8245208/webrev.2/ Diff: http://cr.openjdk.java.net/~pliden/8245208/webrev.2-diff/ * 8246220: ZGC: Introduce ZUnmapper to asynchronous unmap pages Full: http://cr.openjdk.java.net/~pliden/8246220/webrev.1/ Diff: http://cr.openjdk.java.net/~pliden/8246220/webrev.1-diff/ ... and the above patches sits on top of these, which have not been modified in this round: * 8246265: ZGC: Introduce ZConditionLock http://cr.openjdk.java.net/~pliden/8246265/webrev.0/ * 8245204: ZGC: Introduce ZListRemoveIterator http://cr.openjdk.java.net/~pliden/8245204/webrev.0/ * 8245203: ZGC: Don't track size in ZPhysicalMemoryBacking http://cr.openjdk.java.net/~pliden/8245203/webrev.0/ cheers, Per
On 6/1/20 7:06 PM, Per Liden wrote:
On 6/1/20 7:32 AM, Per Liden wrote:
On 5/18/20 11:23 PM, Per Liden wrote: [...]
3) 8245208: ZGC: Don't hold the ZPageAllocator lock while committing/uncommitting memory
We're currently holding the ZPageAllocator lock while performing a number of expensive operations, such as committing and uncommitting memory. This can have a very negative impact on latency, for example, when a Java thread is trying to allocate a page from the page cache while the ZUncommitter thread is uncommitting a portion of the heap.
Bug: https://bugs.openjdk.java.net/browse/JDK-8245208 Webrev: http://cr.openjdk.java.net/~pliden/8245208/webrev.0
Another update, after receiving some more comments from Stefan and Erik:
* 8245208: ZGC: Don't hold the ZPageAllocator lock while committing/uncommitting memory Full: http://cr.openjdk.java.net/~pliden/8245208/webrev.2/ Diff: http://cr.openjdk.java.net/~pliden/8245208/webrev.2-diff/
Just a heads up. I found a nicer way to structure the uncommit_run() function and updated the webrev in-place. cheers, Per
* 8246220: ZGC: Introduce ZUnmapper to asynchronous unmap pages Full: http://cr.openjdk.java.net/~pliden/8246220/webrev.1/ Diff: http://cr.openjdk.java.net/~pliden/8246220/webrev.1-diff/
... and the above patches sits on top of these, which have not been modified in this round:
* 8246265: ZGC: Introduce ZConditionLock http://cr.openjdk.java.net/~pliden/8246265/webrev.0/
* 8245204: ZGC: Introduce ZListRemoveIterator http://cr.openjdk.java.net/~pliden/8245204/webrev.0/
* 8245203: ZGC: Don't track size in ZPhysicalMemoryBacking http://cr.openjdk.java.net/~pliden/8245203/webrev.0/
cheers, Per
Hi, Here are the latest patches for this. Contains more review comments from Stefan, and they have been rebased on today's jdk/jdk. * 8246220: ZGC: Introduce ZUnmapper to asynchronous unmap pages http://cr.openjdk.java.net/~pliden/8246220/webrev.2 * 8245208: ZGC: Don't hold the ZPageAllocator lock while committing/uncommitting memory http://cr.openjdk.java.net/~pliden/8245208/webrev.3 * 8246265: ZGC: Introduce ZConditionLock http://cr.openjdk.java.net/~pliden/8246265/webrev.0/ * (already review) 8245204: ZGC: Introduce ZListRemoveIterator http://cr.openjdk.java.net/~pliden/8245204/webrev.0/ * (already review) 8245203: ZGC: Don't track size in ZPhysicalMemoryBacking http://cr.openjdk.java.net/~pliden/8245203/webrev.0/ And for convenience, here's an all-in-one patch: http://cr.openjdk.java.net/~pliden/zgc/commit_uncommit/all/webrev.1 cheers, Per
Hi Per, Looks good. Thanks, /Erik On 2020-06-05 17:15, Per Liden wrote:
Hi,
Here are the latest patches for this. Contains more review comments from Stefan, and they have been rebased on today's jdk/jdk.
* 8246220: ZGC: Introduce ZUnmapper to asynchronous unmap pages http://cr.openjdk.java.net/~pliden/8246220/webrev.2
* 8245208: ZGC: Don't hold the ZPageAllocator lock while committing/uncommitting memory http://cr.openjdk.java.net/~pliden/8245208/webrev.3
* 8246265: ZGC: Introduce ZConditionLock http://cr.openjdk.java.net/~pliden/8246265/webrev.0/
* (already review) 8245204: ZGC: Introduce ZListRemoveIterator http://cr.openjdk.java.net/~pliden/8245204/webrev.0/
* (already review) 8245203: ZGC: Don't track size in ZPhysicalMemoryBacking http://cr.openjdk.java.net/~pliden/8245203/webrev.0/
And for convenience, here's an all-in-one patch: http://cr.openjdk.java.net/~pliden/zgc/commit_uncommit/all/webrev.1
cheers, Per
Thanks for reviewing, Erik! /Per On 6/9/20 9:14 AM, Erik Österlund wrote:
Hi Per,
Looks good.
Thanks, /Erik
On 2020-06-05 17:15, Per Liden wrote:
Hi,
Here are the latest patches for this. Contains more review comments from Stefan, and they have been rebased on today's jdk/jdk.
* 8246220: ZGC: Introduce ZUnmapper to asynchronous unmap pages http://cr.openjdk.java.net/~pliden/8246220/webrev.2
* 8245208: ZGC: Don't hold the ZPageAllocator lock while committing/uncommitting memory http://cr.openjdk.java.net/~pliden/8245208/webrev.3
* 8246265: ZGC: Introduce ZConditionLock http://cr.openjdk.java.net/~pliden/8246265/webrev.0/
* (already review) 8245204: ZGC: Introduce ZListRemoveIterator http://cr.openjdk.java.net/~pliden/8245204/webrev.0/
* (already review) 8245203: ZGC: Don't track size in ZPhysicalMemoryBacking http://cr.openjdk.java.net/~pliden/8245203/webrev.0/
And for convenience, here's an all-in-one patch: http://cr.openjdk.java.net/~pliden/zgc/commit_uncommit/all/webrev.1
cheers, Per
Looks good. StefanK On 2020-06-09 09:14, Erik Österlund wrote:
Hi Per,
Looks good.
Thanks, /Erik
On 2020-06-05 17:15, Per Liden wrote:
Hi,
Here are the latest patches for this. Contains more review comments from Stefan, and they have been rebased on today's jdk/jdk.
* 8246220: ZGC: Introduce ZUnmapper to asynchronous unmap pages http://cr.openjdk.java.net/~pliden/8246220/webrev.2
* 8245208: ZGC: Don't hold the ZPageAllocator lock while committing/uncommitting memory http://cr.openjdk.java.net/~pliden/8245208/webrev.3
* 8246265: ZGC: Introduce ZConditionLock http://cr.openjdk.java.net/~pliden/8246265/webrev.0/
* (already review) 8245204: ZGC: Introduce ZListRemoveIterator http://cr.openjdk.java.net/~pliden/8245204/webrev.0/
* (already review) 8245203: ZGC: Don't track size in ZPhysicalMemoryBacking http://cr.openjdk.java.net/~pliden/8245203/webrev.0/
And for convenience, here's an all-in-one patch: http://cr.openjdk.java.net/~pliden/zgc/commit_uncommit/all/webrev.1
cheers, Per
Thanks Stefan! /Per On 6/9/20 9:55 AM, Stefan Karlsson wrote:
Looks good.
StefanK
On 2020-06-09 09:14, Erik Österlund wrote:
Hi Per,
Looks good.
Thanks, /Erik
On 2020-06-05 17:15, Per Liden wrote:
Hi,
Here are the latest patches for this. Contains more review comments from Stefan, and they have been rebased on today's jdk/jdk.
* 8246220: ZGC: Introduce ZUnmapper to asynchronous unmap pages http://cr.openjdk.java.net/~pliden/8246220/webrev.2
* 8245208: ZGC: Don't hold the ZPageAllocator lock while committing/uncommitting memory http://cr.openjdk.java.net/~pliden/8245208/webrev.3
* 8246265: ZGC: Introduce ZConditionLock http://cr.openjdk.java.net/~pliden/8246265/webrev.0/
* (already review) 8245204: ZGC: Introduce ZListRemoveIterator http://cr.openjdk.java.net/~pliden/8245204/webrev.0/
* (already review) 8245203: ZGC: Don't track size in ZPhysicalMemoryBacking http://cr.openjdk.java.net/~pliden/8245203/webrev.0/
And for convenience, here's an all-in-one patch: http://cr.openjdk.java.net/~pliden/zgc/commit_uncommit/all/webrev.1
cheers, Per
participants (3)
-
Erik Österlund
-
Per Liden
-
Stefan Karlsson