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. But I still prefer on principle to not hold nested locks if
it is not necessary.
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.
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
-----
