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

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
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!

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
Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
Did a sanity check self-review and found a couple of things in
threadSMR.cpp are out of sort order:

- moved ThreadsList::remove_thread()
- moved ThreadsListSetter::~ThreadsListSetter() and ThreadsListSetter::set()

These are pure code motion and I'll include those changes in
delta webrev after making any other changes from the reviewers.

Dan


On 12/4/17 3:22 PM, 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!
>
> 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
>

Reply | Threaded
Open this post in threaded view
|

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

Stefan Karlsson
In reply to this post by Daniel D. Daugherty
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.

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


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);
+  }

========================================================================
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);

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

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
Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

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

Stefan Karlsson
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
>
Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
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
>>

Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
In reply to this post by Daniel D. Daugherty
Greetings,

I've updated the fix based on Stefan K's code review feedback (and my
own self-review)...

Delta webrev:

http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-1-delta/

Full webrev:

http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-1-full/


Summary of the changes in this round:

- moved ThreadsList::remove_thread()
- moved ThreadsListSetter::~ThreadsListSetter() and ThreadsListSetter::set()
- remove Threads as a friend of class Thread
- add ThreadsSMRSupport::update_smr_tlh_stats() to allow more
ThreadsSMRSupport
   code to be private

- refactor Threads-SMR code in Threads::add() to call
   new function ThreadsSMRSupport::add_thread()
- refactor Threads-SMR code in Threads::remove() to call
   new function ThreadsSMRSupport::remove_thread()
- more ThreadsSMRSupport functions can be private
- move is_a_protected_JavaThread_with_lock() to
   threadSMR.inline.hpp


I have a Mach5 tier[1-5] in progress for this round...

Thanks, in advance, for any comments, questions or suggestions.

Dan



On 12/4/17 3:22 PM, 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!
>
> 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
>

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
Thank you so much for moving this code.  The comment from the main
change with the examples look good too (just rereading it now).

Thanks,
Coleen

On 12/5/17 4:26 PM, Daniel D. Daugherty wrote:

> Greetings,
>
> I've updated the fix based on Stefan K's code review feedback (and my
> own self-review)...
>
> Delta webrev:
>
> http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-1-delta/
>
> Full webrev:
>
> http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-1-full/
>
>
> Summary of the changes in this round:
>
> - moved ThreadsList::remove_thread()
> - moved ThreadsListSetter::~ThreadsListSetter() and
> ThreadsListSetter::set()
> - remove Threads as a friend of class Thread
> - add ThreadsSMRSupport::update_smr_tlh_stats() to allow more
> ThreadsSMRSupport
>   code to be private
>
> - refactor Threads-SMR code in Threads::add() to call
>   new function ThreadsSMRSupport::add_thread()
> - refactor Threads-SMR code in Threads::remove() to call
>   new function ThreadsSMRSupport::remove_thread()
> - more ThreadsSMRSupport functions can be private
> - move is_a_protected_JavaThread_with_lock() to
>   threadSMR.inline.hpp
>
>
> I have a Mach5 tier[1-5] in progress for this round...
>
> Thanks, in advance, for any comments, questions or suggestions.
>
> Dan
>
>
>
> On 12/4/17 3:22 PM, 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!
>>
>> 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
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
Coleen,

Thanks for the review!

Dan


On 12/5/17 4:37 PM, [hidden email] wrote:

> Thank you so much for moving this code.  The comment from the main
> change with the examples look good too (just rereading it now).
>
> Thanks,
> Coleen
>
> On 12/5/17 4:26 PM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I've updated the fix based on Stefan K's code review feedback (and my
>> own self-review)...
>>
>> Delta webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-1-delta/
>>
>> Full webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-1-full/
>>
>>
>> Summary of the changes in this round:
>>
>> - moved ThreadsList::remove_thread()
>> - moved ThreadsListSetter::~ThreadsListSetter() and
>> ThreadsListSetter::set()
>> - remove Threads as a friend of class Thread
>> - add ThreadsSMRSupport::update_smr_tlh_stats() to allow more
>> ThreadsSMRSupport
>>   code to be private
>>
>> - refactor Threads-SMR code in Threads::add() to call
>>   new function ThreadsSMRSupport::add_thread()
>> - refactor Threads-SMR code in Threads::remove() to call
>>   new function ThreadsSMRSupport::remove_thread()
>> - more ThreadsSMRSupport functions can be private
>> - move is_a_protected_JavaThread_with_lock() to
>>   threadSMR.inline.hpp
>>
>>
>> I have a Mach5 tier[1-5] in progress for this round...
>>
>> Thanks, in advance, for any comments, questions or suggestions.
>>
>> Dan
>>
>>
>>
>> On 12/4/17 3:22 PM, 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!
>>>
>>> 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
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by Daniel D. Daugherty
Hi Dan,

This all looks good to me.

I agree with removing the now redundant _smr_ name components at a later
time.

Thanks,
David

On 6/12/2017 7:26 AM, Daniel D. Daugherty wrote:

> Greetings,
>
> I've updated the fix based on Stefan K's code review feedback (and my
> own self-review)...
>
> Delta webrev:
>
> http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-1-delta/
>
> Full webrev:
>
> http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-1-full/
>
>
> Summary of the changes in this round:
>
> - moved ThreadsList::remove_thread()
> - moved ThreadsListSetter::~ThreadsListSetter() and
> ThreadsListSetter::set()
> - remove Threads as a friend of class Thread
> - add ThreadsSMRSupport::update_smr_tlh_stats() to allow more
> ThreadsSMRSupport
>    code to be private
>
> - refactor Threads-SMR code in Threads::add() to call
>    new function ThreadsSMRSupport::add_thread()
> - refactor Threads-SMR code in Threads::remove() to call
>    new function ThreadsSMRSupport::remove_thread()
> - more ThreadsSMRSupport functions can be private
> - move is_a_protected_JavaThread_with_lock() to
>    threadSMR.inline.hpp
>
>
> I have a Mach5 tier[1-5] in progress for this round...
>
> Thanks, in advance, for any comments, questions or suggestions.
>
> Dan
>
>
>
> On 12/4/17 3:22 PM, 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!
>>
>> 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
>>
>
Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
David,

Thanks for the review!

Dan


On 12/5/17 10:38 PM, David Holmes wrote:

> Hi Dan,
>
> This all looks good to me.
>
> I agree with removing the now redundant _smr_ name components at a
> later time.
>
> Thanks,
> David
>
> On 6/12/2017 7:26 AM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I've updated the fix based on Stefan K's code review feedback (and my
>> own self-review)...
>>
>> Delta webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-1-delta/
>>
>> Full webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-1-full/
>>
>>
>> Summary of the changes in this round:
>>
>> - moved ThreadsList::remove_thread()
>> - moved ThreadsListSetter::~ThreadsListSetter() and
>> ThreadsListSetter::set()
>> - remove Threads as a friend of class Thread
>> - add ThreadsSMRSupport::update_smr_tlh_stats() to allow more
>> ThreadsSMRSupport
>>    code to be private
>>
>> - refactor Threads-SMR code in Threads::add() to call
>>    new function ThreadsSMRSupport::add_thread()
>> - refactor Threads-SMR code in Threads::remove() to call
>>    new function ThreadsSMRSupport::remove_thread()
>> - more ThreadsSMRSupport functions can be private
>> - move is_a_protected_JavaThread_with_lock() to
>>    threadSMR.inline.hpp
>>
>>
>> I have a Mach5 tier[1-5] in progress for this round...
>>
>> Thanks, in advance, for any comments, questions or suggestions.
>>
>> Dan
>>
>>
>>
>> On 12/4/17 3:22 PM, 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!
>>>
>>> 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
>>>
>>

Reply | Threaded
Open this post in threaded view
|

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

Gerald Thornbrugh
In reply to this post by Daniel D. Daugherty
Hi Dan,

Your changes look good.

Jerry

> On Dec 5, 2017, at 2:26 PM, Daniel D. Daugherty <[hidden email]> wrote:
>
> Greetings,
>
> I've updated the fix based on Stefan K's code review feedback (and my
> own self-review)...
>
> Delta webrev:
>
> http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-1-delta/
>
> Full webrev:
>
> http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-1-full/
>
>
> Summary of the changes in this round:
>
> - moved ThreadsList::remove_thread()
> - moved ThreadsListSetter::~ThreadsListSetter() and ThreadsListSetter::set()
> - remove Threads as a friend of class Thread
> - add ThreadsSMRSupport::update_smr_tlh_stats() to allow more ThreadsSMRSupport
>   code to be private
>
> - refactor Threads-SMR code in Threads::add() to call
>   new function ThreadsSMRSupport::add_thread()
> - refactor Threads-SMR code in Threads::remove() to call
>   new function ThreadsSMRSupport::remove_thread()
> - more ThreadsSMRSupport functions can be private
> - move is_a_protected_JavaThread_with_lock() to
>   threadSMR.inline.hpp
>
>
> I have a Mach5 tier[1-5] in progress for this round...
>
> Thanks, in advance, for any comments, questions or suggestions.
>
> Dan
>
>
>
> On 12/4/17 3:22 PM, 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!
>>
>> 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
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
Jerry,

Thanks for the review!

Dan


On 12/6/17 9:29 AM, Gerald Thornbrugh wrote:

> Hi Dan,
>
> Your changes look good.
>
> Jerry
>> On Dec 5, 2017, at 2:26 PM, Daniel D. Daugherty <[hidden email]> wrote:
>>
>> Greetings,
>>
>> I've updated the fix based on Stefan K's code review feedback (and my
>> own self-review)...
>>
>> Delta webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-1-delta/
>>
>> Full webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8191789-webrev/jdk10-1-full/
>>
>>
>> Summary of the changes in this round:
>>
>> - moved ThreadsList::remove_thread()
>> - moved ThreadsListSetter::~ThreadsListSetter() and ThreadsListSetter::set()
>> - remove Threads as a friend of class Thread
>> - add ThreadsSMRSupport::update_smr_tlh_stats() to allow more ThreadsSMRSupport
>>    code to be private
>>
>> - refactor Threads-SMR code in Threads::add() to call
>>    new function ThreadsSMRSupport::add_thread()
>> - refactor Threads-SMR code in Threads::remove() to call
>>    new function ThreadsSMRSupport::remove_thread()
>> - more ThreadsSMRSupport functions can be private
>> - move is_a_protected_JavaThread_with_lock() to
>>    threadSMR.inline.hpp
>>
>>
>> I have a Mach5 tier[1-5] in progress for this round...
>>
>> Thanks, in advance, for any comments, questions or suggestions.
>>
>> Dan
>>
>>
>>
>> On 12/4/17 3:22 PM, 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!
>>>
>>> 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
>>>