Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

fix: sleep for 100ms between benchmarks #5

Closed
wants to merge 1 commit into from

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Apr 11, 2024

  • Check the assumption that there may be a bug in setTimeout implementation when it's called with no timeout value.

  • Bring back the console log statement printed after a benchmark is finished but before we schedule the next one. This should give us visibility into the question whether Voyager gets stuck inside the benchmark code or in the main loop logic.

Links:

- Check the assumption that there may be a bug in `setTimeout`
  implementation when it's called with no timeout value.

- Bring back the console log statement printed after a benchmark
  is finished but before we schedule the next one. This should give us
  visibility into the question whether Voyager gets stuck inside
  the benchmark code or in the main loop logic.

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
@bajtos
Copy link
Member Author

bajtos commented Apr 11, 2024

@juliangruber I don't know the ramifications of introducing a 100ms delay between benchmarks. Will it reduce the number of checks performed by our network too much?

We can choose a smaller delay, but OTOH, I think the timer precision is around 50ms, so using a value smaller than 50ms may not make sense.

@juliangruber
Copy link
Member

@juliangruber I don't know the ramifications of introducing a 100ms delay between benchmarks. Will it reduce the number of checks performed by our network too much?

Yes, unfortunately this will negatively affect the amount of tests Voyager schedules.

What about instead we return runSaturnBenchmarkInterval()?

@bajtos
Copy link
Member Author

bajtos commented Apr 11, 2024

https://github.com/filecoin-station/spark/blob/e78405179c0e49a2ae56b13c0d035372690a106b/lib/spark.js#L213-L230

@juliangruber I don't know the ramifications of introducing a 100ms delay between benchmarks. Will it reduce the number of checks performed by our network too much?

Yes, unfortunately this will negatively affect the amount of tests Voyager schedules.

Yeah, I was worried that could be the case.

What about instead we return runSaturnBenchmarkInterval()?

That should work.

Is there any particular reason why we cannot run a while(true) loop instead? We are using that in Spark.

https://github.com/filecoin-station/spark/blob/e78405179c0e49a2ae56b13c0d035372690a106b/lib/spark.js#L213-L230

  async run () {
    while (true) {
      const started = Date.now()
      try {
        await this.nextRetrieval()
        this.#activity.onHealthy()
      } catch (err) {
        this.handleRunError(err)
      }
      const duration = Date.now() - started
      const baseDelay = APPROX_ROUND_LENGTH_IN_MS / this.#maxTasksPerNode
      const delay = baseDelay - duration
      if (delay > 0) {
        console.log('Sleeping for %s seconds before starting the next task...', Math.round(delay / 1000))
        await sleep(delay)
      }
    }
  }

@juliangruber
Copy link
Member

No particular reason, just that we wanted to keep the module code as close to the original service worker as possible

Copy link
Member

@juliangruber juliangruber left a comment

Choose a reason for hiding this comment

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

as you suggested, a while (true) would be great

@bajtos
Copy link
Member Author

bajtos commented Apr 15, 2024

as you suggested, a while (true) would be great

Opened #6

I also realised that runBenchmark always prints a message at the end, so there is no need to print another message in the main loop (as long as there is no delay between runs).

@bajtos bajtos closed this in #6 Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants