ACP: Unchecked construction of core::time::Duration

[jmillikin]

Proposal

Problem statement

Constructing a core::time::Duration with sub-second resolution currently forces the programmer to choose between two unappealing options:

• Call Duration::new(), which is allowed to panic.
• Call Duration::from_secs() and then saturating_add() the nanoseconds, which is relatively expensive.

This proposal would add an unsafe constructor that performs no checking or math.

Motivation, use-cases

When interacting with C APIs that consume or return a struct timespec, it would be useful to have free conversion to core::time::Duration.

When writing code that is not allowed to panic, it would be useful to have a way to construct core::time::Duration values from known-good inputs.

Solution sketches

pub const unsafe fn new_unchecked(seconds: u64, subsec_nanos: u32) -> Duration {
    Duration { secs: seconds, nanos: subsec_nanos }
}

Links and related work

n/a

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

[jhpratt]

I would use this API if it existed for conversions provided by the time crate.

[the8472]

• Call Duration::new(), which is allowed to panic.
• Call Duration::from_secs() and then saturating_add() the nanoseconds, which is relatively expensive.

This proposal would add an unsafe constructor that performs no checking or math.

Well, that could also be solved by a new_checked() -> Result<Duration, > that'd only have to do a comparison.

[jmillikin]

A new_checked() function wouldn't help much, because to get at the result you'd need to inspect the error (and potentially panic).

fn durations() {
  let x = time::Duration::new(100, 0); // static analysis rejection: call to function that might panic

  let y = time::Duration::new_checked(100, 0).unwrap(); // ditto
}

[the8472]

A new_checked() function wouldn't help much, because to get at the result you'd need to inspect the error (and potentially panic).

For new(100, 0) you can use from_seconds. And Result lets the caller choose the error handling strategy can bubble it up, they can fall back to a default value, they can try to sanitize it or they can choose to panic.

For arbitrary inputs, how are you going to ensure correctness without any checks at all?

I.e. where does one get inputs that are already correctly split into seconds (bounded) nanos from without doing some checking and error-handling? Apparently not even operating system APIs can get their own guarantees right and need to be sanitized: rust-lang/rust#108277

Note that getting these values wrong is instant UB because the nanosecond field has range information that is used in optimizations. C's struct timespec doesn't make the same guarantees to the compiler, so Rust has stricter requirements here.

[jhpratt]

A quick search of the uses where I am constructing std::time::Duration from a time::Duration reveals two instances where I would unquestionably use unchecked construction.

• time::Duration::unsigned_abs — Can trivially be unchecked, as time already guarantees the internal (signed) value is in a certain range.
• TryFrom<time::Duration> for std::time::Duration — If both the seconds and nanoseconds conversion succeed from the signed values, then it is guaranteed that the resulting std::time::Duration is valid.

I haven't checked the assembly resulting for these, but I suspect that it is not optimized out as rustc doesn't know about the validity requirements of time.

[dead-claudia]

@jmillikin For a result-returning Duration factory, you could still use the (unsafe) .unwrap_unchecked() if that's truly what you need. And any level of optimization should still lower it to a simple Duration { secs, nsecs } struct creation.

[the8472]

We discussed this during today's libs-API meeting.

We're more in favor of adding new_checked() -> Result<Duration> rather thannew_unchecked. This is more general since it covers both no-panic code and handling untrusted input.
We could perhaps be swayed if someone had perf numbers to prove that the unwrap_unchecked variant does not optimize well enough for real use-cases.

[the8472]

Marking this as accepted on the assumption that new_checked() is palatable. If you really need new_unchecked() please reopen.

[jmillikin]

This might be my own lack of skill, but I haven't been able to get new_checked().unwrap_unchecked() to optimize to the level of the proposed new_unchecked().

Both of these functions compile to the same assembly on x86_64 with v1.78.0 and -Copt-level=3.

fn duration_new_unchecked(secs: u64, nanos: u32) -> Duration {
    let d = Duration::new_checked(secs, nanos);
    unsafe { d.unwrap_unchecked() }
}

fn duration_new_unchecked(secs: u64, nanos: u32) -> Duration {
    if let Some(x) = Duration::new_checked(secs, nanos) {
        return x;
    }
    unsafe { std::hint::unreachable_unchecked() }
}

duration_new_unchecked:
        mov     edx, esi
        mov     rax, rdi
        cmp     esi, 1000000000
        jb      .LBB0_2
        mov     ecx, edx
        shr     ecx, 9
        imul    rcx, rcx, 281475
        shr     rcx, 39
        imul    esi, ecx, 1000000000
        sub     edx, esi
        add     rax, rcx
.LBB0_2:
        ret

Most of those instructions are to handle the case of the nanos parameter exceeding NANOS_PER_SEC.

It is possible to define a function that does compile to the expected assembly, by forcefully rejecting the possibility of nanosecond overflow -- this is what I would expect the new_unchecked() function to be:

fn duration_new_unchecked_v2(secs: u64, subsec_nanos: u32) -> Duration {
    const NANOS_PER_SEC: u32 = 1_000_000_000;
    if subsec_nanos >= NANOS_PER_SEC {
        unsafe { std::hint::unreachable_unchecked() }
    }
    Duration::new(secs, subsec_nanos)
}

(stub asm, which would presumably be inlined to achieve a free conversion from struct timespec)

duration_new_unchecked_v2:
        mov     edx, esi
        mov     rax, rdi
        ret

[jmillikin]

Tracking issue: rust-lang/rust#125748

Initial implementation: rust-lang/rust#125749

The impl PR contains both new_checked() and new_unchecked(). The checked constructor can be used for most panic-free code, and the unsafe unchecked constructor is "free" for code that is known to contain only nanosecond counts normalized to be < 1 second.

[the8472]

Most of those instructions are to handle the case of the nanos parameter exceeding NANOS_PER_SEC.

The assembly is panic-free. It doesn't show that the few extra, non-taken instructions affect any real applications.

[the8472]

Ah, your PR is doing the extra carry, yes, an unwrap_unchecked won't eliminate that. I thought were were talking about one that had the nanos being in range as requirement and returned a None otherwise but that was not clearly discussed.

[jmillikin]

Ah, yeah, I thought it would be confusing if new_checked() returns None in cases where new() returns a valid value. A direct replacement of panic!() with return None seemed best.

If the implementation looked like this:

fn new_checked(secs: u64, subsec_nanos: u32) -> Option<Duration> {
    if subsec_nanos >= NANOS_PER_SEC {
        return None;
    }
    Some(Duration { secs, nanos: unsafe { Nanoseconds(subsec_nanos) } })
}

Then unwrap_unchecked() would be able to produce the assembly I want.

So I guess the unasked question is which semantics for new_checked() would be least surprising to users.

[the8472]

If confusion with new is a concern it could be named from_secs_and_subsec_nanos_checked() or something like it.

[jmillikin]

Just to clarify: regardless of the presence or signature of new_checked(...) -> Option<Duration>, is there interest from the libs team in an unchecked constructor as requested by this ACP?

If not, then it would be better to remove the "accepted" tag, and I'll close this out. Someone who has a need for it might file an ACP for new_checked().