Hi Serguei,
Thank you for reviewing this version of the fix.
Just one question about ThreadIdTable::remove_thread(jlong tid).
What happens if there is no thread with the specified tid in
ThreadIdTable?
Is it possible?
It could be possible when the thread that was started while the
thread table
was initializing exits. At this point the thread table is
initialized and the thread
tries to remove itself from it. Removing non-existing entry from
ConcurrentHashTable
is a correct operation that just leaves the table unchanged.
src/hotspot/share/services/threadIdTable.cpp
233 bool ThreadIdTable::remove_thread(jlong tid) {
234 assert(_is_initialized, "Thread table is not initialized");
235 Thread* thread = Thread::current();
236 ThreadIdTableLookup lookup(tid);
237 return _local_table->remove(thread, lookup);
238 }
src/hotspot/share/utilities/concurrentHashTable.hpp
422 // Returns true if items was deleted matching LOOKUP_FUNC
and
423 // prior to destruction DELETE_FUNC is called.
424 template <typename LOOKUP_FUNC, typename DELETE_FUNC>
425 bool remove(Thread* thread, LOOKUP_FUNC& lookup_f,
DELETE_FUNC& del_f) {
426 return internal_remove(thread, lookup_f, del_f);
427 }
428
429 // Same without DELETE_FUNC.
430 template <typename LOOKUP_FUNC>
431 bool remove(Thread* thread, LOOKUP_FUNC& lookup_f) {
432 return internal_remove(thread, lookup_f, noOp);
433 }
src/hotspot/share/utilities/concurrentHashTable.inline.hpp
446 inline bool ConcurrentHashTable<CONFIG, F>::
447 internal_remove(Thread* thread, LOOKUP_FUNC& lookup_f,
DELETE_FUNC& delete_f)
448 {
449 Bucket* bucket = get_bucket_locked(thread,
lookup_f.get_hash());
450 assert(bucket->is_locked(), "Must be locked.");
451 Node* const volatile * rem_n_prev = bucket->first_ptr();
452 Node* rem_n = bucket->first();
453 bool have_dead = false;
454 while (rem_n != NULL) {
455 if (lookup_f.equals(rem_n->value(), &have_dead)) {
456 bucket->release_assign_node_ptr(rem_n_prev,
rem_n->next());
457 break;
458 } else {
459 rem_n_prev = rem_n->next_ptr();
460 rem_n = rem_n->next();
461 }
462 }
463
464 bucket->unlock();
465
466 if (rem_n == NULL) {
467 return false;
468 }
Best regards,
Daniil
On 9/24/19, 11:35 AM, "serguei.spit...@oracle.com"
<serguei.spit...@oracle.com> wrote:
Hi Daniil,
This version looks good to me.
Thank you for the update!
Just one question about ThreadIdTable::remove_thread(jlong
tid).
What happens if there is no thread with the specified tid in
ThreadIdTable?
Is it possible?
Thanks,
Serguei
On 9/24/19 9:36 AM, Daniil Titov wrote:
> Hi Daniel, David and Serguei,
>
> Please review a new version of the fix (webrev.08) that as
Daniel suggested renames
> ThreadTable to ThreadIdTable (related classes and variables
are renamed as well) and
> corrects formatting issues. There are no other changes in this
webrev.08 comparing
> to the previous version webrev.07.
>
> Testing: Mach5 tier1, tier2, tier3, tier4, and tier5 tests
successfully passed.
>
> Webrev: https://cr.openjdk.java.net/~dtitov/8185005/webrev.08/
> Bug: : https://bugs.openjdk.java.net/browse/JDK-8185005
>
> Thank you!
>
> Best regards,
> Daniil
>
> On 9/20/19, 2:59 PM, "Daniel D. Daugherty"
<daniel.daughe...@oracle.com> 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/
>
> src/hotspot/share/runtime/mutexLocker.hpp
> No comments.
>
> src/hotspot/share/runtime/mutexLocker.cpp
> No comments.
>
> src/hotspot/share/runtime/threadSMR.cpp
> L623: MutexLocker ml(Threads_lock);
> L626: if (!thread->is_exiting()) {
> Re: discussion about is_exiting()
>
> The header comment is pretty clear:
>
> src/hotspot/share/runtime/thread.hpp:
>
> // thread has called JavaThread::exit() or
is terminated
> bool is_exiting() const;
>
> is_exiting() might become true right after you
have called it,
> but its purpose is to ask the question and not
prevent the
> condition from becoming true. As David said, you
should consider
> it an optimization. If you happen to see the
condition is true,
> then you know that the JavaThread isn't going to
be around much
> longer and should act accordingly.
>
> The is_exiting() implementation is:
>
> inline bool JavaThread::is_exiting() const {
> // Use load-acquire so that setting of
_terminated by
> // JavaThread::exit() is seen more quickly.
> TerminatedTypes l_terminated =
(TerminatedTypes)
> OrderAccess::load_acquire((volatile jint *) &_terminated);
> return l_terminated == _thread_exiting ||
> check_is_terminated(l_terminated);
> }
>
> and it depends on the JavaThread's _terminated
field value.
>
> // JavaThread termination support
> enum TerminatedTypes {
> _not_terminated = 0xDEAD - 2,
> _thread_exiting, //
> JavaThread::exit() has been called for this thread
> _thread_terminated, // JavaThread
> is removed from thread list
> _vm_exited // JavaThread
> is still executing native code, but VM is terminated
> // only VM_Exit
> can set _vm_exited
> };
>
> so the JavaThread's _terminated field can get
set to
> _thread_exiting independent of the Threads_lock,
but
> it can't get set to _thread_terminated without the
> Threads_lock.
>
> So by grabbing the Threads_lock on L623, you
make sure
> that ThreadTable::add_thread(java_tid, thread)
does not
> add a JavaThread that's not on the ThreadsList.
It might
> still become is_exiting() == true right after your
>
> L626 if (!thread->is_exiting()) {
>
> but it will still be on the main ThreadsList.
And that
> means that when the JavaThread is removed from
the main
> ThreadsList, you'll still call:
>
> L931: ThreadTable::remove_thread(tid);
>
> L624: // Must be inside the lock to ensure
that we don't
> add the thread to the table
> typo: s/the thread/a thread/
>
> L633: return thread;
> nit - L633 - indented too far (should be 2 spaces)
>
> src/hotspot/share/services/threadTable.hpp
> L42: static void lazy_initialize(const ThreadsList
*threads);
> nit - put space between '*' the variable:
>
> static void lazy_initialize(const ThreadsList*
threads);
>
> like you do in your other decls.
>
> L45: // Lookup and inserts
> Perhaps: // Lookup and list management
>
> L60-61 - nit - please delete these blank lines.
>
> src/hotspot/share/services/threadTable.cpp
> L28: #include "runtime/timerTrace.hpp"
> nit - This should be after threadSMR.hpp...
(alpha sorted order)
>
> L39: static const size_t DefaultThreadTableSizeLog = 8;
> nit - your other 'static const' are not
CamelCase. Why is this one?
>
> L45: static ThreadTableHash* volatile _local_table =
NULL;
> L50: static volatile size_t _current_size = 0;
> L51: static volatile size_t _items_count = 0;
> nit - can you group the file statics together?
(up with L41).
>
> L60: _tid(tid),_java_thread(java_thread) {}
> nit - space after ','
>
> L62 jlong tid() const { return _tid;}
> L63 JavaThread* thread() const {return _java_thread;}
> nit - space before '}'
> nit - space after '{' on L63.
>
> L70: static uintx get_hash(Value const& value,
bool* is_dead) {
> Parameter 'is_dead' is not used.
>
> L74: static void* allocate_node(size_t size,
Value const& value) {
> Parameter 'value' is not used.
>
> 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. There will be a bit of
Threads_lock contention
> as each of the multi-threads tries to add their
entries and
> there will be some wasted work since the
multi-threads will
> likely have similar ThreadLists.
>
> Of course, once _is_initialized is set to true,
then any caller
> to lazy_initialize() will return quickly and
> ThreadsList::find_JavaThread_from_java_tid() will call
> ThreadTable::find_thread_by_tid(). If the target
java_tid isn't
> found, then we do the linear search thing here
and add the
> the entry if we find a match in our current
ThreadsList. Since
> we're only adding the one here, we only contend
for the Threads_lock
> here if we find it.
>
> If ThreadsList::find_JavaThread_from_java_tid()
is called with a
> target java_tid for a JavaThread that was created
after the
> ThreadsList object that the caller has in hand
for the
> find_JavaThread_from_java_tid() call, then, of
course, that
> target 'java_tid' won't be found because the
JavaThread was
> added the main ThreadsList _after_ the
ThreadsList object was
> created by the caller. Of course, you have to ask
where the
> target java_tid value came from since the
JavaThread wasn't
> around when the
ThreadsList::find_JavaThread_from_java_tid()
> call was made with that target java_tid value...
>
> L99: // being concurently populated during
the initalization.
> Typos? Perhaps:
> // to be concurrently populated during
initialization.
>
> But I think those two comment lines are more
appropriate above
> this line:
>
> L96: MutexLocker ml(ThreadTableCreate_lock);
>
> L112: // Must be inside the lock to ensure
that we don't
> add the thread to the table
> typo: s/the thread/a thread/
>
> L141: return ((double)_items_count)/_current_size;
> nit - need spaces around '/'.
>
> L177: bool equals(ThreadTableEntry **value, bool*
is_dead) {
> nit - put space between '**' the variable:
> bool equals(ThreadTableEntry** value,
>
> Parameter 'is_dead' is not used.
>
> L214: while(true) {
> nit - space before '('.
>
>
> 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...
>
>
> > Bug : https://bugs.openjdk.java.net/browse/JDK-8185005
> >
> > Thank you!
> > --Daniil
>
>
>
>
