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.
Dan
The ThreadTableCreate_lock is only held from here:
L95: {
L96: MutexLocker ml(ThreadTableCreate_lock);
to here:
L103: _is_initialized = true;
L104: }
and the for-loop starts here:
L105: for (uint i = 0; i < threads->length(); i++) {
so the ThreadTable_Create_lock does not linearize the
for-loop.
I'm quoting webrev.07... which I believe is the latest.
Dan
. . .
Short version: Thumbs up.
Longer version: I don't think I've spotted anything other than nits
here.
Mostly I've just looked for multi-threaded races, proper usage of the
Thread-SMR stuff, and minimal impact in the case where the new
ThreadsTable is never needed.
Dan
P.S.
ThreadTable is a bit of misnomer. What you really have here is
a ThreadIdTable, but I'm really late to the code review flow
with that comment...
Agreed, ThreadIdTable is better name for this table.
Thanks,
Serguei
Bug : https://bugs.openjdk.java.net/browse/JDK-8185005
Thank you!
--Daniil
