crac icon indicating copy to clipboard operation
crac copied to clipboard

8351402: [CRaC] Use System.nanoTime() in TimedWaitingTest

Open TimPushkin opened this issue 8 months ago • 7 comments

Replaces System.currentTimeMillis() with System.nanoTime() in TimedWaitingTest since the former can, in theory, jump back and forth and that may lead to the test failures.

Also adds a diagnostic assert.


Progress

  • [x] Change must not contain extraneous whitespace

Issue

  • JDK-8351402: [CRaC] Use System.nanoTime() in TimedWaitingTest (Bug - P3)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 209

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/crac/pull/209.diff

Using Webrev

Link to Webrev Comment

TimPushkin avatar Mar 07 '25 12:03 TimPushkin

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

bridgekeeper[bot] avatar Mar 07 '25 12:03 bridgekeeper[bot]

@TimPushkin This change is no longer ready for integration - check the PR body for details.

openjdk[bot] avatar Mar 07 '25 12:03 openjdk[bot]

/integrate

TimPushkin avatar Mar 07 '25 12:03 TimPushkin

@TimPushkin Your change (at version 2f1a1e148208b567232cdaed4346b72fb662d3d3) is now ready to be sponsored by a Committer.

openjdk[bot] avatar Mar 07 '25 12:03 openjdk[bot]

Webrevs

mlbridge[bot] avatar Mar 07 '25 12:03 mlbridge[bot]

There have been private discussions about the test. Whether it should test:

  • the specification compliance: that timed waiting is not shorter than it should be according to the spec — in this case using nanoTime should probably be the fix, or
  • the implementation: that CRaC doesn't manipulate nanoTime in a way that makes timed waiting shorter than it would be without CRaC (this should be a superset of the first point) — in this case we should use some other way to access monotonic time but not nanoTime.

I think we've leaned towards the second, so it should be checked that the non-monotonicity of systemTimeMillis is really the reason of the test failures. So I'll make this a draft for now.

TimPushkin avatar Mar 11 '25 11:03 TimPushkin

I refreshed my memories about the tests, and the first approach looks good for me. There is a comment in the bug. Sorry for misleading you.

AntonKozlov avatar Mar 12 '25 13:03 AntonKozlov

Closing in favor of #221

TimPushkin avatar Apr 14 '25 10:04 TimPushkin