as_millis function on std::time::Duration

[paezao]

Hello everyone!

I have an open issue about it: issue

Cheers

[ticki]

Rendered.

[sfackler]

Is this actually how this should behave? Will users typically only want the subsecond portion of the duration in milliseconds or the entire duration in milliseconds?

[paezao]

Well there could be uses for both I guess but mostly the entire duration.

[ticki]

I think the usecase for the whole duration would be the main one. Another RFC should add a way to construct a subsec duration from the duration.

[RalfJung]

The RFC should be clearer about the exact behavior of as_millis, in two cases:

• What happens if the nanoseconds are not divisible by 1000? I assume it should behave like as_secs, i.e., it should be specified to return the number of whole milliseconds. This is also the consequence of the suggested implementation, which however only returns sub-second milliseconds.
• What happens in case of an overflow -- i.e., in case the number of whole milliseconds is larger than u64, which can easily happen? The function has to either wrap, panic, or return an Option<u64>, all of which are suboptimal. I suspect that this is the reason why no such function exists yet.

[main--]

A serious drawback of the current design is that it doesn't match the behavior of as_secs even though the name is similar. The existing API makes this quite awkward as it's tailored to Duration's internal representation of (secs, nanos).

One example to look at is .NET's TimeSpan which I consider quite well-designed. It's convertible to days, hours, minutes, seconds and milliseconds. The direct getters (e.g. TimeSpan.Minutes) only return the corresponding value up to the next bigger kind (so minutes only range from 0 to 59). There are also Total* versions for when you need the whole duration in a certain unit, but because that would cause both overflows and rounding (@RalfJung's two points) if integers were used, the API instead returns floating-point values. This neatly sidesteps both problems!

Another possible approach would be to offer incredible flexibility by extending the existing API (as_*, sub*_*) to all the different units. However, .NET's approach is probably much closer to users' typical needs than this.

In any case, it may be beneficial to move these utility functions away from the basic Duration type to keep it as simple and obvious as it is right now and avoid the potential confusion with the existing as_secs.

Another thing to consider is that all of this could be developed as a library and merged into std once it's mature enough.

[aturon]

cc @wycats

[tbu-]

I believe as would be the wrong prefix for this conversion. It's not a lossless conversion and it's not free. https://doc.rust-lang.org/style/style/naming/conversions.html

[alexcrichton]

🔔 This RFC is now entering its week-long final comment period 🔔

[alexcrichton]

I personally find it somewhat odd that we have a from_millis constructor function for Duration but not any form of accessor in terms of milliseconds. There are a number of questions, however, with a function like this:

• First up, what does it semantically return? As specified in this RFC it's actually more appropriate to name as subsec_millis as it seems to have similar semantics to subsec_nanos. I agree with @sfackler, however, that this probably isn't what you want most of the time.
• If the whole duration is returned as milliseconds, however, what's the return type? u64 is too small to return everything, and Option<u64> is unfortunately unergonomic.
• Are the "round down" semantics as specified in the RFC the most useful? I would expect most timing applications to actually round up instead.
• Should the API panic anywhere? Would help alleviate return types, but is somewhat unidiomatic to use judiciously.

I feel that there are a few too many open questions at this time to move on this API just yet, but @main--'s comment about .NET seems like a good place to draw inspiration from!

[aturon]

I agree with the questions others have raised about the semantics here -- the RFC needs much more detail, and in particular needs to address the overflow concern directly. I don't think this RFC is ready for acceptance as-is, but I do think that we should add this functionality in some form eventually.

(As a sidenote, I think we may want to consider renaming as_secs to whole_secs or something more clear like that.)

[trolleyman]

I think that the option of returning u64 and panicing on overflow is fine. This is because once you do the calculations I feel like u64 is more than large enough for normal durations.
9,223,372,036,854,775,807 / 1000 / 60 / 60 / 24 / 365 ≈ 292,471,208 years

[tbu-]

We could also call this method to_millis_saturating and return the maximum if it would overflow. This would generally be fine I guess (think sleep, timeouts, etc.).

[belisarius222]

I agree with @trolleyman that an as_millis (or whatever name is eventually chosen) should return a u64 and panic on overflow. That way, cosmology researchers estimating the age of the universe won't get incorrect values, and the rest of us won't have to think about that particular edge case.

I also like that .NET style of accessors that @main-- described. Sounds elegant and easy to work with.

[ticki]

I disagree. Instead, it should have a overflow check when debug_assertions is on.

[ticki]

I think @trolleyman's case actually supports the position of not having a overflow check, since it is extremely unlikely, and will pratically never happen.

[belisarius222]

Hmm good point.

[tbu-]

@belisarius222 as is the wrong name anyway, it's not a lossless and free conversion.

@ticki I can see one motivation for not having an overflow check: Performance. As I don't think performance is of utmost relevance here (see e.g. the + methods of Duration), we should try to use a more correct solution, for which I see two, either saturating or panicking.

If we want to omit the overflow check, we need a motivation. :)

[ticki]

If we want to omit the overflow check, we need a motivation. :)

1. Consistency: Rust use overflow checks when debug_assertions is enabled, omitting them when in release mode. I would find it odd if this symmetry didn't apply to Duration as well.
2. Performance: This is not critical, but it is still a minor gain. Saying that this is simply a too small gain would be totally valid, however applying the same reasoning to all small performance gains, can, in the end, lead to a noticable loss. In other words: the importance of microoptimizations is often underestimated.

Overflow is pratically unreachable in this case, and will likely only be found in testing scenarios. I believe that the best solution would be having overflow checks enabled in debug mode (debug_assertions) only.

[tbu-]

@ticki I don't think we can make a consistency argument here. Rust only tries to do unexpected things if it matters for performance, it's not that it makes sense for this method to wrap around.

[alexcrichton]

The libs team discussed this RFC in triage today and the decision was to close. As written we decided that we probably don't want a function called as_millis that acts like subsec_millis, but more broadly there definitely seems to be utility in getting some notion of milliseconds out of a Duration. Exactly what this does and how it does it, however, was concluded is best a topic left to a future RFC.

In the meantime @sfackler has started developing a time2 crate which acts a similar role to the net2 crate as a breeding ground for extensions to the std::time crate in the standard library. Currently there is an as_millis function to serve the purpose of converting most of an entire duration to milliseconds. In the long run we may wish to move some of these utilities into std::time, but that should likely all be considered as one whole RFC.

Thanks again though for the RFC @paezao!