RFR(S/M): 8191789 - migrate more Thread-SMR stuff from thread.[ch]pp -> threadSMR.[ch]pp

[Daniel D. Daugherty]

Stefan,

Thanks for the fast review!

Replies embedded below...

On 12/5/17 10:07 AM, Stefan Karlsson wrote:
> On 2017-12-04 21:22, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> In terms of actual "new/changed" code, this is a "S" review. However,
>> because of code motion, the changed/insert/delete counts are the size
>> of an "M" or "L" review.
>>
>> Stefan K, this is one of your Thread-SMR follow-up suggestions so I need
>> to hear from you on this thread. Thanks!
>
> Seems like a good step to me.

Thanks! We discussed this during the Thread-SMR project, but didn't
do it before going out for OpenJDK review. It was your comment that
motivated us to finally do it... :-)


> ========================================================================
> http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-0/src/hotspot/share/runtime/threadSMR.hpp.udiff.html 
>
>
> Now that you have moved all these functions and variables to a class 
> named ThreadsSMRSupport maybe you want to consider getting rid of the 
> redundant _smr_ prefix/infix?

I thought about that, but it made the sanity checking phase much more
difficult because of the noise level in the diffs. What I could do is
make that change as the last patch before qfold and qfinish...


> Preexisting: I think you missed moving this in your previous cleanup:
>
> +  static bool is_a_protected_JavaThread_with_lock(JavaThread *thread) {
> +    MutexLockerEx ml(Threads_lock->owned_by_self() ? NULL : 
> Threads_lock);
> +    return is_a_protected_JavaThread(thread);
> +  }

Sorry, I don't know what you mean here. 
is_a_protected_JavaThread_with_lock()
was in the Threads class and now is in the ThreadsSMRSupport class... What
kind of move did I miss?


> ========================================================================
> http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-0/src/hotspot/share/runtime/thread.cpp.frames.html 
>
>
> Would it make sense to move the following code sections into functions 
> in ThreadsSMRSupport? That way we could minimize the number of public 
> functions exposed from ThreadSMRSupport.
>
> ---
> 4369   // Maintain fast thread list
> 4370   ThreadsList *new_list = 
> ThreadsList::add_thread(ThreadsSMRSupport::get_smr_java_thread_list(), 
> p);
> 4371   if (EnableThreadSMRStatistics) {
> 4372     ThreadsSMRSupport::inc_smr_java_thread_list_alloc_cnt();
> 4373 
> ThreadsSMRSupport::update_smr_java_thread_list_max(new_list->length());
> 4374   }
> 4375   // Initial _smr_java_thread_list will not generate a 
> "Threads::add" mesg.
> 4376   log_debug(thread, smr)("tid=" UINTX_FORMAT ": Threads::add: new 
> ThreadsList=" INTPTR_FORMAT, os::current_thread_id(), p2i(new_list));
> 4377
> 4378   ThreadsList *old_list = 
> ThreadsSMRSupport::xchg_smr_java_thread_list(new_list);
> 4379   ThreadsSMRSupport::smr_free_list(old_list);

Sure. We could refactor this into ThreadsSMRSupport::add_thread()...


> ---
> 4396     // Maintain fast thread list
> 4397     ThreadsList *new_list = 
> ThreadsList::remove_thread(ThreadsSMRSupport::get_smr_java_thread_list(), 
> p);
> 4398     if (EnableThreadSMRStatistics) {
> 4399 ThreadsSMRSupport::inc_smr_java_thread_list_alloc_cnt();
> 4400       // This list is smaller so no need to check for a "longest" 
> update.
> 4401     }
> 4402
> 4403     // Final _smr_java_thread_list will not generate a 
> "Threads::remove" mesg.
> 4404     log_debug(thread, smr)("tid=" UINTX_FORMAT ": 
> Threads::remove: new ThreadsList=" INTPTR_FORMAT, 
> os::current_thread_id(), p2i(new_list));
> 4405
> 4406     ThreadsList *old_list = 
> ThreadsSMRSupport::xchg_smr_java_thread_list(new_list);
> 4407     ThreadsSMRSupport::smr_free_list(old_list);

And we could refactor this into ThreadsSMRSupport::remove_thread()...

That would lighten the Thread-SMR footprint in thread.cpp even more...

Dan


>
> Thanks,
> StefanK
>
>>
>> I have a simple (but tedious) cleanup fix for Thread-SMR. The bug is:
>>
>>      JDK-8191789 migrate more Thread-SMR stuff from thread.[ch]pp -> 
>> threadSMR.[ch]pp
>>      https://bugs.openjdk.java.net/browse/JDK-8191789
>>
>> This fix is mostly code motion:
>>
>> - move Thread-SMR related code from thread.cpp -> threadSMR.cpp and
>>    from thread.hpp -> threadSMR.hpp
>>    - move Threads::_smr_* fields -> ThreadsSMRSupport class
>>    - move Thread-SMR functions from Threads -> ThreadsSMRSupport class
>>    - move Thread-SMR helper classes from thread.cpp -> threadsSMR.cpp
>>    - rename a bunch of Threads::foo() calls -> ThreadsSMRSupport::foo()
>>
>> - collateral changes:
>>    - DO_JAVA_THREADS macro usage by code moved to threadSMR.cpp have
>>      to switch to JavaThreadIterator or some other function.
>>    - Add ThreadsSMRSupport::inc_smr_java_thread_list_alloc_cnt().
>>    - Add ThreadsSMRSupport::update_smr_java_thread_list_max().
>>    - Cleanup "friends" for Thread class and ThreadsList class.
>>    - Add includes of runtime/threadSMR.hpp in a few files.
>>
>>
>> Here is the webrev URL:
>>
>> http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-0
>>
>> This fix was (over) tested with a Mach5 tier[1-5] run. There were no
>> unexpected test failures.
>>
>> These changes were sanity checked a couple of ways:
>>
>> - Compare pre-Thread-SMR thread.[ch]pp with thread.[ch]pp after this fix
>>    - Make sure that remaining Thread-SMR changes in thread.cpp and
>>      thread.hpp make sense to leave behind.
>>    - Similar comparison for thread.inline.hpp.
>> - Compare code removed from thread.cpp with code added to threadSMR.cpp,
>>    compare code removed from thread.hpp with code added to 
>> threadSMR.hpp,
>>    compare code removed from thread.inline.hpp with code added to
>>    threadSMR.inline.hpp:
>>    - Make sure the deltas make sense.
>>
>> Thanks, in advance, for any comments, questions or suggestions.
>>
>> Dan