Skip to content

Ensure ChromeTraceTest event timestamps are emitted in correct invervals #21911

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

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

WojciechMazur
Copy link
Contributor

Fixes #21882 flaky test

private def sleep(duration: FiniteDuration): Unit = {
// A bit of additional precautions to ensure we don't continue execution to early
// Both LockSuppport and Thread.sleep can return earlier then expected (depending on OS)
var remainingNanos = duration.toNanos
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is just a matter of personal taste or if it might make some difference in the actual behaviour:
Why not to compute the expected end time of sleep before the lop and then compare it with System.nanoTime in the loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might behave differently. If we just sleep again using duration then in the worst case, we can sleep for 1.99x of expected time. To prevent that, we'd need to calculate the remaining time after pre-mature wake up anyway.

1 possible improvement could be potentially usage of TimeUnit.sleep(ofUnit: Long) but that would probably make it more complicate. Previously I though it's available only since JDK 9

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course I didn't mean sleeping twice with the same duration. I was thinking of something like

  private def sleep(duration: FiniteDuration): Unit = {
    // A bit of additional precautions to ensure we don't continue execution to early
    // Both LockSuppport and Thread.sleep can return earlier then expected (depending on OS)
    // var remainingNanos = duration.toNanos
    var currentNanos = System.nanoTime()
    val deadline = currentNanos + duration.toNanos
    while
      currentNanos = System.nanoTime()
      currentNanos < deadline
    do
      val remainingNanos = deadline - currentNanos
      val millis = NANOSECONDS.toMillis(remainingNanos)
      Thread.sleep(millis, (remainingNanos % 1.millis.toNanos).toInt)
  }

But maybe this doesn't make that much of a difference

@WojciechMazur WojciechMazur merged commit 8a3f375 into scala:main Nov 8, 2024
58 checks passed
@WojciechMazur WojciechMazur deleted the fix/21882 branch November 8, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nightly Dotty workflow of 2024-11-04 failed
2 participants