Hi Serguei, Yes, it is already pushed in the repository [1].
[1] https://hg.openjdk.java.net/jdk/jdk/rev/f4abe950c3b0 Best regards, Daniil On 9/27/19, 10:58 AM, "serguei.spit...@oracle.com" <serguei.spit...@oracle.com> wrote: Hi Daniil, Just notice I did not reply to you. Thank you for the explanation! Have you already pushed this one? Thanks, Serguei On 9/24/19 12:46, Daniil Titov wrote: > 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 > > > > > > > > > > > >
