add support for parsing and printing std::time::Duration
#445
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a slight refactor to reduce a little duplication. I don't mind duplication, but in this case, I was wondering why this block was repeated and had to very carefully scrutinize the code to follow it and make sure there weren't any differences. DRY in this case I think makes it easier to understand.
This brings the code for parsing duration strings (ISO 8601 and friendly) into `SignedDuration` or `Span` into more alignment. There are still some annoying differences. And some bugs I (re)discovered. They are marked as TODOs in this commit. The main motivation here is to support parsing into `std::time::Duration`. A third copy of this code was too unappealing to me, so I'm trying to refactor the code to make it tighter and less redundant.
I think this is a more natural place for them. In particular, I kept going to the top of the file to look for them and then needing to scroll down through a bunch of docs.
... and also a smattering of internal helpers. The `SignedDuration::from_nanos_i128` constructor in particular is analogous to `std::time::Duration::from_nanos_u128` coming to a `std` near you. The rest are just a logical continuation. This also adds some "get units with a remainder" methods that I think will be useful in simplifying some of the duration parsing code.
In particular, `NoUnits` and `NoUnits128`. I dare say that this might be (the very) beginning of getting away from ranged integers. The specific motivation here is that we want to be able to parse `std::time::Duration`. And to do that, we need to be able to parse a `u64`. But before this commit, we were parsing into a `t::NoUnits`, which is an `i64`. And we don't have unsigned ranged integers and I refused to go down that path. So we need to use a `u64` because I refuse to use an `i128` any time we want to parse an integer.
This lets me un-export `Decimal::new`. A small matter.
Specifically, the ranged integer used when parsing fractional values. We'll want to do this in order to switch over to parsing durations into a single shared unified type.
This introduces a new internal helper type, `DurationUnits`, that is used to parse all duration types and formats. It centralizes the logic and trims away a lot of fat. This should reduce code size (although I haven't checked) and also improve perf. Currently, it does significantly improve perf for parsing longer durations and especially for `Span`. It does however slightly regress perf for parsing shorter `SignedDuration` values. However, I think there are a lot of perf improvement opportunities. I'll put those in a subsequent commit. (We still haven't implemented `std::time::Duration` yet. But `DurationUnits` is clearly designed with it in mind. This is one helluva yak shave!) This also fixes a long-standing bug where we couldn't parse `abs(i64::MIN) secs ago`. The `DurationUnits` design was specifically motivated (in part) by this.
The "before" here reflects `master`
(`211a36d5ecddf1aeb9b00648d9d5e631a5420903`) and not the previous
commit:
group master new
----- ------ ---
parse/friendly/longest-time/duration/jiff 1.29 41.9±0.69ns ? ?/sec 1.00 32.4±0.42ns ? ?/sec
parse/friendly/short/duration/jiff 1.14 15.7±0.27ns ? ?/sec 1.00 13.8±0.30ns ? ?/sec
parse/friendly/tiny/duration/jiff 1.23 8.1±0.36ns ? ?/sec 1.00 6.6±0.11ns ? ?/sec
parse/iso8601_duration/long-time/duration/jiff 1.00 27.6±0.32ns ? ?/sec 1.06 29.3±0.27ns ? ?/sec
parse/iso8601_duration/short/duration/jiff 1.05 12.1±0.22ns ? ?/sec 1.00 11.5±0.20ns ? ?/sec
parse/iso8601_duration/tiny/duration/jiff 1.00 6.7±0.10ns ? ?/sec 1.15 7.7±0.09ns ? ?/sec
There are some minor regressions, but also some wins, particularly for
the friendly format.
I mostly focused on optimizing the "turn parsed values into a
`SignedDuration`" part. I used a data dependent optimization that
quickly detects whether we can avoid error handling entirely.
We could probably optimize the general case a bit more too. But this is
enough for now.
This DRY's up some code and tightens up a few things.
This only applies to parsing into a `SignedDuration` since `i64::MIN`
isn't supported by any units on `Span` (including nanoseconds, since it
only supports `(i64::MIN+1)..=i64::MAX` to make negations infallible).
We pull out the u64 prefix parser from the friendly duration and use
that. Since that does the parsing in a single pass and is overall
generally tighter, this also leads to a nice perf improvement on ISO
8601 duration parsing:
group master new
----- ------ ---
parse/iso8601_duration/long-time/duration/jiff 1.14 27.6±0.32ns ? ?/sec 1.00 24.2±0.24ns ? ?/sec
parse/iso8601_duration/long-time/span/jiff 1.21 76.6±0.17ns ? ?/sec 1.00 63.2±0.24ns ? ?/sec
parse/iso8601_duration/long/span/jiff 2.14 101.8±0.21ns ? ?/sec 1.00 47.4±0.24ns ? ?/sec
parse/iso8601_duration/medium/span/jiff 1.71 60.3±0.12ns ? ?/sec 1.00 35.2±0.12ns ? ?/sec
parse/iso8601_duration/short/duration/jiff 1.56 12.1±0.22ns ? ?/sec 1.00 7.8±0.13ns ? ?/sec
parse/iso8601_duration/short/span/jiff 1.50 47.7±0.12ns ? ?/sec 1.00 31.9±0.08ns ? ?/sec
parse/iso8601_duration/tiny/duration/jiff 1.17 6.7±0.10ns ? ?/sec 1.00 5.7±0.03ns ? ?/sec
parse/iso8601_duration/tiny/span/jiff 1.19 33.1±0.08ns ? ?/sec 1.00 27.8±0.10ns ? ?/sec
The results above are compared against current `master`
(`211a36d5ecddf1aeb9b00648d9d5e631a5420903`).
We do something similar like we did for `SignedDuration`: we carve out a fast path that generally lets us quickly skip error checking and what not. Overall comparison with current `master` (211a36d): group master new ----- ------ --- parse/friendly/long/span/jiff 3.26 104.3±0.48ns ? ?/sec 1.00 32.0±0.34ns ? ?/sec parse/friendly/longer/span/jiff 3.18 105.7±0.35ns ? ?/sec 1.00 33.2±0.20ns ? ?/sec parse/friendly/longest-time/duration/jiff 1.26 41.9±0.69ns ? ?/sec 1.00 33.3±0.62ns ? ?/sec parse/friendly/longest-time/span/jiff 3.31 122.1±0.32ns ? ?/sec 1.00 36.8±0.28ns ? ?/sec parse/friendly/longest/span/jiff 3.22 160.5±0.48ns ? ?/sec 1.00 49.8±0.47ns ? ?/sec parse/friendly/medium/span/jiff 2.93 62.3±0.13ns ? ?/sec 1.00 21.2±0.16ns ? ?/sec parse/friendly/short/duration/jiff 1.20 15.7±0.27ns ? ?/sec 1.00 13.0±0.25ns ? ?/sec parse/friendly/short/span/jiff 2.69 49.8±0.19ns ? ?/sec 1.00 18.5±0.04ns ? ?/sec parse/friendly/tiny/duration/jiff 1.08 8.1±0.36ns ? ?/sec 1.00 7.5±0.13ns ? ?/sec parse/friendly/tiny/span/jiff 2.09 34.1±0.11ns ? ?/sec 1.00 16.3±0.08ns ? ?/sec parse/iso8601_duration/long-time/duration/jiff 1.12 27.6±0.32ns ? ?/sec 1.00 24.7±0.22ns ? ?/sec parse/iso8601_duration/long-time/span/jiff 1.28 76.6±0.17ns ? ?/sec 1.00 59.7±0.23ns ? ?/sec parse/iso8601_duration/long/span/jiff 4.36 101.8±0.21ns ? ?/sec 1.00 23.4±0.14ns ? ?/sec parse/iso8601_duration/medium/span/jiff 3.18 60.3±0.12ns ? ?/sec 1.00 19.0±0.11ns ? ?/sec parse/iso8601_duration/short/duration/jiff 1.49 12.1±0.22ns ? ?/sec 1.00 8.1±0.14ns ? ?/sec parse/iso8601_duration/short/span/jiff 2.68 47.7±0.12ns ? ?/sec 1.00 17.8±0.22ns ? ?/sec parse/iso8601_duration/tiny/duration/jiff 1.10 6.7±0.10ns ? ?/sec 1.00 6.0±0.06ns ? ?/sec parse/iso8601_duration/tiny/span/jiff 2.00 33.1±0.08ns ? ?/sec 1.00 16.5±0.08ns ? ?/sec Very happy with this. Especially getting some of the 100ns+ benchmarks down to something much more reasonable is amazing.
BurntSushi
force-pushed
the
ag/fmt-unsigned-duration
branch
from
e74f8b9 to
811d1be
Compare
This is continued progress toward natively supporting `std::time::Duration`. I basically didn't want to duplicate all of the code used to print a `SignedDuration` just so that we could print a `std::time::Duration`. Since we handle the sign before/after most of the meat of printing, we can actually lower a `SignedDuration` to a `std::time::Duration` and focus most of our printing logic from that. There are some papercuts in this commit though. Notably, some casts between `u64` and `i64`, since Jiff's lowest level integer printing routine requires a `u64`. We'll fix that in the next commit. Thank goodness I decided to make `SignedDuration` mimic `std::time::Duration` exactly (except for being signed). That made this change super easy.
... so that we can print `u64` values as-is without suspicious casts. Unsigned integer printing is also simpler, so it's plausible that it's faster too. Although I haven't been careful about benchmarking duration printing. In the next commit, we will finish removing the casts by tweaking fractional printing.
I think when I originally wrote fractional formatting, I was thinking about using precision values greater than `9`. But that never really panned out. And I used signed values because I used signed values everywhere. I think this is the last change we need to be able to feasibly add native `std::time::Duration` support.
This was thankfully as straight-forward as I was hoping!
This was a little hairier than printing, but still overall pretty easy given my previous changes!
This commit writes out the Serde helpers for unsigned durations. This is the pay off for adding the dedicated parsers/printers for a `std::time::Duration`. Previously, we were converting to/from a `SignedDuration`, which fundamentally limited the range of values we could support. Closes #380, Ref #298
BurntSushi
force-pushed
the
ag/fmt-unsigned-duration
branch
from
811d1be to
2c1421f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds support for
std::time::Durationto the ISO 8601and "friendly" duration formats. This includes both parsing and
printing. Moreover, on top of this, Serde helpers for serializing
and deserializing
std::time::Durationhave been added tojiff::fmt::serde::unsigned_duration.This required a fair bit of work to get correct because all of the
parsing machinery was pretty heavily coupled with
i64, which weobviously cannot use with
std::time::Duration(which needs to be ableto parse into a
u64). Moreover, there was a fair bit of work to bedone to avoid duplicating a bunch of parsing logic (particularly in the
"friendly" format).
The new refactoring also fixed some bugs related to parsing integers
at boundaries. For example,
-9223372036854775808spreviously did notwork even though it's a valid
jiff::SignedDuration. See the commitmessages for details.
In the course of this work, I also made parsing durations (of all
types, including
Span) faster, sometimes significantly so:Closes #298, Closes #380