Thank you, David, Daniel, Serguei, and Robbin, for reviewing this change! Best regards, Daniil
On 9/24/19, 11:45 PM, "Robbin Ehn" <robbin....@oracle.com> wrote: Hi Daniil, Looks good, thanks! /Robbin On 9/25/19 12:45 AM, David Holmes wrote: > Looks good to me. > > Thanks, > David > > On 25/09/2019 2: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 >> >>