-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
add more integration with std::time::Duration
?
#21
Comments
std::time::Duration
std::time::Duration
?
I don't know how much you've already considered this, but for me the biggest overhead with Span is not the performance overhead but the mental overhead. For example having to deal with potential errors and edge cases caused by the fact that Span can contain calendar units and having to keep in mind that some functions treat 90 seconds and 1 minute 30 seconds as different spans. |
Have you tried writing any code with Jiff yet? If so, would it be possible to share that code and your journey in figuring out how to write it? It's worth noting that you can only get non-uniform units in your spans if you specifically put them there or request them.
I think it is only the PartialEq and Eq impls that do this, and I think I'm going to have to remove them for this reason. (There is an open issue about it.) Is there another place where they are treated differently that I'm missing? |
Nothing substantive, just some small experiments. I've mostly been reading API docs and thinking about how this might work for some projects I've previously worked on. Sidenote: My opinions are probably somewhat by the fact that most of the use cases I'm thinking of work with timestamps, so I would rarely want or need to use calendar units in spans.
True, but since this isn't enforced at the type level I have to manually keep track of how the span was created and if it might have some kind of problematic units (e.g. calendar units when working with timestamps) in it. I suspect this could get quite tricky if the span is created from user supplied data and/or in a different component of the system.
The field accessors (get_seconds, get_minutes, etc) as well as serialization and formatting functions. Though these seem mostly useful for introspection and moving data around, so offhand I can't think of any cases where they could cause trouble like Eq and PartialEq can. |
I think it should be okay? Jiff won't panic in these cases, it will return an error. So if you're adding a
I'm not sure the getters really apply here because they don't give you a And yes, indeed, |
Aye. Any details you can share about your journey would be helpful. Thanks! |
Maybe it will be okay. Hard to say without more practical experience. Depending on what you're building you might not have a user to bubble the error to (e.g. if you're doing some kind of background processing on a server or a daemon service). |
Yeah but there should always be an error channel of some kind. Most of Jiff's API fallible, e.g., due to overflow. |
Still, it would be good to catch errors early. A span with calendar units is effectively invalid in some contexts (e.g. working with timestamps) and in those cases the ability to enforce at compile time that calendar units aren't present seems like a useful thing that would reduce mental overhead. |
I think adding support for But I am really looking for more concrete experiences as opposed to design philosophy. Compile time errors are great, but they must be considered in the context of trade offs. |
I personally favour defining a signed duration type, because of the clunkiness and complexity that an unsigned duration type brings, but I agree that without concrete experience it is hard to properly evaluate the tradeoffs. |
Adding my own (limited) experience porting my hobby sensor data platform to jiff. I have a tui app showing a graph of sensor values from history (can be a year can be a minute). That history length is currently a fn get_data(&mut self,
start: jiff::Timestamp,
end: jiff::Timestamp,
reading: protocol::Reading,
n: usize,
) -> Result<(Vec<jiff::Timestamp>, Vec<f32>), Error>; Now I would like to use that from a function that gets an api.get_data(
jiff::Timestamp::now() - length, // length is a core::time::Duration
jiff::Timestamp::now(),
reading.clone(),
300,
) Alternatively So currently I am converting to millis then to Span, not very pretty: api .get_data(
jiff::Timestamp::now()
- jiff::Span::default().milliseconds(length.as_millis() as i64),
jiff::Timestamp::now(),
reading.clone(),
300,
); Now this could be solved by switching every duration out for a
I hope this was helpful, if there is any way I can assist let me know! |
Yeah I think #21 will help here. And adding a |
I think having the APIs similar to: let duration: Duration = timestamp2.since_duration(timestamp1).unwrap("the difference was negative");
let duration: Duration = timestamp2.until_duration(timestamp1).unwrap("the difference was negative");
let duration: (i8, Duration) = timestamp2.since_duration_signed(timestamp1);
let duration: (i8, Duration) = timestamp2.until_duration_signed(timestamp1);
let duration = Duration::from_secs(1);
let timestamp: Timestamp = timestamp.checked_add_duration(duration).unwrap("overflow");
let timestamp: Timestamp = timestamp.saturating_add_duration(duration);
let timestamp: Timestamp = timestamp.checked_sub_duration(duration).unwrap("overflow");
let timestamp: Timestamp = timestamp.saturating_sub_duration(duration); Should cover majority of use-cases where simpler "Span" might be enough. For services/libraries working with UTC timestamps, above is really what's needed and we don't need to deal with complexities of relative units in the Difference between two timestamps. Usually, negative difference means that something is expired, and do no-op. For cases where we are interested in negative values, one can just use |
Can you share some code that is more complex than you would like when dealing with My main gripe is that I'm not a huge fan of vomiting a new group of four methods for every single possible duration type supported. |
If we omit the relative spans, I think current API is clean and understandable. It's just that we use
Do you foresee any other types for which this would have to be done? |
Right, okay, that's what I'm trying to understand. I laid out what I believe are two buckets of user stories: the first is "I already use It's not the "relative" aspect of
Yes. I mentioned Jiff defining its own signed duration type in the OP. A |
This PR dips our toes into the water of integration with `std::time::Duration`. I do somewhat expect there to be more integration points, particularly in the datetime arithmetic APIs, but this at least paves a path for folks to _correctly_ convert between a `Duration` and a `Span`. The error cases for the new APIs will ensure you can never misuse these APIs. This adds a few new APIs: * `Span::is_positive`, since we already have `Span::is_negative` and `Span::is_zero`. * `Span::signum`, since it's signed and it's sometimes convenient. * `TryFrom<std::time::Duration> for Span`. * `TryFrom<Span> for std::time::Duration` that returns an error if the span is negative or has non-zero units bigger than days. * `Span::to_duration` that requires a relative date and can convert any positive span into a `Duration` (modulo overflow). Partially addresses #21 Fixes #40
This PR dips our toes into the water of integration with `std::time::Duration`. I do somewhat expect there to be more integration points, particularly in the datetime arithmetic APIs, but this at least paves a path for folks to _correctly_ convert between a `Duration` and a `Span`. The error cases for the new APIs will ensure you can never misuse these APIs. This adds a few new APIs: * `Span::is_positive`, since we already have `Span::is_negative` and `Span::is_zero`. * `Span::signum`, since it's signed and it's sometimes convenient. * `TryFrom<std::time::Duration> for Span`. * `TryFrom<Span> for std::time::Duration` that returns an error if the span is negative or has non-zero units bigger than days. * `Span::to_duration` that requires a relative date and can convert any positive span into a `Duration` (modulo overflow). Partially addresses #21 Fixes #40
OK, I've taken my first crack at integration with
These routines aren't difficult for callers to implement on their own, but they're subtle enough with enough failure scenarios that it's definitely worth it for Jiff to provide them for you. And I designed them in such a way that I believe they are impossible to misuse. |
I think we are in both camps, with the first one being the bigger concern :)
I looked at the implementation of how Line 1271 in 3c3b810
Basically, we could see it as anything that can produce seconds/nanoseconds pair could be added to pub fn checked_add(self, span: impl Into<TimestampSpan>) -> Result<Timestamp, Error> Where pub struct TimestampSpan(i64, i32); Then both pub struct TimestampSpan(i64, i32);
impl From<Duration> for TimestampSpan {
fn from(duration: Duration) -> Self {
let seconds = duration.as_secs();
let nanos = duration.subsec_nanos();
TimestampSpan(seconds as i64, nanos as i32)
}
} Now one can use the same set of methods for adding/subtracting both Wdyt? |
@martintmk Yes, I've been thinking about that. (Although I would probably use |
Wouldn't
I checked the usage and it does support rounding directly when calling |
I talked about signed durations further up. This is part of the problem of the API design here.
I don't see how that would help here? Otherwise, see: https://docs.rs/jiff/latest/jiff/_documentation/design/index.html#what-are-the-zonedifference-and-zonedround-types-for |
Here is what I'm currently leaning towards:
I think this will cover everything. It provides nearly maximal integration with std and also provides a way to do "just add nanoseconds please" without going through the chunkier So... What do we call the signed duration type? I suppose the obvious choice is |
Personally, I love this idea and it's exactly what's was missing to me.
Perfect!
Should also cover a lot in #17 I suppose the obvious choice is SignedDuration, but it's kind of a mouthful in my opinion. But maybe that's the best name since it's the least likely to lead to confusion. I like |
Yeah, I think I am likely to go with I'll probably also change the So we'll add direct integration with I think everything above should work. But it will a be a breaking change. I might try to get a slightly tweaked form into a semver compatible release with some deprecations.
I hopefully think the only gotchas are perf related. Otherwise, it should be very hard to misuse. |
There are two different use cases to untangle here.
The first use case is something like this:
The second use case is something like this:
The second use case is perhaps related to #17, but it is somewhat intertwined here, because one (but not the only) obvious way to address that is to satisfy the first use case: we provide alternative APIs that use
std::time::Duration
(hereafter just calledDuration
). This works because aDuration
is itself just a 96-bit integer. And it always represents absolute time. That is, it is logically an integer number of nanoseconds. Therefore, adding aDuration
to something like aTimestamp
should be a very fast operation. Certainly faster than the existingSpan
focused APIs that we have today.At present, I believe it is correct to say that
Duration
appears in the public API exactly zero times. This was not an accident, because I was unsure of how to integrate it. I think there are two challenges:The first is that
Duration
is unsigned. There is some discussion in the issue tracking the addition ofDuration
(andSystemTime
) to the standard library, and the related RFC has this to say:I was on libs (now libs-api) at the time, and I'm not sure if this was a mistake or not. Maybe the world really does need both unsigned and signed durations. The main issue with an unsigned duration type in Jiff is that it isn't nearly as flexible. For example, if you do
ts1.until(ts2)
andts1
refers to a point in time afterts2
, then Jiff will happily return a negative span. But what happens if you want to return aDuration
? What is the result? One choice is to just return the duration between them such thatts1.until(ts2)
andts2.until(ts1)
are equivalent. The other is to return an error. I would probably lean towards "return an error"... This doesn't come up as much with things likets.checked_add(duration)
since there is ats.checked_sub(duration)
, and because it's up to caller in this case to apply the right operation. It isn't on Jiff to interpret the duration as anything other than unsigned.The second problem with
Duration
integration is that it's not clear how to, well, actually integrate it. Does this mean Jiff should growchecked_add_duration
andsince_duration
methods? I think so... And I think this is probably a requirement to satisfying the second use case above, even if we use a different duration type. I do personally find the names with a_duration
suffix quite clunky, but perhaps it's fine because these will be considered "niche" APIs IMO.It would perhaps be possible to satisfy the first use case without satisfying the second while simultaneously not adding a bunch of clunky
_duration
APIs to all of the datetime types. This could be accomplished by providing APIs to convert between aSpan
and aDuration
. So callers would still need to useSpan
, but they would at least have a paved path for usingDuration
.I think the main alternative point in the design space here is for Jiff to define its own absolute duration type that is signed. We'd still need things like
checked_add_duration
andsince_duration
, but the awkwardness with an unsigned duration type would go away. And then once we have our own absolute duration type, we could define conversions to and from std's unsignedDuration
type. And we could deal with the peculiarities of signedness in a very focused and deliberate way.The text was updated successfully, but these errors were encountered: