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

Can Clock provide delta_as_nanos? #84

Closed
antifuchs opened this issue May 20, 2023 · 7 comments
Closed

Can Clock provide delta_as_nanos? #84

antifuchs opened this issue May 20, 2023 · 7 comments

Comments

@antifuchs
Copy link
Contributor

In governor, I am using a raw nanoseconds representation for quanta readings, which means that every time I take a clock reading, quanta converts nanos into an std::time::Duration from a nanos value, which I then convert back. That costs us quite a bit of performance, as well as being so dissatisfyingly close to what I want: You already have the right kind of value, can I just get it without costly conversions? (-:

@tobz
Copy link
Member

tobz commented May 24, 2023

I'm not sure I follow how you're using Clock.

If you're already taking raw measurements (Clock::raw), then Clock::delta already provides the means to get the reference clock-scaled delta back, in the form of Duration. Getting the nanosecond measurement from Duration is then a const operation: Duration::as_nanos.

You only pay the conversion cost once, the same as if you called Clock::now and got back quanta::Instant.

@antifuchs
Copy link
Contributor Author

antifuchs commented May 24, 2023

Sorry for being unclear here - the conversion you describe is exactly what I'm doing.

There are no const optimizations though, right? We're dealing with dynamic values, so the from_nanos function does an integer division & modulo operation, and the as_nanos function does the corresponding multiplication & addition to convert, every time I extract the nanoseconds from the duration offset (which is at least once per measurement that governor does).

Anyway, to make this more concrete, I made a microbenchmark to measure the time spent doing each of these and the round-trip costs about 818 picoseconds on an M1max machine, almost a whole nanosecond (2.4ns on a Xeon Silver 4114 machine running Linux). This doesn't sound like a lot, but governor's rate-limiting operations take 20ns per, and so that conversion adds a pretty hefty chunk of overhead!

@tobz
Copy link
Member

tobz commented May 25, 2023

Ah, sorry, right: I had forgotten that Duration is holding seconds/fractional nanoseconds. 🤦🏻

Short answer to the question as posed in the title: yes. Let's call it delta_nanos, though.

The move to exposing Instant/Duration was originally to avoid people using the values as implied wall-clock times, the same as stdlib does... but since Duration can be boiled down to the whole nanoseconds, while Instant can never be converted to a wall-clock time... I think we can safely do the same, albeit with new methods.

Might take a few days to whip this up -- time is sparse for me lately -- but I'm also open to a PR that works as described above.

@antifuchs
Copy link
Contributor Author

OK, that sounds like a good path forward! For governor, I was considering making a crate with its nanosecond-scale duration type that's much cheaper to pass around than std Duration, so now might just be the time to do it (or maybe there's one out there already!), I'll go look around if I can find something or publish what I've got.

@tobz
Copy link
Member

tobz commented May 25, 2023

I'd say, generally, I wouldn't be looking to introduce another type to quanta, especially one from an external crate. I'd only want to go as far as introducing Clock::delta_nanos(&self, ...) -> u64 and calling it a day.

@antifuchs
Copy link
Contributor Author

Fair enough! That's an easy change, then (:

antifuchs added a commit to antifuchs/quanta that referenced this issue May 25, 2023
This addresses metrics-rs#84 and
should results in a benchmark that's 2ns faster (on a m1 max macOS
machine) than the corresponding Duration-converting benchmark.
@tobz
Copy link
Member

tobz commented Sep 18, 2023

Closing as this has been long since taken care of.

@tobz tobz closed this as completed Sep 18, 2023
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

No branches or pull requests

2 participants