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
