Hi Dan,
On 9/20/19 18:11, Daniel D. Daugherty wrote:
On 9/20/19 9:07 PM, Daniel D. Daugherty wrote:
On 9/20/19 7:15 PM, serguei.spit...@oracle.com wrote:
Hi Dan,
Please, find a minor correction below.
On 9/20/19 2:59 PM, Daniel D. Daugherty wrote:
Daniil,
Thanks for sticking with this project through the many versions.
Sorry this review is late...
On 9/19/19 8:30 PM, Daniil Titov wrote:
Hi David and Serguei,
Please review new version of the fix that includes the changes
Serguei suggested:
1. If racing threads initialize the thread table only one of
these threads will populate the table with the threads from the
thread list
2. The code that adds the thread to the tread table is put
inside Threads_lock to ensure that we cannot accidentally add the
thread
that has just passed the removal point in
ThreadsSMRSupport::remove_thread()
The changes are in ThreadTable::lazy_initialize() method only.
Testing: Mach5 tier1, tier2, tier3, tier4, and tier5 tests
successfully passed.
Webrev: https://cr.openjdk.java.net/~dtitov/8185005/webrev.07/
. . .
L93: void ThreadTable::lazy_initialize(const ThreadsList *threads) {
Re: discussion about lazy_initialize() racing with
ThreadsList::find_JavaThread_from_java_tid()
There's a couple of aspects to these two pieces of code racing
with each other and racing with new thread creation. Racing
with
new thread creation is the easy one:
If a new thread isn't added to the ThreadTable by
ThreadsSMRSupport::add_thread() calling
ThreadTable::add_thread(),
then the point in the future where someone calls
find_JavaThread_from_java_tid() will add it to the table
due to
the linear search when ThreadTable::find_thread_by_tid()
returns NULL.
As for multi-threads calling
ThreadsList::find_JavaThread_from_java_tid()
at the same time which results in multi-threads in
lazy_initialize()
at the same time...
- ThreadTable creation will be linear due to
ThreadTableCreate_lock.
After _is_initialized is set to true, then no more callers to
lazy_initialize() will be in the "if (!_is_initialized)"
block.
- Once the ThreadTable is created, then multi-threads can be
executing the for-loop to add their ThreadsList entries to
the ThreadTable.
I guess, there is a confusion here.
The lines 97-101 were recently added into the latest webrev:
93 void ThreadTable::lazy_initialize(const ThreadsList *threads) {
94 if (!_is_initialized) {
95 {
96 MutexLocker ml(ThreadTableCreate_lock);
97 if (_is_initialized) {
98 // There is no obvious benefits in allowing the thread
table
99 // being concurently populated during the initalization.
100 return;
101 }
It prevents multi-threads executing the for-loop to add their
ThreadsList entries to the ThreadTable.
Instead, these threads may not find the requested threads in the
ThreadTable and so, will start linear search in their ThreadsList
to add them into the ThreadTable.
I should have read your comment more carefully...
Yes, you are right about the early cut out on L97-100 causing
the for-loop to be skipped by any thread that saw (!_is_initialized)
and was blocked on the ThreadTableCreate_lock.
So my response below is wrong...
Sorry about the noise.
It is hard to follow all moves on this email thread. :)
Thanks,
Serguei
Dan
