RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation

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

RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation

Anton Kozlov-2
Hi,

Please reivew a small non-functional change that extracts inline SafeFetch functions to a separate file. This is preliminary work for JEP-391 integration that will reduce the size of that patch.

CC @dcubed-ojdk

Thanks!

-------------

Commit messages:
 - Update copyrights
 - Merge remote-tracking branch 'upstream/jdk/master' into 8261075-stubroutines-inline
 - Merge remote-tracking branch 'upstream/jdk/master' into 8261075-stubroutines-inline
 - Extract SafeFetch32/N to stubRoutines.inline.hpp

Changes: https://git.openjdk.java.net/jdk/pull/2542/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2542&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261075
  Stats: 86 lines in 11 files changed: 53 ins; 20 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2542.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2542/head:pull/2542

PR: https://git.openjdk.java.net/jdk/pull/2542
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation

Daniel D.Daugherty
On Fri, 12 Feb 2021 09:52:44 GMT, Anton Kozlov <[hidden email]> wrote:

> Hi,
>
> Please reivew a small non-functional change that extracts inline SafeFetch functions to a separate file. This is preliminary work for JEP-391 integration that will reduce the size of that patch.
>
> CC @dcubed-ojdk
>
> Thanks!

Thumbs up.

-------------

Marked as reviewed by dcubed (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2542
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation

Thomas Stuefe
In reply to this post by Anton Kozlov-2
On Fri, 12 Feb 2021 09:52:44 GMT, Anton Kozlov <[hidden email]> wrote:

> Hi,
>
> Please reivew a small non-functional change that extracts inline SafeFetch functions to a separate file. This is preliminary work for JEP-391 integration that will reduce the size of that patch.
>
> CC @dcubed-ojdk
>
> Thanks!

I would rename the new header safefetch.hpp, just because these functions were never part of the StubRoutines namespace, and the naming is much clearer.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2542
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation

Anton Kozlov-2
On Fri, 12 Feb 2021 17:29:29 GMT, Thomas Stuefe <[hidden email]> wrote:

> I would rename the new header safefetch.hpp, just because these functions were never part of the StubRoutines namespace, and the naming is much clearer.

Good idea. It should be safefetch.inline.hpp, right?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2542
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation

Thomas Stuefe
On Mon, 15 Feb 2021 15:18:57 GMT, Anton Kozlov <[hidden email]> wrote:

> > I would rename the new header safefetch.hpp, just because these functions were never part of the StubRoutines namespace, and the naming is much clearer.
>
> Good idea. It should be safefetch.inline.hpp, right?

I don't think so? To my understanding, xxx.inline.hpp is a companion file to an xxx.hpp which you create if you want to remove some of the rarely used functions from a high traffic header. Here, we just have some independent global utility functions.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2542
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation

Anton Kozlov-2
On Mon, 15 Feb 2021 15:36:18 GMT, Thomas Stuefe <[hidden email]> wrote:

> To my understanding, xxx.inline.hpp is a companion file to an xxx.hpp which you create if you want to remove some of the rarely used functions from a high traffic header.

Oh, you're right. I had an impression that there are standalone inline.hpp files, and I could not find the opposite in the Hotspot Style Guide. However, now I see inline.hpp are just the way you've described. Thanks!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2542
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation

Stefan Karlsson-3
On Mon, 15 Feb 2021 15:47:47 GMT, Anton Kozlov <[hidden email]> wrote:

>>> > I would rename the new header safefetch.hpp, just because these functions were never part of the StubRoutines namespace, and the naming is much clearer.
>>>
>>> Good idea. It should be safefetch.inline.hpp, right?
>>
>> I don't think so? To my understanding, xxx.inline.hpp is a companion file to an xxx.hpp which you create if you want to remove some of the rarely used functions from a high traffic header. Here, we just have some independent global utility functions.
>
>> To my understanding, xxx.inline.hpp is a companion file to an xxx.hpp which you create if you want to remove some of the rarely used functions from a high traffic header.
>
> Oh, you're right. I had an impression that there are standalone inline.hpp files, and I could not find the opposite in the Hotspot Style Guide. However, now I see inline.hpp are just the way you've described. Thanks!

If you put non-trivial code in the header, then it should go into an .inline.hpp file (or .cpp file). From the style-guide:
Do not put non-trivial function implementations in .hpp files. If the implementation depends on other .hpp files, put it in a .cpp or a .inline.hpp file.
It doesn't matter if there exists a .hpp file or not. However, what's non-trivial becomes a judgment call. I tend to use rule that if it uses code from another header, then it's non-trivial, and should most likely be moved out of the .hpp file.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2542
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation [v2]

Anton Kozlov-2
In reply to this post by Anton Kozlov-2
> Hi,
>
> Please reivew a small non-functional change that extracts inline SafeFetch functions to a separate file. This is preliminary work for JEP-391 integration that will reduce the size of that patch.
>
> CC @dcubed-ojdk
>
> Thanks!

Anton Kozlov has updated the pull request incrementally with one additional commit since the last revision:

  stubRoutines.inline.hpp -> safefetch.hpp

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2542/files
  - new: https://git.openjdk.java.net/jdk/pull/2542/files/a00d9064..d2957b98

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2542&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2542&range=00-01

  Stats: 20 lines in 10 files changed: 7 ins; 8 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2542.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2542/head:pull/2542

PR: https://git.openjdk.java.net/jdk/pull/2542
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation [v2]

Anton Kozlov-2
In reply to this post by Stefan Karlsson-3
On Mon, 15 Feb 2021 19:53:48 GMT, Stefan Karlsson <[hidden email]> wrote:

> It doesn't matter if there exists a .hpp file or not. However, what's non-trivial becomes a judgment call. I tend to use rule that if it uses code from another header, then it's non-trivial, and should most likely be moved out of the .hpp file.

I suppose this file to be on the edge between trivial and not. Later, it will have a W^X transition and will include thread.hpp, I don't want to rename it again. @stefank, what do you think, should it be safefetch.inline.hpp? Or are you fine with safefetch.hpp?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2542
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation [v2]

Stefan Karlsson-3
In reply to this post by Anton Kozlov-2
On Tue, 16 Feb 2021 13:04:04 GMT, Anton Kozlov <[hidden email]> wrote:

>> Hi,
>>
>> Please reivew a small non-functional change that extracts inline SafeFetch functions to a separate file. This is preliminary work for JEP-391 integration that will reduce the size of that patch.
>>
>> CC @dcubed-ojdk
>>
>> Thanks!
>
> Anton Kozlov has updated the pull request incrementally with one additional commit since the last revision:
>
>   stubRoutines.inline.hpp -> safefetch.hpp

Marked as reviewed by stefank (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2542
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation [v2]

Stefan Karlsson-3
In reply to this post by Anton Kozlov-2
On Tue, 16 Feb 2021 16:04:05 GMT, Anton Kozlov <[hidden email]> wrote:

>> If you put non-trivial code in the header, then it should go into an .inline.hpp file (or .cpp file). From the style-guide:
>> Do not put non-trivial function implementations in .hpp files. If the implementation depends on other .hpp files, put it in a .cpp or a .inline.hpp file.
>> It doesn't matter if there exists a .hpp file or not. However, what's non-trivial becomes a judgment call. I tend to use rule that if it uses code from another header, then it's non-trivial, and should most likely be moved out of the .hpp file.
>
>> It doesn't matter if there exists a .hpp file or not. However, what's non-trivial becomes a judgment call. I tend to use rule that if it uses code from another header, then it's non-trivial, and should most likely be moved out of the .hpp file.
>
> I suppose this file to be on the edge between trivial and not. Later, it will have a W^X transition and will include thread.hpp, I don't want to rename it again. @stefank, what do you think, should it be safefetch.inline.hpp? Or are you fine with safefetch.hpp?

I'm fine with leaving it as safefetch.hpp.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2542
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation [v2]

Thomas Stuefe
In reply to this post by Anton Kozlov-2
On Tue, 16 Feb 2021 13:04:04 GMT, Anton Kozlov <[hidden email]> wrote:

>> Hi,
>>
>> Please reivew a small non-functional change that extracts inline SafeFetch functions to a separate file. This is preliminary work for JEP-391 integration that will reduce the size of that patch.
>>
>> CC @dcubed-ojdk
>>
>> Thanks!
>
> Anton Kozlov has updated the pull request incrementally with one additional commit since the last revision:
>
>   stubRoutines.inline.hpp -> safefetch.hpp

LGTM

-------------

Marked as reviewed by stuefe (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2542
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation [v2]

Daniel D.Daugherty
In reply to this post by Anton Kozlov-2
On Tue, 16 Feb 2021 13:04:04 GMT, Anton Kozlov <[hidden email]> wrote:

>> Hi,
>>
>> Please reivew a small non-functional change that extracts inline SafeFetch functions to a separate file. This is preliminary work for JEP-391 integration that will reduce the size of that patch.
>>
>> CC @dcubed-ojdk
>>
>> Thanks!
>
> Anton Kozlov has updated the pull request incrementally with one additional commit since the last revision:
>
>   stubRoutines.inline.hpp -> safefetch.hpp

Still thumbs up.

-------------

Marked as reviewed by dcubed (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2542
Reply | Threaded
Open this post in threaded view
|

Integrated: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation

Anton Kozlov-2
In reply to this post by Anton Kozlov-2
On Fri, 12 Feb 2021 09:52:44 GMT, Anton Kozlov <[hidden email]> wrote:

> Hi,
>
> Please reivew a small non-functional change that extracts inline SafeFetch functions to a separate file. This is preliminary work for JEP-391 integration that will reduce the size of that patch.
>
> CC @dcubed-ojdk
>
> Thanks!

This pull request has now been integrated.

Changeset: b955f85e
Author:    Anton Kozlov <[hidden email]>
Committer: Vladimir Kempik <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/b955f85e
Stats:     91 lines in 11 files changed: 58 ins; 26 del; 7 mod

8261075: Create stubRoutines.inline.hpp with SafeFetch implementation

Reviewed-by: dcubed, stuefe, stefank

-------------

PR: https://git.openjdk.java.net/jdk/pull/2542
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation [v2]

Anton Kozlov-2
In reply to this post by Stefan Karlsson-3
On Tue, 16 Feb 2021 16:11:36 GMT, Stefan Karlsson <[hidden email]> wrote:

>> Anton Kozlov has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   stubRoutines.inline.hpp -> safefetch.hpp
>
> Marked as reviewed by stefank (Reviewer).

I've messed up with this :( I'm trying to fix @stefank 's note https://github.com/openjdk/jdk/pull/2200#discussion_r572707505. I need to rename a file.hpp included in the safepoint.hpp to file.inline.hpp. This will require renaming safepoint.hpp to safepoint.inline.hpp, polluting the PR #2200 again and defeating the purpose of having this patch extracted. I think the better alternative will be a follow-up patch just renaming safepoint.hpp to safepoint.inline.hpp. I'm going to do the follow-up. Or is there a better alternative? Sorry and thanks.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2542
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation [v2]

Stefan Karlsson-3
On Thu, 4 Mar 2021 18:39:02 GMT, Anton Kozlov <[hidden email]> wrote:

>> Marked as reviewed by stefank (Reviewer).
>
> I've messed up with this :( I'm trying to fix @stefank 's note https://github.com/openjdk/jdk/pull/2200#discussion_r572707505. I need to rename a file.hpp included in the safepoint.hpp to file.inline.hpp. This will require renaming safepoint.hpp to safepoint.inline.hpp, polluting the PR #2200 again and defeating the purpose of having this patch extracted. I think the better alternative will be a follow-up patch just renaming safepoint.hpp to safepoint.inline.hpp. I'm going to do the follow-up. Or is there a better alternative? Sorry and thanks.

Just so that I understand. Did you really mean safepoint.hpp and not safefetch.hpp? I assume this all has to do with the fact that safefetch.hpp is going to include threadWXSetter.hpp, which you are going to change to threadWXSsetter.inline.hpp, because it includes thread.inline.hpp. If that's the case then I think the easy fix is to just rename safefetch.hpp to safefetch.inline.hpp.

I don't think that will be problematic, or defeat the purpose of this patch. The previous change from stubRoutines.hpp to stubRoutines.inline.hpp has already been successfully been removed from your #2200 patch. If you create a new PR with a safefetch.hpp to safefetch.inline.hpp rename, I can review it as a trivial change and you will be able to push it immediately.

Or did I misunderstand anything? Maybe you could point me to the problematic files?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2542
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation [v2]

Anton Kozlov-2
On Thu, 4 Mar 2021 19:13:05 GMT, Stefan Karlsson <[hidden email]> wrote:

> Just so that I understand. Did you really mean safepoint.hpp and not safefetch.hpp?

Definitely safefetch.hpp. Sorry, it seems I was thinking also about another problem at that moment :)

> I assume this all has to do with the fact that safefetch.hpp is going to include threadWXSetter.hpp, which you are going to change to threadWXSsetter.inline.hpp, because it includes thread.inline.hpp. If that's the case then I think the easy fix is to just rename safefetch.hpp to safefetch.inline.hpp.

This is correct. My main concern was the noise I'm creating in the git repository, but I also think this is the best way in this situation. Thanks for confirmation! The bug is JDK-8263068 and PR is #2844.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2542