[libcxx-commits] [PATCH] D65339: Implement std::condition_variable via pthread_cond_clockwait() where available

[Tom via Phabricator via libcxx-commits]

tomcherry added a comment.

I simplified the code a bit more and responded to the comments.  I haven't had a good chance to test this yet, but pushing early for feedback.



================
Comment at: include/__mutex_base:528
+    nanoseconds d = tp.time_since_epoch();
+    if (d > nanoseconds(0x59682F000000E941))
+        d = nanoseconds(0x59682F000000E941);
----------------
EricWF wrote:
> tomcherry wrote:
> > ldionne wrote:
> > > Please pardon my naiveness, but what is that number?
> > It's taken from condition_variable.cpp, added in https://reviews.llvm.org/rGaad745a024a3d0fd573ac9649d4c659e6a713702.  
> > 
> > I tried to keep all of the safety checks around chrono intact.
> Lets give it a name so it makes sense. 
In retrospect, I removed it.  There's no accompanying comments on why it was added originally and there's no obvious reason why it would be needed.


================
Comment at: include/__mutex_base:18
 
+#if defined(_LIBCPP_HAS_COND_CLOCKWAIT)
+#include <time.h>
----------------
EricWF wrote:
> Can't we include `time.h` unconditionally?
I thought it would be better to not include it if not needed, but I'll make it unconditionally included.


================
Comment at: include/__mutex_base:311
 
+    template <class _Duration>
+        _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
----------------
ldionne wrote:
> EricWF wrote:
> > This deduction is wrong. Either you deduce the entire time point, like we were originally, or you use the timepoint and duration specified by the clock.
> > 
> > You don't specify the clock and deduce the other parameters
> > 
> > Also, fewer overloads is better. We want to declare a  _single_ public overload. We can do the magic behind the scenes .
> > This deduction is wrong. Either you deduce the entire time point, like we were originally, or you use the timepoint and duration specified by the clock.
> 
> Why? I was the one to suggest this in the last diff, because we otherwise needed SFINAE and the resulting code was a lot more complex than it needed to be. What's wrong with this implementation?
The latest patchset has it back to the original public interface.


================
Comment at: include/__mutex_base:357
        chrono::time_point<chrono::system_clock, chrono::nanoseconds>) _NOEXCEPT;
+#if defined(_LIBCPP_HAS_COND_CLOCKWAIT)
+    void __do_timed_wait(unique_lock<mutex>& __lk,
----------------
EricWF wrote:
> This can likely be the same implementation as  above.
The new patchset does this all with the one public interface.


================
Comment at: include/__mutex_base:474
+    steady_clock::time_point __c_now = steady_clock::now();
+#if defined(_LIBCPP_HAS_COND_CLOCKWAIT)
+    typedef time_point<steady_clock, duration<long double, nano> > __c_tpf;
----------------
EricWF wrote:
> This section seems like it can be generalized to work for both halves of this ifdef.
> Having the same code tested in all cases is always better than preprocessor branches.
> 
> Forgive me if I'm missing something.
I'm not sure it can.  The first branch does the overflow checking with steady_clock and the second does it with system_clock.  If we have steady_clock available, we'll want to use it.  If we don't have steady_clock available, then we must use system_clock, since all non supported clocks end up calling wait_for() with the difference between their now() and the input time.


================
Comment at: include/__mutex_base:475
+#if defined(_LIBCPP_HAS_COND_CLOCKWAIT)
+    typedef time_point<steady_clock, duration<long double, nano> > __c_tpf;
+    typedef time_point<steady_clock, nanoseconds> __c_tpi;
----------------
EricWF wrote:
> `using` instead of `typedef` in new code if you don't mind.
Will do.


================
Comment at: include/__mutex_base:476
+    typedef time_point<steady_clock, duration<long double, nano> > __c_tpf;
+    typedef time_point<steady_clock, nanoseconds> __c_tpi;
+    __c_tpf _Max = __c_tpi::max();
----------------
EricWF wrote:
> This assumes the resolution of the `steady_clock`, no?
Sort of.  It mostly assumes that steady_clock::now() can be converted into nanoseconds safely, which I think is a reasonable assumption to make.  The old code assumed the resolution of system_clock too, so it's not a regression from there.

I'm happy to hear a better solution, because both this code and the original have never sit well with me.  What we really want is a safe_duration_cast, where we get a ceiling effect if there would be overflow.  Since that is not how std::chrono was created, we're left with two options:

1) This function ends up with signed integer overflow if someone passes milliseconds::max() to it and steady/system_clock.  
2) Work-arounds like this


================
Comment at: include/__mutex_base:510
+void
+condition_variable::__do_timed_wait(unique_lock<mutex>& lk,
+     chrono::time_point<chrono::steady_clock, chrono::nanoseconds> tp) _NOEXCEPT
----------------
EricWF wrote:
> `__lk`, `__tp`, ` __d`, etc. 
> 
> Everything that isn't a part of the public API needs a reserved name.
Got it, done in next patch.


================
Comment at: include/__mutex_base:523
+    typedef decltype(ts.tv_sec) ts_sec;
+    _LIBCPP_CONSTEXPR ts_sec ts_sec_max = numeric_limits<ts_sec>::max();
+    if (s.count() < ts_sec_max)
----------------
EricWF wrote:
> `const` works the same in all dialects.
It's constexpr in the original code, but const is sufficient, so I changed it to that.


================
Comment at: include/__mutex_base:527
+        ts.tv_sec = static_cast<ts_sec>(s.count());
+        ts.tv_nsec = static_cast<decltype(ts.tv_nsec)>((d - s).count());
+    }
----------------
EricWF wrote:
> What does the cast do except silence bugs?
> 
> (Same question above).
The first cast makes sense, we don't know what size time_t is, but we know that if s.count() < ts_sec_max, that we can convert into it.

The second cast isn't needed, removed.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65339/new/

https://reviews.llvm.org/D65339