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
>
>
>
>
