RFR: 8212748: ZGC: Add reentrant locking functionality
Hi, ZGC needs a per-nmethod lock to be used for concurrent IC cleaning, protecting misaligned oops from concurrently being patched by nmethod entry barriers, and read using CompiledMethod::is_unloading(), with interactions crossing JavaThreads and non-Java threads. This patch adds that utility. Webrev: http://cr.openjdk.java.net/~eosterlund/8212748/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8212748 Thanks, /Erik
Hi Erik, On 2018-10-25 15:44, Erik Österlund wrote:
Hi,
ZGC needs a per-nmethod lock to be used for concurrent IC cleaning, protecting misaligned oops from concurrently being patched by nmethod entry barriers, and read using CompiledMethod::is_unloading(), with interactions crossing JavaThreads and non-Java threads. This patch adds that utility.
Webrev: http://cr.openjdk.java.net/~eosterlund/8212748/webrev.00/
I'd like to suggest that ZReentrantLock doens't inherit from ZLock, but that it instead has a ZLock. And that ZLocker is adjusted to take a T* instead of a ZLock*. Also, I'm not sure I see the need for the reentrant_lock() function. Shouldn't we just have a lock counter and let lock/unlock do the right thing? cheers, Per
Bug: https://bugs.openjdk.java.net/browse/JDK-8212748
Thanks, /Erik
Hi Per, Thanks for having a look at this. On 2018-10-25 17:27, Per Liden wrote:
Hi Erik,
On 2018-10-25 15:44, Erik Österlund wrote:
Hi,
ZGC needs a per-nmethod lock to be used for concurrent IC cleaning, protecting misaligned oops from concurrently being patched by nmethod entry barriers, and read using CompiledMethod::is_unloading(), with interactions crossing JavaThreads and non-Java threads. This patch adds that utility.
Webrev: http://cr.openjdk.java.net/~eosterlund/8212748/webrev.00/
I'd like to suggest that ZReentrantLock doens't inherit from ZLock, but that it instead has a ZLock. And that ZLocker is adjusted to take a T* instead of a ZLock*.
Wouldn't it be annoying that you can't use a plain ZLocker then, but instead would have to use ZLocker<ZLock>? I'm not sure I see the advantage of that approach, only a usability disadvantage. I'm not opposed either though if you prefer it to be done in that way.
Also, I'm not sure I see the need for the reentrant_lock() function. Shouldn't we just have a lock counter and let lock/unlock do the right thing?
That is a possibility indeed. However, there are contexts where the lock is not (and must not be) taken in a reentrant way, and it is an important assumption that it is in fact not reentrant, and I want it to not work if that assumption is wrong. And I wanted that to be the default and be the behaviour when using the ZLocker. Plus given that motivation, I also don't know if I want the extra counter state that seems unnecessary for the contexts where reentrancy is handled. Again, I'm not opposed to what you are suggesting, just trying to weigh the pros and cons. What do you think? Thanks /Erik
cheers, Per
Bug: https://bugs.openjdk.java.net/browse/JDK-8212748
Thanks, /Erik
On Oct 25, 2018, at 12:20 PM, Erik Österlund <erik.osterlund@oracle.com> wrote:
Hi Per,
Thanks for having a look at this.
On 2018-10-25 17:27, Per Liden wrote:
Hi Erik, On 2018-10-25 15:44, Erik Österlund wrote:
Hi,
ZGC needs a per-nmethod lock to be used for concurrent IC cleaning, protecting misaligned oops from concurrently being patched by nmethod entry barriers, and read using CompiledMethod::is_unloading(), with interactions crossing JavaThreads and non-Java threads. This patch adds that utility.
Webrev: http://cr.openjdk.java.net/~eosterlund/8212748/webrev.00/ I'd like to suggest that ZReentrantLock doens't inherit from ZLock, but that it instead has a ZLock. And that ZLocker is adjusted to take a T* instead of a ZLock*.
Wouldn't it be annoying that you can't use a plain ZLocker then, but instead would have to use ZLocker<ZLock>? I'm not sure I see the advantage of that approach, only a usability disadvantage. I'm not opposed either though if you prefer it to be done in that way.
For what its worth, that’s exactly how the similar C++11 facility works, e.g. one says std::lock_guard<std::mutex> lk(m); or std::lock_guard<std::recursive_mutex> lk(rm);
Hi, On 10/26/18 2:00 AM, Kim Barrett wrote:
On Oct 25, 2018, at 12:20 PM, Erik Österlund <erik.osterlund@oracle.com> wrote:
Hi Per,
Thanks for having a look at this.
On 2018-10-25 17:27, Per Liden wrote:
Hi Erik, On 2018-10-25 15:44, Erik Österlund wrote:
Hi,
ZGC needs a per-nmethod lock to be used for concurrent IC cleaning, protecting misaligned oops from concurrently being patched by nmethod entry barriers, and read using CompiledMethod::is_unloading(), with interactions crossing JavaThreads and non-Java threads. This patch adds that utility.
Webrev: http://cr.openjdk.java.net/~eosterlund/8212748/webrev.00/ I'd like to suggest that ZReentrantLock doens't inherit from ZLock, but that it instead has a ZLock. And that ZLocker is adjusted to take a T* instead of a ZLock*.
Wouldn't it be annoying that you can't use a plain ZLocker then, but instead would have to use ZLocker<ZLock>? I'm not sure I see the advantage of that approach, only a usability disadvantage. I'm not opposed either though if you prefer it to be done in that way.
For what its worth, that’s exactly how the similar C++11 facility works, e.g. one says
std::lock_guard<std::mutex> lk(m); or std::lock_guard<std::recursive_mutex> lk(rm);
Erik and I discussed this some more and we ended up with the following patch: http://cr.openjdk.java.net/~pliden/8212748/webrev.0/ cheers, Per
Hi Per, Looks good. Thanks, /Erik On 2018-11-20 08:45, Per Liden wrote:
Hi,
On 10/26/18 2:00 AM, Kim Barrett wrote:
On Oct 25, 2018, at 12:20 PM, Erik Österlund <erik.osterlund@oracle.com> wrote:
Hi Per,
Thanks for having a look at this.
On 2018-10-25 17:27, Per Liden wrote:
Hi Erik, On 2018-10-25 15:44, Erik Österlund wrote:
Hi,
ZGC needs a per-nmethod lock to be used for concurrent IC cleaning, protecting misaligned oops from concurrently being patched by nmethod entry barriers, and read using CompiledMethod::is_unloading(), with interactions crossing JavaThreads and non-Java threads. This patch adds that utility.
Webrev: http://cr.openjdk.java.net/~eosterlund/8212748/webrev.00/ I'd like to suggest that ZReentrantLock doens't inherit from ZLock, but that it instead has a ZLock. And that ZLocker is adjusted to take a T* instead of a ZLock*.
Wouldn't it be annoying that you can't use a plain ZLocker then, but instead would have to use ZLocker<ZLock>? I'm not sure I see the advantage of that approach, only a usability disadvantage. I'm not opposed either though if you prefer it to be done in that way.
For what its worth, that’s exactly how the similar C++11 facility works, e.g. one says
std::lock_guard<std::mutex> lk(m); or std::lock_guard<std::recursive_mutex> lk(rm);
Erik and I discussed this some more and we ended up with the following patch:
http://cr.openjdk.java.net/~pliden/8212748/webrev.0/
cheers, Per
On Nov 20, 2018, at 2:45 AM, Per Liden <per.liden@oracle.com> wrote:
Hi,
On 10/26/18 2:00 AM, Kim Barrett wrote:
On Oct 25, 2018, at 12:20 PM, Erik Österlund <erik.osterlund@oracle.com> wrote:
Hi Per,
Thanks for having a look at this.
On 2018-10-25 17:27, Per Liden wrote:
Hi Erik, On 2018-10-25 15:44, Erik Österlund wrote:
Hi,
ZGC needs a per-nmethod lock to be used for concurrent IC cleaning, protecting misaligned oops from concurrently being patched by nmethod entry barriers, and read using CompiledMethod::is_unloading(), with interactions crossing JavaThreads and non-Java threads. This patch adds that utility.
Webrev: http://cr.openjdk.java.net/~eosterlund/8212748/webrev.00/ I'd like to suggest that ZReentrantLock doens't inherit from ZLock, but that it instead has a ZLock. And that ZLocker is adjusted to take a T* instead of a ZLock*.
Wouldn't it be annoying that you can't use a plain ZLocker then, but instead would have to use ZLocker<ZLock>? I'm not sure I see the advantage of that approach, only a usability disadvantage. I'm not opposed either though if you prefer it to be done in that way. For what its worth, that’s exactly how the similar C++11 facility works, e.g. one says std::lock_guard<std::mutex> lk(m); or std::lock_guard<std::recursive_mutex> lk(rm);
Erik and I discussed this some more and we ended up with the following patch:
http://cr.openjdk.java.net/~pliden/8212748/webrev.0/
cheers, Per
------------------------------------------------------------------------------ Is there a reason why this was not based on a pthread_mutex_t configured with the PTHREAD_MUTEX_RECURSIVE attribute? (Rather than this "hand-rolled" recursion.) ------------------------------------------------------------------------------ What is RecursiveZLock::is_owned for? That might be why the RECURSIVE attribute wasn't used, but since I don't see the point, except perhaps for assertion checks. Part of my question is, if is_owned is desirable for recursive case, why not for ordinary? A similar implementation would seem to suffice. ------------------------------------------------------------------------------
Hi Kim, On 11/20/18 9:37 PM, Kim Barrett wrote:
On Nov 20, 2018, at 2:45 AM, Per Liden <per.liden@oracle.com> wrote:
Hi,
On 10/26/18 2:00 AM, Kim Barrett wrote:
On Oct 25, 2018, at 12:20 PM, Erik Österlund <erik.osterlund@oracle.com> wrote:
Hi Per,
Thanks for having a look at this.
On 2018-10-25 17:27, Per Liden wrote:
Hi Erik, On 2018-10-25 15:44, Erik Österlund wrote:
Hi,
ZGC needs a per-nmethod lock to be used for concurrent IC cleaning, protecting misaligned oops from concurrently being patched by nmethod entry barriers, and read using CompiledMethod::is_unloading(), with interactions crossing JavaThreads and non-Java threads. This patch adds that utility.
Webrev: http://cr.openjdk.java.net/~eosterlund/8212748/webrev.00/ I'd like to suggest that ZReentrantLock doens't inherit from ZLock, but that it instead has a ZLock. And that ZLocker is adjusted to take a T* instead of a ZLock*.
Wouldn't it be annoying that you can't use a plain ZLocker then, but instead would have to use ZLocker<ZLock>? I'm not sure I see the advantage of that approach, only a usability disadvantage. I'm not opposed either though if you prefer it to be done in that way. For what its worth, that’s exactly how the similar C++11 facility works, e.g. one says std::lock_guard<std::mutex> lk(m); or std::lock_guard<std::recursive_mutex> lk(rm);
Erik and I discussed this some more and we ended up with the following patch:
http://cr.openjdk.java.net/~pliden/8212748/webrev.0/
cheers, Per
------------------------------------------------------------------------------
Is there a reason why this was not based on a pthread_mutex_t configured with the PTHREAD_MUTEX_RECURSIVE attribute? (Rather than this "hand-rolled" recursion.)
------------------------------------------------------------------------------
What is RecursiveZLock::is_owned for? That might be why the RECURSIVE attribute wasn't used, but since I don't see the point, except perhaps for assertion checks. Part of my question is, if is_owned is desirable for recursive case, why not for ordinary? A similar implementation would seem to suffice.
Yes, is_owned() is used in some asserts, but more importantly it's used in few other cases too. The locking around concurrent class unloading is a bit messy, but in some contexts (which has multiple entry points) is_owned() is used to figure out if it's safe to do some operations because the thread has the lock, or if that should be postponed. cheers, Per
------------------------------------------------------------------------------
On Nov 21, 2018, at 5:18 AM, Per Liden <per.liden@oracle.com> wrote:
Hi Kim,
On 11/20/18 9:37 PM, Kim Barrett wrote:
Erik and I discussed this some more and we ended up with the following patch:
http://cr.openjdk.java.net/~pliden/8212748/webrev.0/
cheers, Per
------------------------------------------------------------------------------ Is there a reason why this was not based on a pthread_mutex_t configured with the PTHREAD_MUTEX_RECURSIVE attribute? (Rather than this "hand-rolled" recursion.) ------------------------------------------------------------------------------ What is RecursiveZLock::is_owned for? That might be why the RECURSIVE attribute wasn't used, but since I don't see the point, except perhaps for assertion checks. Part of my question is, if is_owned is desirable for recursive case, why not for ordinary? A similar implementation would seem to suffice.
Yes, is_owned() is used in some asserts, but more importantly it's used in few other cases too. The locking around concurrent class unloading is a bit messy, but in some contexts (which has multiple entry points) is_owned() is used to figure out if it's safe to do some operations because the thread has the lock, or if that should be postponed.
That sounds rather icky. Change looks good.
On 2018-11-21 14:45, Kim Barrett wrote: [...]
What is RecursiveZLock::is_owned for? That might be why the RECURSIVE attribute wasn't used, but since I don't see the point, except perhaps for assertion checks. Part of my question is, if is_owned is desirable for recursive case, why not for ordinary? A similar implementation would seem to suffice.
Yes, is_owned() is used in some asserts, but more importantly it's used in few other cases too. The locking around concurrent class unloading is a bit messy, but in some contexts (which has multiple entry points) is_owned() is used to figure out if it's safe to do some operations because the thread has the lock, or if that should be postponed.
That sounds rather icky.
Yes, the locking issues related to class unloading is super-icky. I hope we'll eventually get to a point where all that icky-ness goes away (and hopefully ZReentrantLock can also go away). But it might take some time to get there.
Change looks good.
Thanks for reviewing. cheers, Per
participants (3)
-
Erik Österlund
-
Kim Barrett
-
Per Liden