On 9/20/19 23:53, David Holmes wrote:
On 21/09/2019 4:21 pm, serguei.spit...@oracle.com wrote:
Hi David,
I do not want to insist in the area (Threads_lock use) where you have
much more expertise.
It's more conservatism than expertise :) I had a look through and
there are not too many usages of the Threads_lock in the M&M code and
they do appear safe.
I've figured the same.
But I still prefer on principle to not hold nested locks if it is not
necessary.
Simplicity and conservatism rule! :)
Also, I agreed that Daniil's webrev.07 version is correct and less
risky.
The only my concern was about possible performance impact of linear
searches
from the ThreadsList::find_JavaThread_from_java_tid() calls that can
be executed
concurrently with the ThreadTable::lazy_initialize() if the
ThreadsList is big.
But it should not be that big issue.
I think we can waste a lot of time wondering about what kinds of
scenarios perform better or worse with which variant of the code. You
will always be able to create a microbenchmark for which a particular
code pattern works best. So I'd say stick to the simplest and least
risky code and worry about performance issues like this if, or when,
they turn up.
Agreed.
It is my usual approach too (which I've just tried to break). :)
Thanks,
Serguei
So, I'm okay with the webrev.07 modulo ThreadTable renaming.
Okay.
Thanks,
David
Thanks,
Serguei
On 9/20/19 19:20, David Holmes wrote:
Hi Serguei,
On 21/09/2019 9:50 am, serguei.spit...@oracle.com wrote:
Hi Daniil,
Yes, the Threads_lock is still needed around thread->is_exiting()
check and add_thread().
And if we try to use 2 locks, ThreadTableCreate_lock as in your
snippet
and then the nested Threads_lock around thread->is_exiting() and
add_thread(java_tid, thread) lines then it will not work since the
rank
of Threads_lock is higher than the rank of ThreadTableCreate_lock.
The ThreadTableCreate_lock is only used in the lazy_initialize()
function.
It looks safe to adjust the ThreadTableCreate_lock to fix the above
problem.
Then the approach below will work as needed.
void ThreadTable::lazy_initialize(const ThreadsList *threads) {
if (_is_initialized) {
return;
}
MutexLocker ml(ThreadTableCreate_lock);
if (_is_initialized) {
// There is no obvious benefits in allowing the thread table
// being concurrently populated during the initalization.
return;
}
create_table(threads->length());
_is_initialized = true;
for (uint i = 0; i < threads->length(); i++) {
JavaThread* thread = threads->thread_at(i);
oop tobj = thread->threadObj();
if (tobj != NULL) {
jlong java_tid = java_lang_Thread::thread_id(tobj);
MutexLocker ml(Threads_lock);
if (!thread->is_exiting()) {
// Must be inside the lock to ensure that we don't add
the thread to the table
// that has just passed the removal point in
ThreadsSMRSupport::remove_thread()
add_thread(java_tid, thread);
}
}
}
}
I don't like holding a completely unrelated lock, whilst holding the
Threads_lock, particularly as it seems unnecessary for correctness.
We have to be certain that no M&M operation that holds the
Threads_lock can try to initialize the thread table. Testing can
only prove the presence of that problem, not the absence.
I think Daniil's webrev.07 version is correct and less risky than
what you propose. Sorry.
David
-----
