RFR(S/M): 8191789 - migrate more Thread-SMR stuff from thread.[ch]pp -> threadSMR.[ch]pp
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Dec 5 16:47:55 UTC 2017
On 12/5/17 11:22 AM, Stefan Karlsson wrote:
> 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.
Okay. I'll take it on as a separate changeset.
>
>>
>>> 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.
Since that one wasn't tagged as "inline" so I didn't move it earlier;
my focus in that review cycle was moving inlines out of .hpp files.
I can move it now (and will likely make it an inline)...
>
>>
>>
>>> ========================================================================
>>>
>>> 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.
Dan
>
> 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