[SERVER-47709] Add function to coerce numbers to Duration types Created: 22/Apr/20  Updated: 29/Oct/23  Resolved: 22/Jan/21

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: 4.9.0

Type: New Feature Priority: Major - P3
Reporter: Benjamin Caimano (Inactive) Assignee: Billy Donahue
Resolution: Fixed Votes: 0
Labels: servicearch-wfbf-day
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Backwards Compatibility: Fully Compatible
Sprint: Service arch 2020-05-04, Service arch 2020-05-18, Service arch 2020-12-28, Service Arch 2021-02-08
Participants:

 Description   

Our Duration types expect signed integral types upon construction. However, occasionally, our time data is in double or unsigned integer form. We should make functions to intentionally coerce these numbers into durations.



 Comments   
Comment by Billy Donahue [ 22/Jan/21 ]

Added deduceChronoDuration function. To make full use of it we would need some fixes to mongo::duration_cast. But we can rely on a 2-step duration_cast chain in the meantime.

Comment by Githook User [ 22/Jan/21 ]

Author:

{'name': 'Billy Donahue', 'email': 'billy.donahue@mongodb.com', 'username': 'BillyDonahue'}

Message: SERVER-47709 add deduceChronoDuration to duration.h
Branch: master
https://github.com/mongodb/mongo/commit/332df3bdd7e6d5f7f9a09c9bd0861f5370bc667c

Comment by Billy Donahue [ 19/Jan/21 ]

Code review with JUST the deduceChronoDuration function and its tests.
To really make full use of it we'd need the mongo::duration_cast fixes discussed above.

https://mongodbcr.appspot.com/750430046/

Comment by Billy Donahue [ 09/Dec/20 ]

I've resurrected and rebased my old branch in which I got this in pretty good shape a while ago.

It would be a big improvement for mongo::Duration.

It's almost ready to review but we will have to think of more really evil torture tests for it.

I've self-bikeshedded to deduceChronoDuration.
To interpret as seconds, you can leave out the Period ratio parameter.

These produce std::chrono<Rep, Per> with the specified period but a deduced Rep that matches the numeric argument type.

    deduceChronoDuration(valSecs);
    deduceChronoDuration<std::milli>(valMillis);
    deduceChronoDuration<std::micro>(valMicros);

Once you have these, duration_cast can, with maximal precision, do a correct conversion.
The important thing is to not change Rep too early in the conversion chain.

PoC:
https://github.com/BillyDonahue/mongo/commits/SERVER-47709

Comment by Billy Donahue [ 08/May/20 ]

Our mongo::Duration<Period> types can't use arbitrary Rep like std::chrono::duration<Rep,Period> can. But we do have a mongo::duration_cast that accepts any std::chrono::duration. This is good, but it isn't general enough. It converts to our Duration rep type and then does scaling. I don't think it works well with chrono::duration<Float,Period>.

I recommend two things to fix this problem.

  • First:

Fix our mongo::duration_cast to be more useful and correct for any std::chrono:duration. We could just use the std::chrono::duration_cast logic, but we want our overflow and narrowing protection, so we'll have to rewrite it a little. But it's open source, and we can just go grab it and change it.
https://github.com/llvm/llvm-project/blob/5e74cf29991c498de8905f039f1b0bbfc715acdc/libcxx/include/chrono#L880-L946

  • Second:

Make it easy to make a std::chrono::duration<Rep,Period> with deduced Rep.
It would be nice if std::chrono::duration had a deduction guide for this but whadda gonna do, right? Recommend new deducing factory function, something like:

template <typename Rep, typename Per=std::ratio<1>>
auto makeChronoDuration(Rep rep, Per={}){
    return std::chrono::duration<Rep, Per>(rep);
}

  • Combining these two orthogonal primitives gives us what we want.

// Doesn't matter which arithmetic type T is. Could be double.
T valSecs;    // type-unsafe seconds
T valMillis;  // type-unsafe milliseconds
T valMicros;  // type-unsafe microseconds
 
LOGV2(0,
      "Waited a while",
      "d1"_attr = duration_cast<Milliseconds>(makeChronoDuration(valSecs)),
      "d2"_attr = duration_cast<Milliseconds>(makeChronoDuration(valMillis, std::milli{})),
      "d3"_attr = duration_cast<Milliseconds>(makeChronoDuration(valMicros, std::micro{})));

https://gcc.godbolt.org/z/SVyy8Y

I don't recommend adding members to mongo::Duration.

I don't think we should combine the two steps into a convenience function.
It's good to have the duration_cast and makeChronoDuration calls directly represented, and the explicit composing syntax will suggest flexibility to maintainers.

Generated at Thu Feb 08 05:15:00 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.