RFR(S/M): 8191789 - migrate more Thread-SMR stuff from thread.[ch]pp -> threadSMR.[ch]pp
Stefan Karlsson
stefan.karlsson at oracle.com
Tue Dec 5 16:22:56 UTC 2017
Hi Dan,
On 2017-12-05 16:27, Daniel D. Daugherty wrote:
> 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...
>
It makes sense to me to not change it while moving the files. I think
its better to push that change as a separate RFE.
>
>> 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?
I should have been more clear. This function should preferably move out
from the header file into an .inline.hpp or .cpp file. During
pre-reviews of the SMR work we discussed this, but it seems like we
missed this function. This can be handled by a separate RFE.
>
>
>> ========================================================================
>> 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...
Sounds good to me.
Thanks,
StefanK
>
> 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
>
More information about the hotspot-runtime-dev
mailing list