libstd’s Duration refactor

[1-more]

This commit changes the internal representation of Duration from

pub struct Duraion {
    secs: i64,
    nanos: i32
}

to

pub struct Duration {
    millis: i64, // Milliseconds
    nanos:  i32, // Nanoseconds, |nanos| < NANOS_PER_MILLI
}

[aturon]

Thanks for the PR, @1-more! Would you mind outlining the tradeoffs with this new representation and why the choice is being made?

[1-more]

That's easy!

Show me your data structures, and I won't usually need your code. This is not the case with the old Duration representation:

• It has a field named secs, but in code it is restricted to hold values from i64::MIN -1 to i64::MAX milliseconds. Otherwise num_milliseconds() method, which is heavily used throughout Rust's code, overflows.
• nanos field type is i32, but it is restricted to hold only positive values.

I think that the new representation is more adequate for our purpose:

• millis field is to hold any i64 value with no num_milliseconds() method overflow.
• nanos field is to have both positive and negative values.

And last but not least, while our old representation is meant to be compatible with C like Timespec (see this, the new one is easily convertible to that.

[huonw]

For reviewers, etc: previous discussion.

[1-more]

Yeah!

According to previous discussion, the best definition of Duration type should look like:

pub struct Duraion {
    pub ticks: i64, // A single tick represents 100 nanoseconds
}

That is the way .NET and Joda-Time go. It gives the best performance. It fits to alignment rules. The full range of values is large enough etc. But it takes nanoseconds away.

Nobody knows do we really need them. At the moment, the only place in Rust code that uses Duration::nanoseconds and Duration::num_nanoseconds() is this file. It also the only one that uses C like Timespec.

The discussion is still in progress. My commit respects Timespec and nanos, but makes the Duration type a bit more consistent.

[huonw]

This derived Rand is incorrect: an arbitrary random i32 x is highly likely to have |x| >= NANOS_PER_MILLI violating the restriction on nanos.

[huonw]

This can still overflow, so the mention of when None is returned should be maintained/updated.

[huonw]

Hm, could you explain why the change made here is neither of the suggestions from the previous discussion?

At the moment, the only place in Rust code that uses Duration::nanoseconds and Duration::num_nanoseconds() is this file

The stdlib/compiler is not a particularly good place to look for uses of specific functions. This repo will not care or need many functions: they are mainly designed for use by others. A global search of github is a slightly better way to measure the prevalence of a function.

[huonw]

(To be clear, I don't necessarily think this design is bad, it was just a little surprising to me to follow that discussion and then see something else implemented, so a bit of background motivation for the change would be nice.)

[1-more]

Well, as I've said before: the discussion is still in progress. And at least one file exists that needs Duration::nanoseconds and Duration::num_nanoseconds(). I don't want to break someone else's code ...

As for is_digit method includes non-ASCII digit sequences, I have no idea at the moment. Any advice?

[huonw]

Well, as I've said before: the discussion is still in progress

Sure, the discussion is in progress, but this PR seems to be ignoring that discussion entirely. I don't think the choice here is bad, per se, but I would like to see some justification for it, i.e. why we should change Duration to this format rather than leaving it as is or changing it to one of the proposals in the discuss thread (since apparently they are the best format).

(I see you've justified why we shouldn't leave it as-is above, but the latter is still missing: why is ticks: i64, nanos: i8 less appropriate?)

As for is_digit method includes non-ASCII digit sequences, I have no idea at the moment. Any advice?

Just do normal comparison: '0' <= c && c <= '9'.

[1-more]

ticks: i64, nanos: i8 vs millis: i64, nanos: i32?

There is no big difference between them. It's 3 bytes less vs 3 bytes more choice, that is nothing due to alignment rules. But in second case the full range of values is larger. It might be important for someone.

In fact, I dislike both and wish the third case (ticks: i64) to be.

[1-more]

I found one more reason why ticks: i64, nanos: i8 (or even ticks: i64) is less appropriate:
num_microseconds method will always return a valid i64 value. I mean we should change method signature from:

pub fn num_microseconds(&self) -> Option<i64>

to

pub fn num_microseconds(&self) -> i64

But it might to be a compatibility issue. Or leave it as is, then it'll be inconsistent.