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

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

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

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

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

coleen.phillimore

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.

Otherwise, looks fine.  Thank you for moving these!

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

Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
On 11/29/17 5:04 PM, [hidden email] 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
>

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore

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

Thanks,
Coleen

On 11/29/17 5:11 PM, Daniel D. Daugherty wrote:

> On 11/29/17 5:04 PM, [hidden email] 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
>>
>

Reply | Threaded
Open this post in threaded view
|

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

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

On 30/11/2017 7:16 AM, 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

3779 inline ThreadsList* Threads::get_smr_java_thread_list() {
3780   return
(ThreadsList*)OrderAccess::load_acquire(&_smr_java_thread_list);
3781 }

Don't these inline definitions need to come before any use of them
elsewhere in the source file, to actually get the inlining benefit?

Otherwise no comments from me on the code motion. I'm abstaining from
the debate on whether to list the functions immediately after the
associated fields, or whether to group fields and functions separately
(normally different access means they are separate, but not in this
case). :)

Thanks,
David
-----

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

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

Daniel D. Daugherty
In reply to this post by coleen.phillimore
On 11/29/17 10:46 PM, [hidden email] 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, [hidden email] 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
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
In reply to this post by David Holmes
On 11/30/17 12:07 AM, David Holmes wrote:

> Hi Dan,
>
> On 30/11/2017 7:16 AM, 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
>
> 3779 inline ThreadsList* Threads::get_smr_java_thread_list() {
> 3780   return
> (ThreadsList*)OrderAccess::load_acquire(&_smr_java_thread_list);
> 3781 }
>
> Don't these inline definitions need to come before any use of them
> elsewhere in the source file, to actually get the inlining benefit?

I don't know. I was under the impression that some of the compilers
do not properly do inlining of functions in .cpp files, but I was
told during the 8167108 review that that is no longer the case with
the current compilers.

Hopefully one of the C++ compiler cognizant folks will chime in...


> Otherwise no comments from me on the code motion. I'm abstaining from
> the debate on whether to list the functions immediately after the
> associated fields, or whether to group fields and functions separately
> (normally different access means they are separate, but not in this
> case). :)

Thanks for the review!

Dan


>
> Thanks,
> David
> -----
>
>>
>> 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

Reply | Threaded
Open this post in threaded view
|

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

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

I have updated the fix based on Coleen's and David H's code reviews.

Delta webrev:

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

Full webrev:

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

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

Dan

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
>

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore

Looks great!
Coleen

On 11/30/17 10:49 AM, Daniel D. Daugherty wrote:

> Greetings,
>
> I have updated the fix based on Coleen's and David H's code reviews.
>
> Delta webrev:
>
> http://cr.openjdk.java.net/~dcubed/8191787-webrev/jdk10-1-delta/
>
> Full webrev:
>
> http://cr.openjdk.java.net/~dcubed/8191787-webrev/jdk10-1-full/
>
> Thanks, in advance, for any comments, questions or suggestions.
>
> Dan
>
> 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
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
Thanks for taking another look!

Dan


On 11/30/17 1:57 PM, [hidden email] wrote:

>
> Looks great!
> Coleen
>
> On 11/30/17 10:49 AM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I have updated the fix based on Coleen's and David H's code reviews.
>>
>> Delta webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8191787-webrev/jdk10-1-delta/
>>
>> Full webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8191787-webrev/jdk10-1-full/
>>
>> Thanks, in advance, for any comments, questions or suggestions.
>>
>> Dan
>>
>> 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
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

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

May I suggest simply moving all of the inline smr functions to the same
position, after all the field initializations, so that it is hopefully
more evident that they appear before any use.

My layperson understanding - perhaps out of date in 2017 - is that to
inline a function the compiler has to have already seen the definition.

Thanks,
David

On 1/12/2017 1:49 AM, Daniel D. Daugherty wrote:

> Greetings,
>
> I have updated the fix based on Coleen's and David H's code reviews.
>
> Delta webrev:
>
> http://cr.openjdk.java.net/~dcubed/8191787-webrev/jdk10-1-delta/
>
> Full webrev:
>
> http://cr.openjdk.java.net/~dcubed/8191787-webrev/jdk10-1-full/
>
> Thanks, in advance, for any comments, questions or suggestions.
>
> Dan
>
> 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
>>
>
Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
Hi David,

Thanks for looking at this again!


On 11/30/17 4:24 PM, David Holmes wrote:
> Hi Dan,
>
> May I suggest simply moving all of the inline smr functions to the
> same position, after all the field initializations, so that it is
> hopefully more evident that they appear before any use.

I'm in my pre-integration Mach5 tier[12] test cycle so I wasn't planning
on any more tweaks for this bug. Can I pick up this change in my next
code motion fix (8191789)? This fix (8191787) will likely make Jesper's
snapshot on 12.01. My next fix will not...


> My layperson understanding - perhaps out of date in 2017 - is that to
> inline a function the compiler has to have already seen the definition.

I was hoping for someone else to jump in to confirm or deny, but
that hasn't happened. That's why I went ahead and moved the one
inline definition before its first use... All other inline defs
are before first use (but it is not obvious).

Dan


>
> Thanks,
> David
>
> On 1/12/2017 1:49 AM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I have updated the fix based on Coleen's and David H's code reviews.
>>
>> Delta webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8191787-webrev/jdk10-1-delta/
>>
>> Full webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8191787-webrev/jdk10-1-full/
>>
>> Thanks, in advance, for any comments, questions or suggestions.
>>
>> Dan
>>
>> 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
>>>
>>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
On 1/12/2017 7:33 AM, Daniel D. Daugherty wrote:

> Hi David,
>
> Thanks for looking at this again!
>
>
> On 11/30/17 4:24 PM, David Holmes wrote:
>> Hi Dan,
>>
>> May I suggest simply moving all of the inline smr functions to the
>> same position, after all the field initializations, so that it is
>> hopefully more evident that they appear before any use.
>
> I'm in my pre-integration Mach5 tier[12] test cycle so I wasn't planning
> on any more tweaks for this bug. Can I pick up this change in my next
> code motion fix (8191789)? This fix (8191787) will likely make Jesper's
> snapshot on 12.01. My next fix will not...

Sure, no problem. It just avoids the need to check where the first use
may be.

Thanks,
David

>
>> My layperson understanding - perhaps out of date in 2017 - is that to
>> inline a function the compiler has to have already seen the definition.
>
> I was hoping for someone else to jump in to confirm or deny, but
> that hasn't happened. That's why I went ahead and moved the one
> inline definition before its first use... All other inline defs
> are before first use (but it is not obvious).
>
> Dan
>
>
>>
>> Thanks,
>> David
>>
>> On 1/12/2017 1:49 AM, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> I have updated the fix based on Coleen's and David H's code reviews.
>>>
>>> Delta webrev:
>>>
>>> http://cr.openjdk.java.net/~dcubed/8191787-webrev/jdk10-1-delta/
>>>
>>> Full webrev:
>>>
>>> http://cr.openjdk.java.net/~dcubed/8191787-webrev/jdk10-1-full/
>>>
>>> Thanks, in advance, for any comments, questions or suggestions.
>>>
>>> Dan
>>>
>>> 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
>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
On 11/30/17 8:08 PM, David Holmes wrote:

> On 1/12/2017 7:33 AM, Daniel D. Daugherty wrote:
>> Hi David,
>>
>> Thanks for looking at this again!
>>
>>
>> On 11/30/17 4:24 PM, David Holmes wrote:
>>> Hi Dan,
>>>
>>> May I suggest simply moving all of the inline smr functions to the
>>> same position, after all the field initializations, so that it is
>>> hopefully more evident that they appear before any use.
>>
>> I'm in my pre-integration Mach5 tier[12] test cycle so I wasn't planning
>> on any more tweaks for this bug. Can I pick up this change in my next
>> code motion fix (8191789)? This fix (8191787) will likely make Jesper's
>> snapshot on 12.01. My next fix will not...
>
> Sure, no problem. It just avoids the need to check where the first use
> may be.

Thanks!

Dan


>
> Thanks,
> David
>
>>
>>> My layperson understanding - perhaps out of date in 2017 - is that
>>> to inline a function the compiler has to have already seen the
>>> definition.
>>
>> I was hoping for someone else to jump in to confirm or deny, but
>> that hasn't happened. That's why I went ahead and moved the one
>> inline definition before its first use... All other inline defs
>> are before first use (but it is not obvious).
>>
>> Dan
>>
>>
>>>
>>> Thanks,
>>> David
>>>
>>> On 1/12/2017 1:49 AM, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>>
>>>> I have updated the fix based on Coleen's and David H's code reviews.
>>>>
>>>> Delta webrev:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8191787-webrev/jdk10-1-delta/
>>>>
>>>> Full webrev:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8191787-webrev/jdk10-1-full/
>>>>
>>>> Thanks, in advance, for any comments, questions or suggestions.
>>>>
>>>> Dan
>>>>
>>>> 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
>>>>>
>>>>
>>