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