RFR(XS): 8191787 - move private inline functions from thread.inline.hpp -> thread.cpp

[Daniel D. Daugherty]

On 11/29/17 10:46 PM, coleen.phillimore at oracle.com wrote:
>
> Yes, definitely the "kitchen sink collection".    I meant instead of :
>
>     // Safe Memory Reclamation (SMR) support:
> + // The coordination between Threads::release_stable_list() and
> + // Threads::smr_delete() uses the smr_delete_lock in order to
> + // reduce the traffic on the Threads_lock.
>     static Monitor*              _smr_delete_lock;
> + static Monitor* smr_delete_lock() { return _smr_delete_lock; }
>     // The '_cnt', '_max' and '_times" fields are enabled via
>     // -XX:+EnableThreadSMRStatistics (see thread.cpp for a
>     // description about each field):
>     static uint                  _smr_delete_lock_wait_cnt;
>     static uint                  _smr_delete_lock_wait_max;
> + // The smr_delete_notify flag is used for proper double-check
> + // locking in order to reduce the traffic on the smr_delete_lock.
>     static volatile uint         _smr_delete_notify;
> + static bool smr_delete_notify();
> + static void set_smr_delete_notify();
> + static void clear_smr_delete_notify();
>     static volatile uint         _smr_deleted_thread_cnt;
> + static void inc_smr_deleted_thread_cnt();
>     static volatile uint         _smr_deleted_thread_time_max;
> + static void update_smr_deleted_thread_time_max(uint new_value);
>     static volatile uint         _smr_deleted_thread_times;
> + static void add_smr_deleted_thread_times(uint add_value);
>     static ThreadsList* volatile _smr_java_thread_list;
>     static ThreadsList*          get_smr_java_thread_list();
>     static ThreadsList*          xchg_smr_java_thread_list(ThreadsList* new_list);
>     static uint64_t              _smr_java_thread_list_alloc_cnt;
>     static uint64_t              _smr_java_thread_list_free_cnt;
>
> To have the below.  They're all together but fields and functions have 
> been separated.
>
>     // Safe Memory Reclamation (SMR) support:
> + // The coordination between Threads::release_stable_list() and
> + // Threads::smr_delete() uses the smr_delete_lock in order to
> + // reduce the traffic on the Threads_lock.
>     static Monitor*              _smr_delete_lock;
>     // The '_cnt', '_max' and '_times" fields are enabled via
>     // -XX:+EnableThreadSMRStatistics (see thread.cpp for a
>     // description about each field):
>     static uint                  _smr_delete_lock_wait_cnt;
>     static uint                  _smr_delete_lock_wait_max;
> + // The smr_delete_notify flag is used for proper double-check
> + // locking in order to reduce the traffic on the smr_delete_lock.
>     static volatile uint         _smr_delete_notify;
>     static volatile uint         _smr_deleted_thread_cnt;
>     static volatile uint         _smr_deleted_thread_time_max;
>     static volatile uint         _smr_deleted_thread_times;
>     static ThreadsList* volatile _smr_java_thread_list;
>     static uint64_t              _smr_java_thread_list_alloc_cnt;
>     static uint64_t              _smr_java_thread_list_free_cnt;
> + static Monitor* smr_delete_lock() { return _smr_delete_lock; } + 
> static bool smr_delete_notify();
> + static void set_smr_delete_notify();
> + static void clear_smr_delete_notify();
> + static void inc_smr_deleted_thread_cnt();
> + static void update_smr_deleted_thread_time_max(uint new_value);
> + static void add_smr_deleted_thread_times(uint add_value);
>     static ThreadsList*          get_smr_java_thread_list();
>     static ThreadsList*          xchg_smr_java_thread_list(ThreadsList* new_list);


I can do this. I will sort that list of functions though... :-)

Dan


>
> Thanks,
> Coleen
>
> On 11/29/17 5:11 PM, Daniel D. Daugherty wrote:
>> On 11/29/17 5:04 PM, coleen.phillimore at oracle.com wrote:
>>>
>>> http://cr.openjdk.java.net/~dcubed/8191787-webrev/jdk10-0/src/hotspot/share/runtime/thread.hpp.udiff.html 
>>>
>>>
>>> Can you put a section with the static data member declarations, then 
>>> a blank line and then have a section with the functions?   We don't 
>>> usually mix them like that (maybe in thread.hpp because it's the 
>>> kitchen sink but not everywhere else).  It makes it hard to read.
>>
>> We're trying to keep all the "stuff" associated with a field together.
>> It's definitely not a style typically in use in HotSpot, but we're
>> experimenting with it because thread.hpp is currently a kitchen sink
>> collection. Some folks would say it is a mess... :-)
>>
>> I'm going to take a look this cleanup bug also:
>>
>> JDK-8191789 migrate more Thread-SMR stuff from thread.[ch]pp -> 
>> threadSMR.[ch]pp
>> https://bugs.openjdk.java.net/browse/JDK-8191789
>>
>> since it will primarily be "code motion" fix also. Keeping "stuff"
>> together will make that cleanup bug easier...
>>
>> Would you be okay with this fix (8191787) as it is?
>>
>>
>>> Otherwise, looks fine.  Thank you for moving these!
>>
>> Thanks!
>>
>> Dan
>>
>>
>>>
>>> Coleen
>>>
>>> On 11/29/17 4:16 PM, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>>
>>>> Coleen, 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 cleanup fix for Thread-SMR. The bug is:
>>>>
>>>>     JDK-8191787 move private inline functions from 
>>>> thread.inline.hpp -> thread.cpp
>>>> https://bugs.openjdk.java.net/browse/JDK-8191787
>>>>
>>>> This fix is pure code motion:
>>>>
>>>> - moving inline functions from thread.inline.hpp -> thread.cpp
>>>> - making a few functions in thread.hpp private instead of public
>>>>
>>>> Here is the webrev URL:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8191787-webrev/jdk10-0
>>>>
>>>> This fix was (over) tested with a Mach5 tier[1-5] run. There were no
>>>> unexpected test failures.
>>>>
>>>> Thanks, in advance, for any comments, questions or suggestions.
>>>>
>>>> Dan
>>>
>>
>