Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8246112: Remove build-time and run-time checks for clock_gettime and CLOCK_MONOTONIC #2090

Closed
wants to merge 13 commits into from

Conversation

dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Jan 15, 2021

We are now confident that we have build-time and runtime support for clock_gettime and CLOCK_MONOTONIC on all our OpenJDK supported POSIX platforms - see bug report for some more details on different OS. Consequently we can simplify a lot of the code in this area and move common code to os_posix.

As of glibc 2.17 the necessary functions are in glibc rather than librt, but we (Oracle at least) aren't yet in position to set our minimum Linux version to support that. We still have supported platforms at glibc 2.12. So to address that we link librt at build time on Linux. This seems to work find for older and more modern Linuxes and also works for the Apline Linux with Musl variant.

The changes are in layered commits:

Step 1: Remove build time checks. SUPPORTS_CLOCK_MONOTONIC is assumed true and removed.
Step 2: make supports_monotonic_clock always true and so remove checking in OS code
Step 3: Replace gettimeofday by clock_gettime(CLOCK_REALTIME)
Step 4: Move shared time functions to os_posix.cpp
Step 5: Alway link librt on Linux so we don't rely on glibc > 2.17

Testing: tiers 1-3 for functional testing
built and checked (-Xlog:os) on Linux with glibc 2.12 and 2.17, macOS 10.13.6 and 10.15.7


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8246112: Remove build-time and run-time checks for clock_gettime and CLOCK_MONOTONIC

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2090/head:pull/2090
$ git checkout pull/2090

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 15, 2021

👋 Welcome back dholmes! 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 openjdk bot added the rfr Pull request is ready for review label Jan 15, 2021
@openjdk
Copy link

openjdk bot commented Jan 15, 2021

@dholmes-ora The following labels will be automatically applied to this pull request:

  • build
  • hotspot-runtime

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

@openjdk openjdk bot added hotspot-runtime hotspot-runtime-dev@openjdk.org build build-dev@openjdk.org labels Jan 15, 2021
@dholmes-ora dholmes-ora reopened this Jan 18, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 18, 2021

Webrevs

Copy link
Member

@magicus magicus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good cleanup. But please minimize the use of -lrt to hotspot only as per the comment above.

# But once our supported minimum build and runtime platform
# has glibc 2.17, this can be removed as the functions are
# in libc.
OS_LDFLAGS="-lrt"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be OS_LDFLAGS_JVM_ONLY, right?

@mlbridge
Copy link

mlbridge bot commented Jan 18, 2021

Mailing list message from David Holmes on build-dev:

On 18/01/2021 11:59 pm, Magnus Ihse Bursie wrote:

On Fri, 15 Jan 2021 04:57:24 GMT, David Holmes <dholmes at openjdk.org> wrote:

We are now confident that we have build-time and runtime support for clock_gettime and CLOCK_MONOTONIC on all our OpenJDK supported POSIX platforms - see bug report for some more details on different OS. Consequently we can simplify a lot of the code in this area and move common code to os_posix.

As of glibc 2.17 the necessary functions are in glibc rather than librt, but we (Oracle at least) aren't yet in position to set our minimum Linux version to support that. We still have supported platforms at glibc 2.12. So to address that we link librt at build time on Linux. This seems to work find for older and more modern Linuxes and also works for the Apline Linux with Musl variant.

The changes are in layered commits:

Step 1: Remove build time checks. SUPPORTS_CLOCK_MONOTONIC is assumed true and removed.
Step 2: make supports_monotonic_clock always true and so remove checking in OS code
Step 3: Replace gettimeofday by clock_gettime(CLOCK_REALTIME)
Step 4: Move shared time functions to os_posix.cpp
Step 5: Alway link librt on Linux so we don't rely on glibc > 2.17

Testing: tiers 1-3 for functional testing
built and checked (-Xlog:os) on Linux with glibc 2.12 and 2.17, macOS 10.13.6 and 10.15.7

Looks like a good cleanup. But please minimize the use of -lrt to hotspot only as per the comment above.

make/autoconf/flags-ldflags.m4 line 116:

114: # has glibc 2.17, this can be removed as the functions are
115: # in libc.
116: OS_LDFLAGS="-lrt"

I believe this should be `OS_LDFLAGS_JVM_ONLY`, right?

Right - sorry, don't know how I missed that.

Thanks,
David

Copy link
Member

@erikj79 erikj79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build changes look good to me.

@openjdk
Copy link

openjdk bot commented Jan 19, 2021

@dholmes-ora this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8246112-mono
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jan 19, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 20, 2021

Mailing list message from David Holmes on build-dev:

Thanks for the reviews Erik and Magnus!

David

On 20/01/2021 12:26 am, Erik Joelsson wrote:

@@ -1,5 +1,5 @@
/*
* Copyright (c) 1999, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1999, 2030, Oracle and/or its affiliates. All rights reserved.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Year 2030 ?

jlong usecs = jlong(time.tv_sec) * (1000 * 1000) + jlong(time.tv_usec);
return 1000 * usecs;
}
void os::javaTimeNanos_info(jvmtiTimerInfo *info_ptr) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come we need os::javaTimeNanos_info here if there is already one in os_posix.cpp?

// These need to be members so we can access them from inline functions
static int (*_clock_gettime)(clockid_t, struct timespec *);
static int (*_clock_getres)(clockid_t, struct timespec *);
public:
static bool supports_monotonic_clock();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this API at all now?

thread.cpp uses it here:

  assert(!os::supports_monotonic_clock(),
         "unexpected time moving backwards detected in JavaThread::sleep()");

so we can just remove this usage?

@@ -1211,28 +1158,13 @@ void os::Posix::init(void) {
}

void os::Posix::init_2(void) {
log_info(os)("Use of CLOCK_MONOTONIC is%s supported",
(_clock_gettime != NULL ? "" : " not"));
log_info(os)("Use of CLOCK_MONOTONIC is supported");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are keeping this output to avoid breaking whomever might be looking for this text?

@openjdk
Copy link

openjdk bot commented Jan 21, 2021

@dholmes-ora 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:

8246112: Remove build-time and run-time checks for clock_gettime and CLOCK_MONOTONIC

Reviewed-by: ihse, erikj, gziemski, hseigel

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 31 new commits pushed to the master branch:

  • 19b6f61: 8260334: Remove deprecated sv_for_node_id() from Compile
  • 1bebd41: 8260421: Shenandoah: Fix conc_mark_roots timing name and indentations
  • 42cef27: 8260343: Delete obsolete classes in the Windows L&F
  • 9f0a043: 8260286: Manual Test "ws/open/test/jdk/sun/security/tools/jarsigner/compatibility/Compatibility.java" fails
  • fd00ed7: 8256298: Shenandoah: Enable concurrent stack processing
  • b07797c: 8260391: Remove StringCoding::err
  • af8a08f: 8260378: [TESTBUG] DcmdMBeanTestCheckJni.java reports false positive
  • 8d2f77f: 8260406: Do not copy pure java source code to gensrc
  • 5e8e0ad: 8242456: PreviewFeature.Feature enum removal of TEXT_BLOCKS
  • e080ce9: 8252545: runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java timed out
  • ... and 21 more: https://git.openjdk.java.net/jdk/compare/764111ff831096729eb1bcc69a54d92fde67b044...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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.

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Jan 21, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 21, 2021

Mailing list message from David Holmes on build-dev:

Hi Gerard,

Thanks for looking at this!

On 21/01/2021 7:34 am, Gerard Ziemski wrote:

On Tue, 19 Jan 2021 01:53:03 GMT, David Holmes <dholmes at openjdk.org> wrote:

We are now confident that we have build-time and runtime support for clock_gettime and CLOCK_MONOTONIC on all our OpenJDK supported POSIX platforms - see bug report for some more details on different OS. Consequently we can simplify a lot of the code in this area and move common code to os_posix.

As of glibc 2.17 the necessary functions are in glibc rather than librt, but we (Oracle at least) aren't yet in position to set our minimum Linux version to support that. We still have supported platforms at glibc 2.12. So to address that we link librt at build time on Linux. This seems to work find for older and more modern Linuxes and also works for the Apline Linux with Musl variant.

The changes are in layered commits:

Step 1: Remove build time checks. SUPPORTS_CLOCK_MONOTONIC is assumed true and removed.
Step 2: make supports_monotonic_clock always true and so remove checking in OS code
Step 3: Replace gettimeofday by clock_gettime(CLOCK_REALTIME)
Step 4: Move shared time functions to os_posix.cpp
Step 5: Alway link librt on Linux so we don't rely on glibc > 2.17

src/hotspot/os/bsd/os_bsd.inline.hpp line 2:

1: /*
2: * Copyright (c) 1999, 2030, Oracle and/or its affiliates. All rights reserved.

Year 2030 ?

No idea how that happened :) Fixed via merge with mainline.

src/hotspot/os/bsd/os_bsd.cpp line 828:

826: }
827:
828: void os::javaTimeNanos_info(jvmtiTimerInfo *info_ptr) {

How come we need `os::javaTimeNanos_info` here if there is already one in `os_posix.cpp`?

javaTimeNanos_info describes the properties of the implementation of
javaTimeNanos. As we have a custom implementation of javaTimeNanos_info
for macOS (and AIX) it seemed appropriate to also define
javaTimeNanos_info, even if it happens to be the same as the Posix
version. Alternatively I could put a comment block in there warning
anyone who might change the implementation to check if the Posix version
is still correct (and I'd change the ifdefs in the os_posix.cpp code).

src/hotspot/os/posix/os_posix.hpp line 95:

93: static void ucontext_set_pc(ucontext_t* ctx, address pc);
94:
95: static bool supports_monotonic_clock();

Why do we need this API at all now?

thread.cpp uses it here:

   assert\(\!os\:\:supports\_monotonic\_clock\(\)\,
          \"unexpected time moving backwards detected in JavaThread\:\:sleep\(\)\"\)\;

so we can just remove this usage?

Good catch! Yes we can do this. I had intended to do it but got
distracted by the linking issues and forgot about it.

src/hotspot/os/posix/os_posix.cpp line 1161:

1159:
1160: void os::Posix::init_2(void) {
1161: log_info(os)("Use of CLOCK_MONOTONIC is supported");

We are keeping this output to avoid breaking whomever might be looking for this text?

I find it useful information, especially in conjunction with the lines
that follow, and allows comparing different JDK versions.

Thanks,
David

@tstuefe
Copy link
Member

tstuefe commented Jan 22, 2021

Builds fine on AIX. I scheduled tests for the coming nights.
About os::javaTimeNanos, not sure if we still need that for AIX, but monotonic clocks gave us a lot of grief in the past on that .. platform and I prefer it to leave that way for now. Investigating if we can use clock_gettime for os::javaTimeNanos like Linux does can be done in a separate RFE.

Copy link
Member

@hseigel hseigel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi David,
The changes look good. Just a couple of nits.

  1. Could you add a comment explaining why AIX and BSD have their own version of javaTimeNanos_info().
  2. A few copyrights need to be updated to 2021.
    Thanks, Harold

@mlbridge
Copy link

mlbridge bot commented Jan 22, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Harold,

Thanks for the review.

On 23/01/2021 2:20 am, Harold Seigel wrote:

On Thu, 21 Jan 2021 05:24:09 GMT, David Holmes <dholmes at openjdk.org> wrote:

We are now confident that we have build-time and runtime support for clock_gettime and CLOCK_MONOTONIC on all our OpenJDK supported POSIX platforms - see bug report for some more details on different OS. Consequently we can simplify a lot of the code in this area and move common code to os_posix.

As of glibc 2.17 the necessary functions are in glibc rather than librt, but we (Oracle at least) aren't yet in position to set our minimum Linux version to support that. We still have supported platforms at glibc 2.12. So to address that we link librt at build time on Linux. This seems to work find for older and more modern Linuxes and also works for the Apline Linux with Musl variant.

The changes are in layered commits:

Step 1: Remove build time checks. SUPPORTS_CLOCK_MONOTONIC is assumed true and removed.
Step 2: make supports_monotonic_clock always true and so remove checking in OS code
Step 3: Replace gettimeofday by clock_gettime(CLOCK_REALTIME)
Step 4: Move shared time functions to os_posix.cpp
Step 5: Alway link librt on Linux so we don't rely on glibc > 2.17

Testing: tiers 1-3 for functional testing
built and checked (-Xlog:os) on Linux with glibc 2.12 and 2.17, macOS 10.13.6 and 10.15.7

David Holmes has updated the pull request incrementally with one additional commit since the last revision:

Remove the always true os::supports_monotonic_clock()

Hi David,
The changes look good. Just a couple of nits.
1. Could you add a comment explaining why AIX and BSD have their own version of javaTimeNanos_info().

Where would you like that comment? in os_posix.cpp? I can't give a
definitive reason other than its an implementation of monotonic time
that worked on those platforms. It is possible (as Thomas mentioned for
AIX) that these days clock_gettime would be fine, but someone would have
to investigate that.

2. A few copyrights need to be updated to 2021.

Thanks for the reminder - I did most of this last year :)

Thanks,
David

@mlbridge
Copy link

mlbridge bot commented Jan 22, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 22/01/2021 8:13 pm, Thomas Stuefe wrote:

On Thu, 21 Jan 2021 15:26:43 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

David Holmes has updated the pull request incrementally with one additional commit since the last revision:

Remove the always true os::supports_monotonic_clock()

Marked as reviewed by gziemski (Committer).

Builds fine on AIX. I scheduled tests for the coming nights.
About os::javaTimeNanos, not sure if we still need that for AIX, but monotonic clocks gave us a lot of grief in the past on that .. platform and I prefer it to leave that way for now. Investigating if we can use clock_gettime for os::javaTimeNanos like Linux does can be done in a separate RFE.

Thanks Thomas. I will wait to hear back from you before finalising this.

David

@tstuefe
Copy link
Member

tstuefe commented Jan 25, 2021

I don't see any new issues on AIX in our tests with this patch. Thanks for waiting!

@mlbridge
Copy link

mlbridge bot commented Jan 25, 2021

Mailing list message from David Holmes on build-dev:

On 25/01/2021 5:19 pm, Thomas Stuefe wrote:

On Fri, 22 Jan 2021 16:17:57 GMT, Harold Seigel <hseigel at openjdk.org> wrote:

David Holmes has updated the pull request incrementally with one additional commit since the last revision:

Remove the always true os::supports_monotonic_clock()

Hi David,
The changes look good. Just a couple of nits.
1. Could you add a comment explaining why AIX and BSD have their own version of javaTimeNanos_info().
2. A few copyrights need to be updated to 2021.
Thanks, Harold

I don't see any new issues on AIX in our tests with this patch. Thanks for waiting!

Thanks Thomas!

David

@hseigel
Copy link
Member

hseigel commented Jan 25, 2021

Hi David,

I think that os_posix.cpp is a good place to put a comment explaining why AIX and BSD have their own javaTimeNanos_info(). Stating something like "AIX and BSD have their own versions because their implementations of monotonic time have worked on those platforms." seems fine.

Thanks for doing this!
Harold

Comment on lines +1286 to +1292
jlong os::javaTimeMillis() {
struct timespec ts;
int status = clock_gettime(CLOCK_REALTIME, &ts);
assert(status == 0, "clock_gettime error: %s", os::strerror(errno));
return jlong(ts.tv_sec) * MILLIUNITS +
jlong(ts.tv_nsec) / NANOUNITS_PER_MILLIUNIT;
}
Copy link
Contributor

@mkraljt mkraljt Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

David,
Great to see simplifications like this happening in OpenJdk code.

  • Posix letting you delete platform specific code.
  • Always nice to see 'if' branching eliminated to simplify test coverage and runtime.

@dholmes-ora
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Jan 27, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 27, 2021
@openjdk
Copy link

openjdk bot commented Jan 27, 2021

@dholmes-ora Since your change was applied there have been 31 commits pushed to the master branch:

  • 19b6f61: 8260334: Remove deprecated sv_for_node_id() from Compile
  • 1bebd41: 8260421: Shenandoah: Fix conc_mark_roots timing name and indentations
  • 42cef27: 8260343: Delete obsolete classes in the Windows L&F
  • 9f0a043: 8260286: Manual Test "ws/open/test/jdk/sun/security/tools/jarsigner/compatibility/Compatibility.java" fails
  • fd00ed7: 8256298: Shenandoah: Enable concurrent stack processing
  • b07797c: 8260391: Remove StringCoding::err
  • af8a08f: 8260378: [TESTBUG] DcmdMBeanTestCheckJni.java reports false positive
  • 8d2f77f: 8260406: Do not copy pure java source code to gensrc
  • 5e8e0ad: 8242456: PreviewFeature.Feature enum removal of TEXT_BLOCKS
  • e080ce9: 8252545: runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java timed out
  • ... and 21 more: https://git.openjdk.java.net/jdk/compare/764111ff831096729eb1bcc69a54d92fde67b044...master

Your commit was automatically rebased without conflicts.

Pushed as commit 6f2be9c.

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

@dholmes-ora dholmes-ora deleted the 8246112-mono branch January 27, 2021 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
7 participants