Implement Mul<u64> and Div<u64> for Duration

[sfackler]

This probably can't merge as-is because it breaks any code that
multiplies or divides a Duration by an unsuffixed literal :(

However, these operations are useful in many contexts (e.g. data
transfer estimation) where a u32 isn't big enough and the logic is
pretty complex, especially for multiplication. Hopefully we can expose
these in some way?

cc @rust-lang/libs

Any ideas on where to go with this functionality, @aturon?

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[alexcrichton]

Hm the breakage with unsuffixed literals indeed seems like a non-starter here.

I wonder if it'd be worth investigating the underlying type inference bug?

[sfackler]

Not sure it's a bug strictly - there's no precedence on which integer type a literal should be if multiple apply and none are i32 - maybe we could do something like picking the smallest type that would fit the value?

[durka]

Can you fix the inference regression by implementing the ops for i32, simply casting the argument to u32 and handing off to the u32 impl?

It seems to work (adds a warning though).

[sfackler]

I'd be a bit uncomfortable with that - some_duration * -3 would compile when it really shouldn't.

[aturon]

@sfackler @alexcrichton

@sfackler is correct that this isn't a bug. We made the decision to allow "input" types to a trait (like type parameters) influence type inference, which is often helpful, but can be brittle when you add new impls.

It's possible that default type parameter fallback could help here, but I don't immediately see a route.

This is a kind of API evolution we certainly should allow, but the interconnecting pieces are really tricky (and the future of even default param fallback is in question). So (1) I will take this example into consideration on the lang design front and (2) meanwhile, we should figure out a different route to gain this functionality.

[alexcrichton]

Bah oh well. An idea, though, perhaps?

impl<T: Into<u64>> Mul<T> for Duration {
   // ...
}

Now that we have a number of Into impls, would that work? The default fallback of i32 doesn't implement Into<u64>, however, so maybe we'd still need suffixes?

[durka]

@alexcrichton I tried plugging that into my test above, it doesn't really solve the problem.

[alexcrichton]

The libs team discussed this PR during triage and the decision was to merge with an inherent method of a name other than mul for now. There are a number of language-level possibilities for solving this eventually, but in the near term it seems prudent to allow this form of functionality at all! We can always deprecate it at a later date if more language features become available.

[sfackler]

I updated the PR to move these to mul_u64 and div_u64 methods, but div_u64 is unfortunately broken. Specifically, this calculation overflows when rhs is large. Does anyone see a nice way of breaking the calculation up so that all of the numbers are small enough?

[ranma42]

Why is this not simply let carry = self.secs % rhs;?

[ranma42]

AFAIK it requires either a muldiv-like operation (which would be the best option at least on x86), or u128 arithmetics (or using BigNum? it definitely looks like overkill) :(
Some references about software integer division can be found here and a good implementation is discussed here, but I would rather avoid putting the generic division algorithm here.

Looks like one use case for rust-lang/rfcs#1504

[alexcrichton]

@sfackler maybe this function can help?

[sfackler]

Ooh, nice, that looks like exactly what's needed! I'll update tonight.

[sfackler]

mul_div_u64 unfortunately doesn't work since NANOS_PER_SEC * rhs is larger than a u64.

[alexcrichton]

Hm can you clarify? The operation we want is carry * NANOS_PER_SEC / rhs which when calling that function will yield:

let q = carry / rhs;
let r = carry % rhs;

q * NANOS_PER_SEC + r * NANOS_PER_SEC / rhs

Which when substituted looks like:

(carry / rhs) * NANOS_PER_SEC + (carry % rhs) * NANOS_PER_SEC / rhs

Given that, where does NANOS_PER_SEC * rhs happen? In theory this wouldn't overflow unless it really does indeed overflow?

[sfackler]

The docs on mul_div_u64 state that it "Computes (value * numer) / denom without overflow, as long as both (numer*denom) and the overall result fit into i64 (which is the case for our time conversions)", since carry % rhs is on the order of the size of rhs

[alexcrichton]

Ok, well I'm basically lost in the weeds of what's going on, is this blocked on landing until that's fixed?

[sfackler]

I think the best approach would probably be to add some intrinsics to have LLVM generate u64 * u64 -> u128 and u128 / u64 -> u64 logic for us. Doing it correctly by hand is really complex in the general case: https://github.com/sdroege/rust-muldiv/blob/master/src/u64_muldiv.rs but really simple on common architectures: https://github.com/sdroege/rust-muldiv/blob/master/src/u64_muldiv_x86_64.rs

[sfackler]

I created a time2 crate that adds these methods and a few others - https://github.com/sfackler/rust-time2. I think it makes more sense for it to bake out there for now and then move a larger set of functionality in later.