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

Update docs, comments to nudge timing with monotonic clock #214

Closed
wants to merge 1 commit into from

Conversation

openbl
Copy link

@openbl openbl commented Feb 28, 2024

What

Update docs and comments to encourage people to use the monotonic clock.

Why

The docs and comments show use of Time.now for timing interactor descendants. However, Time.now can suffer from "discontinuous jumps" that are solved by using the system's monotonic clock.1

The docs and comments show use of `Time.now` for timing interactor
descendants. However, `Time.now` can suffer from "discontinuous jumps" that
are solved by using the system's monotonic clock.
@gaffneyc
Copy link
Member

Thanks for the pull request (and sorry for it sitting so long). Do you know if CLOCK_MONOTONIC is available across all operating systems and architectures that Ruby runs on?

Looking at the examples, they're focusing on how to use hooks rather than what's in the code example itself. While using CLOCK_MONOTONIC is the right way to do it I'm not sure changing it meaningfully adds to the example as Time.now is usually good enough. The example could have used Benchmark.realtime { ... } to calculate total runtime which uses CLOCK_MONOTONIC under the hood but that would have obscured the use of interactor.call.

@gaffneyc
Copy link
Member

While using CLOCK_MONOTONIC is the right way to do it I think it just complicates the examples for little gain.

Going to close this but open to an alternative change that uses a different example to show the same concept.

@gaffneyc gaffneyc closed this Dec 28, 2024
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.

2 participants