8311639: Replace currentTimeMillis() with nanoTime() in jtreg/gc #15331

lkorinth · 2023-08-17T15:56:04Z

I have removed usage of currentTimeMillis() in jtreg/gc as currentTimeMillis() is not monotonic. It is mostly changing from milliseconds to nanoseconds. In certain places, I have changed the code from something like (instant1 < instant2) to (instant1 - instant2 < 0) It might look like I am removing instant2 from both sides of the inequality and the result ought to be the same, but due to overflow arithmetic, the change should be better if nanoseconds where to overflow. I have also removed some loops where the loop is doing nothing except sleeping. I somewhat shortened the string in the key of a hash map because the nanosecond value made the string too long for the heap size.

All tests within jtreg/gc passes.

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-8311639: Replace currentTimeMillis() with nanoTime() in jtreg/gc (Enhancement - P4)

Reviewers

Stefan Karlsson (@stefank - Reviewer)
Albert Mingkun Yang (@albertnetymk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15331

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-08-17T15:57:45Z

👋 Welcome back lkorinth! 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 · 2023-08-17T15:59:56Z

@lkorinth The following labels will be automatically applied to this pull request:

hotspot-gc
shenandoah

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.

mlbridge · 2023-08-17T16:03:41Z

Webrevs

albertnetymk · 2023-08-17T17:46:28Z

test/hotspot/jtreg/gc/stress/systemgc/TestSystemGC.java

@@ -77,7 +77,7 @@ class LongLivedAllocationTask extends Exitable implements Runnable {
    @Override
    public void run() {
        while (!shouldExit()) {
-            String prefix = "long" + System.currentTimeMillis();
+            String prefix = "long" + (System.nanoTime() % 10000); // limit to 4 digits after changing from milliseconds to nanoseconds, else the key will use more memory


Is this used to calculate elapsed time? If not, I think using currentTimeMillis is fine.

It is not to calculate elapsed time. It is to create a unique(ish) key for a hash table. The new code does not guarantee a unique value, but then neither did the old.

I could possibly add some characters to the key if that is what bothers you. I could of course also use currentTimeMillis as it is nothing wrong with it. But I would prefer removing currentTimeMillis in all gc testing.

I will try to see if I can do it without using time at all.

Not using time is probably the best. Not obvious if #digits here is critical. Maybe this is useful -- e.g. to generate fixed 6-digit int:

Random rnd = new Random(); int n = 100_000 + rnd.nextInt(900_000);

I reverted to the original implementation as I did not understand the idea behind it.

stefank

Looks good. I'd also go with Albert's suggestion and keep timeout logging in TestSystemGC.

stefank · 2023-08-18T07:14:41Z

test/hotspot/jtreg/gc/stress/systemgc/TestSystemGC.java

-        System.out.println("Running with timeout of " + timeout + "ms");
-        endTime = System.currentTimeMillis() + timeout;
+        long timeoutNanos = Integer.parseInt(args[0]) * 1_000_000_000L;
+        System.out.println("Running with timeout of " + timeoutNanos + "ns");


The precision here is in seconds. Maybe just leave this print out to be in ms?

I will change it to seconds.

openjdk · 2023-08-18T07:18:14Z

@lkorinth 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:

8311639: Replace currentTimeMillis() with nanoTime() in jtreg/gc

Reviewed-by: stefank, ayang

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

7521211: 8314265: Fix -Wconversion warnings in miscellaneous runtime code
2f04bc5: 8313697: [XWayland][Screencast] consequent getPixelColor calls are slow
33d5dfd: 8314543: gitattributes: make diffs easier to read
5058854: 8314389: AttachListener::pd_set_flag obsolete
891c3f4: 8307356: Metaspace: simplify BinList handling
0299364: 8314249: Refactor handling of invokedynamic in JVMCI ConstantPool
96778dd: 8314169: Combine related RoundingMode logic in j.text.DigitList
808bb1f: 8314246: javax/swing/JToolBar/4529206/bug4529206.java fails intermittently on Linux
6445314: 8314477: Improve definition of "prototypical type"
d27daf0: 8314129: Make fields final in java.util.Scanner
... and 24 more: https://git.openjdk.org/jdk/compare/b80001de0c0aeedeb412430660a4727fc26be98b...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.

lkorinth · 2023-08-18T12:11:01Z

I will refactor a bit more...

lkorinth · 2023-08-18T13:11:01Z

Thanks Albert and StefanK, I will integrate next week.

lkorinth · 2023-08-21T12:18:08Z

/integrate

openjdk · 2023-08-21T12:19:38Z

Going to push as commit 17a19dc.
Since your change was applied there have been 51 commits pushed to the master branch:

0b3f452: 8314161: G1: Fix -Wconversion warnings in G1CardSetConfiguration::_bitmap_hash_mask
abac608: 8313962: G1: Refactor G1ConcurrentMark::_num_concurrent_workers
812f475: 8314501: Shenandoah: sun/tools/jhsdb/heapconfig/JMapHeapConfigTest.java fails
8939d15: 8314100: G1: Improve collection set candidate selection code
ec1f7a8: 8311630: [s390] Implementation of Foreign Function & Memory API (Preview)
c50315d: 8314495: Update to use jtreg 7.3.1
ed0f75f: 8313290: Misleading exception message from STS.Subtask::get when task forked after shutdown
febc34d: 8314610: hotspot can't compile with the latest of gtest because of
58f5826: 8311222: strace004 can fail due to unexpected stack length after JDK-8309408
395fc78: 8309475: Test java/foreign/TestByteBuffer.java fails: a problem with msync (aix)
... and 41 more: https://git.openjdk.org/jdk/compare/b80001de0c0aeedeb412430660a4727fc26be98b...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-08-21T12:19:47Z

@lkorinth Pushed as commit 17a19dc.

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

8311639: Replace currentTimeMillis() with nanoTime() in jtreg/gc

6d2fe20

openjdk bot added the rfr Pull request is ready for review label Aug 17, 2023

openjdk bot added hotspot-gc hotspot-gc-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Aug 17, 2023

albertnetymk reviewed Aug 17, 2023

View reviewed changes

stefank approved these changes Aug 18, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 18, 2023

fix after comments from Albert and StefanK

c2e26a8

printing unparsed values

926a7f3

albertnetymk approved these changes Aug 18, 2023

View reviewed changes

stefank approved these changes Aug 18, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Aug 21, 2023

openjdk bot closed this Aug 21, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8311639: Replace currentTimeMillis() with nanoTime() in jtreg/gc #15331

8311639: Replace currentTimeMillis() with nanoTime() in jtreg/gc #15331

lkorinth commented Aug 17, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 17, 2023

openjdk bot commented Aug 17, 2023

mlbridge bot commented Aug 17, 2023 •

edited

Loading

albertnetymk Aug 17, 2023

lkorinth Aug 18, 2023

lkorinth Aug 18, 2023

lkorinth Aug 18, 2023

albertnetymk Aug 18, 2023

lkorinth Aug 18, 2023

stefank left a comment

stefank Aug 18, 2023

lkorinth Aug 18, 2023

lkorinth Aug 18, 2023

openjdk bot commented Aug 18, 2023 •

edited

Loading

lkorinth commented Aug 18, 2023

lkorinth commented Aug 18, 2023

lkorinth commented Aug 21, 2023

openjdk bot commented Aug 21, 2023

openjdk bot commented Aug 21, 2023

8311639: Replace currentTimeMillis() with nanoTime() in jtreg/gc #15331

8311639: Replace currentTimeMillis() with nanoTime() in jtreg/gc #15331

Conversation

lkorinth commented Aug 17, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Aug 17, 2023

openjdk bot commented Aug 17, 2023

mlbridge bot commented Aug 17, 2023 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Aug 18, 2023 • edited Loading

lkorinth commented Aug 18, 2023

lkorinth commented Aug 18, 2023

lkorinth commented Aug 21, 2023

openjdk bot commented Aug 21, 2023

openjdk bot commented Aug 21, 2023

lkorinth commented Aug 17, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 17, 2023 •

edited

Loading

openjdk bot commented Aug 18, 2023 •

edited

Loading