Hi David,
I do not want to insist in the area (Threads_lock use) where you have
much more expertise.
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.
So, I'm okay with the webrev.07 modulo ThreadTable renaming.
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
-----
