Page MenuHomePhabricator

Add Chrono.h - std::chrono support header
ClosedPublic

Authored by labath on Oct 9 2016, 7:19 PM.

Details

Summary

std::chrono mostly covers the functionality of llvm::sys::TimeValue and
lldb_private::TimeValue. This header adds additional utility functions, which
make the usage of the library and porting code from TimeValues easier:

  • conversion between std::chrono types and legacy C types (time_t, struct timeval, struct timespec).
  • string converision
  • a TimePoint typedef which defines a system_clock timepoint with nanosecond precision.

Rationale behind some of the design decisions:

  • precision of system_clock is implementation defined - using a well-defined precision helps maintain consistency between platforms, makes it interact better with existing TimeValue classes, and avoids cases there a time point is implicitly convertible to a specific precision on some platforms but not on others.
  • system_clock::to_time_t only accepts time_points with the default system precision (even though time_t has only second precision on all platforms we support). To avoid the need for explicit casts, I have added a toTimeT() wrapper function. toTimePoint(time_t) was not strictly necessary, but I have added it for symmetry.
  • the duration version of toTimeVal() only accepts durations with microsecond precision (forcing the user to explicitly cast if he has a nanosecond duration), but the time point version, casts away the precision implicitly. The rationalle behind that is that for time points the extra precision is not likely to be important, while one may concievably care about the extra precision for durations.

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 74090.Oct 9 2016, 7:19 PM
labath retitled this revision from to Add Chrono.h - std::chrono support header.
labath updated this object.
labath added reviewers: zturner, mehdi_amini.
labath added a subscriber: llvm-commits.
zturner edited edge metadata.Oct 9 2016, 7:22 PM

Did you forget Windows/Chrono.inc?

labath added a comment.Oct 9 2016, 7:32 PM

I don't have that written yet :) - I'll do that on Tuesday when my hands on a windows machine (I also may need to adjust the includes a bit to make sure we have timespec and timeval defined on all platforms). The general interface should be complete and working (currently I'm testing on osx) though, and I'm looking for a first round of feedback.

I am considering a somewhat different approach now. timespec does not seem to be defined on windows at all, and timeval only exists to support select() in the bsd socket emulation layer. I don't think llvm should be exposing such a platform-specific interface. A better solution would be to identify things in lldb which need these functions, lower them into llvm and wrap in a platform-independent interface which speaks std::chrono (then the conversion utilities can remain in unix-specific code). This will require more cleanups on the lldb side though.

I'd still like to commit this without the part that deals with the fancy conversions - even without them, this supersedes llvm and lldb TimeValue, and replacing TimeValue with it seems pretty easy (I am almost done with that locally).

labath updated this revision to Diff 74144.Oct 10 2016, 10:27 AM
labath edited edge metadata.

Summary of changes:

  • remove code dealing with system types (timespec, timeval)
  • Switch TimeValue to use TimePoint under the hood for some things to avoid duplication
  • Add (still untested) windows implementation - I've removed the "old mingw" support code present in TimeValue - the last (unsuccessful) attempt at that was three years ago - we'll see if we have better luck this time. I've followed TimeValue's patter here, but the two implementations have a lot of code in common, and it might be worth refactoring it a bit.
labath updated this revision to Diff 74697.Oct 14 2016, 8:52 AM

Sorry about the delay, I only got to check the windows version today. This
update actually makes it compile now. I've tested this on mac, osx, and windows
(with msvc) and the tests pass.

Could you take a look and let me know what you think?

mehdi_amini edited edge metadata.Oct 14 2016, 9:11 AM

Looks fine to me overall! Thanks.

unittests/Support/Chrono.cpp
36 ↗(On Diff #74697)

I initially thought this test (StringConversion ) would be doing something else (TimePoint string conversion), but both are valuable so what do you think about another test like:

  auto Now = system_clock::now();
  
  std::string S1;
  {
    raw_string_ostream OS(S1);
    OS << system_clock::now();
   }

  TimePoint T(Now);
  std::string S2;
  {
    raw_string_ostream OS(S2);
    OS << T;
   }
   ASSERT_EQ(S1, S2);
}

(I don't think you added coverage for raw_ostream &operator<<(raw_ostream &OS, TimePoint TP) {, it is likely covered by the TimeValue tests (if any), but if we remove TimeValue at some point we may lose the coverage.

labath added inline comments.Oct 14 2016, 11:51 AM
unittests/Support/Chrono.cpp
36 ↗(On Diff #74697)

TimePoint is just a typedef for a pre-existing STL type that is implicitly constructible from the result of system_clock::now(). So, OS << system_clock::now() will actually invoke the raw_ostream &operator<<(raw_ostream &OS, TimePoint TP). The only difference between OS << system_clock::now(), and OS << TimePoint(system_clock::now() is that in the latter, the conversion operation is explicit (i.e., the newly introduced operator<< will have coverage even when TimeValue is removed, which I plan to do soon).

I can add the test you mentioned, but I don't think it is that useful, as it will just compare two invocations of the same function. I can also add an explicit TimePoint cast to the existing test, if you think that will make things clearer.

mehdi_amini added inline comments.Oct 14 2016, 12:04 PM
unittests/Support/Chrono.cpp
36 ↗(On Diff #74697)

I missed that the conversion was implicit here.

zturner added inline comments.Oct 17 2016, 9:44 AM
include/llvm/Support/Chrono.h
26–27 ↗(On Diff #74697)

I'm not sure if it's appropriate to have a generic name like TimePoint (which indicates it should be used for any and all time calculations) be forced to a specific resolution. A couple of possibilities would be:

  1. Call it TimePointNano
  2. making it an alias template (e.g. template<typename R> using TimePoint = time_point<system_clock, T>)
30 ↗(On Diff #74697)

Any reason this function shouldn't be templated to work with arbitrary resolutions?

Also, I would use LLVM_ATTRIBUTE_ALWAYS_INLINE instead of the inline keyword.

37 ↗(On Diff #74697)

Same.

include/llvm/Support/TimeValue.h
117 ↗(On Diff #74697)

I might be missing something, but PosixZeroTimeSeconds is defined in a cpp file but you're using it in a header file. And it's not forward declared anywhere?

lib/Support/Windows/Chrono.inc
23 ↗(On Diff #74697)

The windows and non-windows versions appear identical except for the call to _localtime64_s vs localtime_r. This is a case where the implementations are similar enough that I think it does more harm than good to separate the files like this. I would put one function in Chrono.cpp with a preprocessor directive to select either _localtime64_s or localtime_r.

Responses to comments inline. I will upload a new version of the code tomorrow.

include/llvm/Support/Chrono.h
26–27 ↗(On Diff #74697)

I wanted to make the usage as frictionless as possible (and TimeValue already used nanoseconds precision and noone seemed to be bothered by that), but I do see your point. How about we go for the second option, but we have a default template parameter, so people don't have to specify precision unless they actually need to. I have two counter-proposals:

  • template<typename D = nanoseconds> using TimePoint = time_point<system_clock, D>, with the usage being: TimePoint<std::chrono::microseconds>
  • template<typename R = nano> using TimePoint = time_point<system_clock, duration<XXX, R>>, with the usage being TimePoint<std::micro>

I'd tend towards the second option as it is shorter if you don't import std::chrono namespace, but I don't care too much, as I don't anticipate much usage with a custom precision.

30 ↗(On Diff #74697)

All (reasonable) time_points are implicitly convertible to a time_point with nanosecond precision, so this will already work with all of them, without the need for templating. The only difference I see is that a time_point with second precision will probably have higher range than the nanosecond one, but I don't think we ought to be relying on anywhere anyway.

37 ↗(On Diff #74697)

I suppose we could return a time_point with second precision, as that is all that's stored in a time_t anyway. I don't think we need templates, because of implicit conversions.

include/llvm/Support/TimeValue.h
117 ↗(On Diff #74697)

It's declared in this file, line 375.

lib/Support/Windows/Chrono.inc
23 ↗(On Diff #74697)

Sounds like a good idea.

zturner added inline comments.Oct 17 2016, 11:48 AM
include/llvm/Support/Chrono.h
26–27 ↗(On Diff #74697)

I agree, it might not ever be used, but who knows. The second usage looks fine to me.

37 ↗(On Diff #74697)

Ok, as long as there are implicit conversions then no worries. Can you add a unit test that exercises one of the implicit conversions?

labath updated this revision to Diff 74971.Oct 18 2016, 3:42 AM
labath edited edge metadata.

Summary of changes:

  • Made TimePoint a template. User can specify the desired precision, but the default will be nanoseconds. I decided to go with the first proposal as there is to typedef for std::ratio<1>. If one wants to override precision, he should use the duration types (std::chrono::seconds).
  • Added tests that verify that: a) result of toTimePoint can be stored in any timepoint with less-than-second precision; b) toTimeT accepts timepoints with more-than-nanosecond precisions
  • use LLVM_ATTRIBUTE_ALWAYS_INLINE for the tiny wrapper functions
  • move Chrono.inc files back into Chrono.cpp
labath updated this revision to Diff 74993.Oct 18 2016, 6:50 AM
  • remove dead code I accidentaly left it
  • Make the TimeValue conversion constructor a template. Otherwise the compiler would not find the implicit conversion sequence TimePoint<X> -> TimePoint<> -> TimeValue, and the only reason I am having that constructor is to make piecewise migration easier.
zturner accepted this revision.Oct 18 2016, 9:25 AM
zturner edited edge metadata.

Feel free to commit after addressing the comments below.

lib/Support/Chrono.cpp
1 ↗(On Diff #74993)

lib/Support/Chrono.h

28 ↗(On Diff #74993)

I believe std::time_t still works here (and moreover that it's incorrect as written, in the case that _USE_32BIT_TIME_T is defined. This can probably be raised out of the #if

32–34 ↗(On Diff #74993)

I would probably write this as:

#if defined(LLVM_ON_WIN32)
#else
#endif
This revision is now accepted and ready to land.Oct 18 2016, 9:25 AM
This revision was automatically updated to reflect the committed changes.