8290324: Move atomic operations outside of os_xxx.hpp #9501

iklam · 2022-07-14T21:38:58Z

The os_xxx.hpp files inject extra methods/fields that are specific to atomic operations into the os class. However, the injected methods/fields are used only by a specific os/cpu combination. Therefore, they should not be inside the os class, which should contain only APIs that are used across platforms.

For ports where the atomic_copy64() function is used in a single file, I moved it as an inline function in that file
Otherwise it's moved to atomic_<os>_<cpu>.hpp
The linux/arm port is a little more involved, but the new code should be a little cleaner than the old code.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8290324: Move atomic operations outside of os_xxx.hpp

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 397ba800
Kim Barrett (@kimbarrett - Reviewer) ⚠️ Review applies to 0d8a150f

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9501/head:pull/9501
$ git checkout pull/9501

Update a local copy of the PR:
$ git checkout pull/9501
$ git pull https://git.openjdk.org/jdk pull/9501/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9501

View PR using the GUI difftool:
$ git pr show -t 9501

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9501.diff

bridgekeeper · 2022-07-14T21:39:52Z

👋 Welcome back iklam! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-07-14T21:43:34Z

@iklam The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-07-14T21:53:38Z

Webrevs

dholmes-ora

Seems okay - good cleanup.

I'm okay with it if Aarch64 folk are.

Thanks.

openjdk · 2022-07-15T06:16:22Z

@iklam This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8290324: Move atomic operations outside of os_xxx.hpp

Reviewed-by: dholmes, kbarrett

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

kimbarrett · 2022-07-18T20:51:14Z

It seems weird that for x86 we have _Atomic_move_long while for other platforms we have atomic_copy64. It's also odd that we have 64bit implementation of the latter but not the former, in the sense that one would expect to have both or neither.

I think all of the 64bit implementations of atomic_copy64 are identical. I wonder if a better refactoring might be to have a shared atomic_copy64.[inline.?]hpp that has a shared 64bit implementation and uses includes platform files (using the usual macros for that) to get 32bit implementations when appropriate.

Maybe these could all be dealt with as followups.

Several of the atomic_copy64 implementations are being moved to places where they are being given C linkage. What's the rationale for that?

iklam · 2022-07-18T21:12:17Z

It seems weird that for x86 we have _Atomic_move_long while for other platforms we have atomic_copy64. It's also odd that we have 64bit implementation of the latter but not the former, in the sense that one would expect to have both or neither.

I think all of the 64bit implementations of atomic_copy64 are identical. I wonder if a better refactoring might be to have a shared atomic_copy64.[inline.?]hpp that has a shared 64bit implementation and uses includes platform files (using the usual macros for that) to get 32bit implementations when appropriate.

Maybe these could all be dealt with as followups.

For this PR, I just wanted to move code around but otherwise avoid making any actual changes. I'd prefer to do the further clean up in a follow up issue.

Several of the atomic_copy64 implementations are being moved to places where they are being given C linkage. What's the rationale for that?

That was unintentional. They just happened to be moved to immediate above the only function that uses them, and that function happens to have C linkage. Maybe I should explicitly mark these atomic_copy64 as static inline to be clear that whether they are C or C++ linkage is inconsequential?

kimbarrett · 2022-07-18T20:54:22Z

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp

@@ -582,18 +582,23 @@ extern "C" {
        *(to--) = *(from--);
    }
  }
+
+  inline void atomic_copy64(const volatile void *src, volatile void *dst) {
+    *(jlong *) dst = *(const jlong *) src;


s/jlong/uint64_t/ (or int64_t). And casting away volatile seems kind of sketchy. This could be

Atomic::store((uint64_t*)dst, Atomic::load((const uint64_t*)src));

which seems clearer to me.

This is also from the original version. I think we can fix this in a separate PR.

kimbarrett · 2022-07-18T21:01:14Z

src/hotspot/os_cpu/bsd_zero/atomic_bsd_zero.hpp

 template<>
 template<typename T>
 inline T Atomic::PlatformLoad<8>::operator()(T const volatile* src) const {
  STATIC_ASSERT(8 == sizeof(T));
  volatile int64_t dest;
-  os::atomic_copy64(reinterpret_cast<const volatile int64_t*>(src), reinterpret_cast<volatile int64_t*>(&dest));
+  atomic_copy64(reinterpret_cast<const volatile int64_t*>(src), reinterpret_cast<volatile int64_t*>(&dest));


[pre-existing] Why do we have these casts when atomic_copy64 takes volatile void*? (There are more like this that I didn't bother to comment directly.)

To be honest I don't know :-)

kimbarrett · 2022-07-18T21:04:51Z

src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.cpp

  void _Copy_conjoint_jlongs_atomic(const jlong* from, jlong* to, size_t count) {
    if (from > to) {
      const jlong *end = from + count;
      while (from < end)
-        os::atomic_copy64(from++, to++);
+        atomic_copy64(from++, to++);


If atomic_copy64 was a shared API with platform-specific implementations we could eliminate all these copies of _Copy_conjoint_jlongs_atomic.

kimbarrett · 2022-07-18T21:11:25Z

src/hotspot/os_cpu/linux_arm/atomic_linux_arm.hpp

+                                    volatile int32_t *dest);
+};
+
+extern arm_atomic_funcs _arm_atomic;


Why is arm_atomic_funcs not an AllStatic class? (And it has a non-conventional name.)

…". Also removed os:: from C_frame_offset on linux/arm.

dholmes-ora

Still okay. But I would have resisted making non-move-related changes and leave that for future cleanups.

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp

iklam · 2022-07-20T23:49:52Z

I have created a new JBS issue ( https://bugs.openjdk.org/browse/JDK-8290745 - Refactor atomic_copy64() functions ) to capture @kimbarrett's comments on the various problems with the atomic copying functions.

…move-atomic-ops-outside-os-hpp

iklam · 2022-07-21T05:09:37Z

Thanks to @dholmes-ora and @kimbarrett for the review. Passed build tiers 1 and 5.
/integrate

openjdk · 2022-07-21T05:12:18Z

Going to push as commit 2c73a1f.

openjdk · 2022-07-21T05:12:36Z

@iklam Pushed as commit 2c73a1f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8290324: Move atomic operations outside of os_xxx.hpp

e840200

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jul 14, 2022

iklam marked this pull request as ready for review July 14, 2022 21:47

openjdk bot added the rfr Pull request is ready for review label Jul 14, 2022

dholmes-ora approved these changes Jul 15, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 15, 2022

Merge branch 'master' into 8290324-move-atomic-ops-outside-os-hpp

2ba6add

kimbarrett reviewed Jul 18, 2022

View reviewed changes

@kimbarrett comments: cleaned up ARMAtomicFuncs; added "static inline…

397ba80

…". Also removed os:: from C_frame_offset on linux/arm.

dholmes-ora approved these changes Jul 19, 2022

View reviewed changes

kimbarrett suggested changes Jul 20, 2022

View reviewed changes

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp Outdated Show resolved Hide resolved

moved 'static inline void atomic_copy64' outside of 'extern "C"' block

0d8a150

kimbarrett approved these changes Jul 21, 2022

View reviewed changes

Merge branch 'master' of https://github.com/openjdk/jdk into 8290324-…

d646fcc

…move-atomic-ops-outside-os-hpp

openjdk bot added the integrated Pull request has been integrated label Jul 21, 2022

openjdk bot closed this Jul 21, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8290324: Move atomic operations outside of os_xxx.hpp #9501

8290324: Move atomic operations outside of os_xxx.hpp #9501

iklam commented Jul 14, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 14, 2022

openjdk bot commented Jul 14, 2022

mlbridge bot commented Jul 14, 2022 •

edited

Loading

dholmes-ora left a comment

openjdk bot commented Jul 15, 2022 •

edited

Loading

kimbarrett commented Jul 18, 2022

iklam commented Jul 18, 2022

kimbarrett Jul 18, 2022

iklam Jul 18, 2022

kimbarrett Jul 18, 2022

iklam Jul 18, 2022

kimbarrett Jul 18, 2022

kimbarrett Jul 18, 2022

dholmes-ora left a comment

iklam commented Jul 20, 2022

iklam commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

8290324: Move atomic operations outside of os_xxx.hpp #9501

8290324: Move atomic operations outside of os_xxx.hpp #9501

Conversation

iklam commented Jul 14, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jul 14, 2022

openjdk bot commented Jul 14, 2022

mlbridge bot commented Jul 14, 2022 • edited Loading

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

openjdk bot commented Jul 15, 2022 • edited Loading

kimbarrett commented Jul 18, 2022

iklam commented Jul 18, 2022

kimbarrett Jul 18, 2022

Choose a reason for hiding this comment

iklam Jul 18, 2022

Choose a reason for hiding this comment

kimbarrett Jul 18, 2022

Choose a reason for hiding this comment

iklam Jul 18, 2022

Choose a reason for hiding this comment

kimbarrett Jul 18, 2022

Choose a reason for hiding this comment

kimbarrett Jul 18, 2022

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

iklam commented Jul 20, 2022

iklam commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

iklam commented Jul 14, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 14, 2022 •

edited

Loading

openjdk bot commented Jul 15, 2022 •

edited

Loading