We construct our WTF::Lock and WTF::Condition based on ParkingLot. And ParkingLot depends on the platform mutex and condition variable, which are WTF::Mutex and WTF::ThreadCondition. WTF::Mutex is implemented in pthread_mutex_t in POSIX, and CRITICAL_SECTION in Windows. But I don't think we should maintain these platform-specific WTF::Mutex and WTF::ThreadCondition. We should use C++ std::mutex and std::condition_variable. They offer several benefits 1. We can remove bunch of code creating WTF::Mutex and WTF::ThreadCondition. We can just use std::mutex and std::condition_variable directly. 2. Implementation quality is improved by the platform libraries. In POSIX environment + LLVM libc++, std::mutex is implemented with pthread_mutex_t. And std::condition_variable is implemented by pthread_cond_t. So there are no implementation difference. In Windows, std::mutex is implemented with SRW lock. So it offers better performance than CRITICAL_SECTION[1,2]. 3. We can clearly state that WTF::Lock and WTF::Condition are preferred ones. When we have WTF::Mutex and WTF::Lock, it is a bit unclear that which one is better since both are in WTF. But if we have std::mutex and WTF::Lock, we can easily know that WTF::Lock is better one since we additionally have this one in WTF. [1]: https://stackoverflow.com/questions/9997473/stdmutex-performance-compared-to-win32-critical-section [2]: https://msdn.microsoft.com/library/windows/desktop/aa904937(v=vs.85).aspx
Created attachment 330193 [details] Patch
Created attachment 330196 [details] Patch
Comment on attachment 330196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330196&action=review > Source/WTF/wtf/ParkingLot.cpp:601 > + me->parkingCondition.wait_for(locker, std::chrono::duration<double> { (timeout - timeout.nowWithSameClock()).value() }); This is only place we contact with std::condition_variable with std::chrono types. So, we should handle this extra carefully. I think this is correct. And per implementation perspective, this is OK in libstdc++ and libc++. But I would like to ask the opinion of C++ expert about this. The question is simple, what happens if we use, condition.wait_for(locker, std::chrono::duration<double> { std::numeric_limits<double>::infinity() }); OR condition.wait_for(locker, std::chrono::duration<double> { Really Large Double Number });
Comment on attachment 330196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330196&action=review >> Source/WTF/wtf/ParkingLot.cpp:601 >> + me->parkingCondition.wait_for(locker, std::chrono::duration<double> { (timeout - timeout.nowWithSameClock()).value() }); > > This is only place we contact with std::condition_variable with std::chrono types. So, we should handle this extra carefully. > I think this is correct. And per implementation perspective, this is OK in libstdc++ and libc++. > But I would like to ask the opinion of C++ expert about this. > The question is simple, what happens if we use, > > condition.wait_for(locker, std::chrono::duration<double> { std::numeric_limits<double>::infinity() }); > > OR > > condition.wait_for(locker, std::chrono::duration<double> { Really Large Double Number }); If this assumption does not met, we should insert extra careful conversion here. But even if we do this, it is worth introducing this patch because, 1: Such careful conversion already exists in old ThreadCondition::timedWait implementation. We removed it now in this patch. But if it is necessary, we can just insert it here. 2: This is only place we contact with std::condition_variable with std::chrono types. So, if we can handle std::chrono types well here, we can just forget all the chrono types in the other places.
(In reply to Yusuke Suzuki from comment #4) > Comment on attachment 330196 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=330196&action=review > > >> Source/WTF/wtf/ParkingLot.cpp:601 > >> + me->parkingCondition.wait_for(locker, std::chrono::duration<double> { (timeout - timeout.nowWithSameClock()).value() }); > > > > This is only place we contact with std::condition_variable with std::chrono types. So, we should handle this extra carefully. > > I think this is correct. And per implementation perspective, this is OK in libstdc++ and libc++. > > But I would like to ask the opinion of C++ expert about this. > > The question is simple, what happens if we use, > > > > condition.wait_for(locker, std::chrono::duration<double> { std::numeric_limits<double>::infinity() }); > > > > OR > > > > condition.wait_for(locker, std::chrono::duration<double> { Really Large Double Number }); > > If this assumption does not met, we should insert extra careful conversion > here. > But even if we do this, it is worth introducing this patch because, > > 1: Such careful conversion already exists in old ThreadCondition::timedWait > implementation. We removed it now in this patch. But if it is necessary, we > can just insert it here. > 2: This is only place we contact with std::condition_variable with > std::chrono types. So, if we can handle std::chrono types well here, we can > just forget all the chrono types in the other places. It would be best if you used an absolute wait here. pthread_cond lets you do it. If C++ doesn’t, then maybe we should keep using pthread_cond. It seems weird to be introducing relative-absolute time conversions because of a desire to use a different-but-not-better set of locking primitives. At some point, I converted a bunch of code away from std::mutex and condition_variable because: - chrono is terrible. We have had multiple bugs because of it. One time it was because of a buggy implementation of some chrono function on some platform. I believe your code could trigger some bug like this if the timeout was Infinity. I think your code is UB if Infinity is used. - C++ requires you to use unique_lock when waiting, which ia pointless from a functional standpoint. Overall, I would prefer for WebKit to move away from chrono, mutex, and other std:: concurrency stuff, since it’s buggy and poorly designed. Is there some benefit to this change that is greater than these downsides?
Comment on attachment 330196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330196&action=review >>>> Source/WTF/wtf/ParkingLot.cpp:601 >>>> + me->parkingCondition.wait_for(locker, std::chrono::duration<double> { (timeout - timeout.nowWithSameClock()).value() }); >>> >>> This is only place we contact with std::condition_variable with std::chrono types. So, we should handle this extra carefully. >>> I think this is correct. And per implementation perspective, this is OK in libstdc++ and libc++. >>> But I would like to ask the opinion of C++ expert about this. >>> The question is simple, what happens if we use, >>> >>> condition.wait_for(locker, std::chrono::duration<double> { std::numeric_limits<double>::infinity() }); >>> >>> OR >>> >>> condition.wait_for(locker, std::chrono::duration<double> { Really Large Double Number }); >> >> If this assumption does not met, we should insert extra careful conversion here. >> But even if we do this, it is worth introducing this patch because, >> >> 1: Such careful conversion already exists in old ThreadCondition::timedWait implementation. We removed it now in this patch. But if it is necessary, we can just insert it here. >> 2: This is only place we contact with std::condition_variable with std::chrono types. So, if we can handle std::chrono types well here, we can just forget all the chrono types in the other places. > > It would be best if you used an absolute wait here. pthread_cond lets you do it. If C++ doesn’t, then maybe we should keep using pthread_cond. It seems weird to be introducing relative-absolute time conversions because of a desire to use a different-but-not-better set of locking primitives. > > At some point, I converted a bunch of code away from std::mutex and condition_variable because: > > - chrono is terrible. We have had multiple bugs because of it. One time it was because of a buggy implementation of some chrono function on some platform. I believe your code could trigger some bug like this if the timeout was Infinity. I think your code is UB if Infinity is used. > > - C++ requires you to use unique_lock when waiting, which ia pointless from a functional standpoint. > > Overall, I would prefer for WebKit to move away from chrono, mutex, and other std:: concurrency stuff, since it’s buggy and poorly designed. > > Is there some benefit to this change that is greater than these downsides? Yes, I completely agree that std::chrono's overflow unawareness is terrible. And I'm not sure why std::condition_variable requires std::unique_lock for waitxxx methods. The largest benefit is removing large code base in ThreadingPthread.cpp and ThreadingWin.cpp. Especially, removing code in ThreadingWin.cpp is nice. We do not have many contributors in Windows WebKit. So if we can remove Windows specific code, it significantly reduces the maintenance burden. The second tiny benefit is these primitives will be optimized by the platform. Actually, in Windows, std::mutex backend is replaced from CRITICAL_SECTION to SRWLock. I think this is basically the only place to consider about std::mutex and std::condition_variable since we use WTF::Lock and WTF::Condition in other places. Currently, the only place we use WTF::Mutex and WTF::ThreadCondition is here. For example, our WordLock uses std::mutex and std::condition. If we need to use WTF::Mutex/WTF::ThreadCondition more for some reasons, we can create a WTF::Mutex and WTF::ThreadCondition as a wrapper of this std::mutex and std::thread_condition, and put this handling in this wrapper. But I don't think it is required. WTF::Lock and WTF::Condition should be sufficient. Anyway, basically, we can forget about the platform-specific synchronization primitives at all. In C++ std::condition_variable, we have std::condition_variable::wait_until. So we can use absolute time. But I used wait_for here because of the following three reasons. 1. If we use wait_for, we can forget about the kind of times. And if std::chrono::duration<double> works well here, we can just use it here. 2. We do not need to know how to convert our Time to std::chrono::time_point. 3. In libc++, wait_until is a proxy over wait_for. So rather than calculating std::chrono::time_point here, passing duration is more efficient. But maybe, third reason is not much important since the thread is about to sleep.
(In reply to Yusuke Suzuki from comment #6) > Comment on attachment 330196 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=330196&action=review > > >>>> Source/WTF/wtf/ParkingLot.cpp:601 > >>>> + me->parkingCondition.wait_for(locker, std::chrono::duration<double> { (timeout - timeout.nowWithSameClock()).value() }); > >>> > >>> This is only place we contact with std::condition_variable with std::chrono types. So, we should handle this extra carefully. > >>> I think this is correct. And per implementation perspective, this is OK in libstdc++ and libc++. > >>> But I would like to ask the opinion of C++ expert about this. > >>> The question is simple, what happens if we use, > >>> > >>> condition.wait_for(locker, std::chrono::duration<double> { std::numeric_limits<double>::infinity() }); > >>> > >>> OR > >>> > >>> condition.wait_for(locker, std::chrono::duration<double> { Really Large Double Number }); > >> > >> If this assumption does not met, we should insert extra careful conversion here. > >> But even if we do this, it is worth introducing this patch because, > >> > >> 1: Such careful conversion already exists in old ThreadCondition::timedWait implementation. We removed it now in this patch. But if it is necessary, we can just insert it here. > >> 2: This is only place we contact with std::condition_variable with std::chrono types. So, if we can handle std::chrono types well here, we can just forget all the chrono types in the other places. > > > > It would be best if you used an absolute wait here. pthread_cond lets you do it. If C++ doesn’t, then maybe we should keep using pthread_cond. It seems weird to be introducing relative-absolute time conversions because of a desire to use a different-but-not-better set of locking primitives. > > > > At some point, I converted a bunch of code away from std::mutex and condition_variable because: > > > > - chrono is terrible. We have had multiple bugs because of it. One time it was because of a buggy implementation of some chrono function on some platform. I believe your code could trigger some bug like this if the timeout was Infinity. I think your code is UB if Infinity is used. > > > > - C++ requires you to use unique_lock when waiting, which ia pointless from a functional standpoint. > > > > Overall, I would prefer for WebKit to move away from chrono, mutex, and other std:: concurrency stuff, since it’s buggy and poorly designed. > > > > Is there some benefit to this change that is greater than these downsides? > > Yes, I completely agree that std::chrono's overflow unawareness is terrible. > And I'm not sure why std::condition_variable requires std::unique_lock for > waitxxx methods. > > The largest benefit is removing large code base in ThreadingPthread.cpp and > ThreadingWin.cpp. Especially, removing code in ThreadingWin.cpp is nice. We > do not have many contributors in Windows WebKit. > So if we can remove Windows specific code, it significantly reduces the > maintenance burden. > The second tiny benefit is these primitives will be optimized by the > platform. Actually, in Windows, std::mutex backend is replaced from > CRITICAL_SECTION to SRWLock. Got it. I agree that this is a benefit. Based on this, I’m ok with this change. > > I think this is basically the only place to consider about std::mutex and > std::condition_variable since we use WTF::Lock and WTF::Condition in other > places. > Currently, the only place we use WTF::Mutex and WTF::ThreadCondition is > here. For example, our WordLock uses std::mutex and std::condition. > If we need to use WTF::Mutex/WTF::ThreadCondition more for some reasons, we > can create a WTF::Mutex and WTF::ThreadCondition as a wrapper of this > std::mutex and std::thread_condition, and put this handling in this wrapper. > But I don't think it is required. WTF::Lock and WTF::Condition should be > sufficient. Anyway, basically, we can forget about the platform-specific > synchronization primitives at all. > > In C++ std::condition_variable, we have std::condition_variable::wait_until. > So we can use absolute time. > But I used wait_for here because of the following three reasons. > > 1. If we use wait_for, we can forget about the kind of times. And if > std::chrono::duration<double> works well here, we can just use it here. > 2. We do not need to know how to convert our Time to std::chrono::time_point. > 3. In libc++, wait_until is a proxy over wait_for. So rather than > calculating std::chrono::time_point here, passing duration is more efficient. > > But maybe, third reason is not much important since the thread is about to > sleep. Got it. That makes sense. Can you make sure that Infinity works correctly? I don’t think it quite does right now.
Comment on attachment 330196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330196&action=review >>>>>> Source/WTF/wtf/ParkingLot.cpp:601 >>>>>> + me->parkingCondition.wait_for(locker, std::chrono::duration<double> { (timeout - timeout.nowWithSameClock()).value() }); >>>>> >>>>> This is only place we contact with std::condition_variable with std::chrono types. So, we should handle this extra carefully. >>>>> I think this is correct. And per implementation perspective, this is OK in libstdc++ and libc++. >>>>> But I would like to ask the opinion of C++ expert about this. >>>>> The question is simple, what happens if we use, >>>>> >>>>> condition.wait_for(locker, std::chrono::duration<double> { std::numeric_limits<double>::infinity() }); >>>>> >>>>> OR >>>>> >>>>> condition.wait_for(locker, std::chrono::duration<double> { Really Large Double Number }); >>>> >>>> If this assumption does not met, we should insert extra careful conversion here. >>>> But even if we do this, it is worth introducing this patch because, >>>> >>>> 1: Such careful conversion already exists in old ThreadCondition::timedWait implementation. We removed it now in this patch. But if it is necessary, we can just insert it here. >>>> 2: This is only place we contact with std::condition_variable with std::chrono types. So, if we can handle std::chrono types well here, we can just forget all the chrono types in the other places. >>> >>> It would be best if you used an absolute wait here. pthread_cond lets you do it. If C++ doesn’t, then maybe we should keep using pthread_cond. It seems weird to be introducing relative-absolute time conversions because of a desire to use a different-but-not-better set of locking primitives. >>> >>> At some point, I converted a bunch of code away from std::mutex and condition_variable because: >>> >>> - chrono is terrible. We have had multiple bugs because of it. One time it was because of a buggy implementation of some chrono function on some platform. I believe your code could trigger some bug like this if the timeout was Infinity. I think your code is UB if Infinity is used. >>> >>> - C++ requires you to use unique_lock when waiting, which ia pointless from a functional standpoint. >>> >>> Overall, I would prefer for WebKit to move away from chrono, mutex, and other std:: concurrency stuff, since it’s buggy and poorly designed. >>> >>> Is there some benefit to this change that is greater than these downsides? >> >> Yes, I completely agree that std::chrono's overflow unawareness is terrible. And I'm not sure why std::condition_variable requires std::unique_lock for waitxxx methods. >> >> The largest benefit is removing large code base in ThreadingPthread.cpp and ThreadingWin.cpp. Especially, removing code in ThreadingWin.cpp is nice. We do not have many contributors in Windows WebKit. >> So if we can remove Windows specific code, it significantly reduces the maintenance burden. >> The second tiny benefit is these primitives will be optimized by the platform. Actually, in Windows, std::mutex backend is replaced from CRITICAL_SECTION to SRWLock. >> >> I think this is basically the only place to consider about std::mutex and std::condition_variable since we use WTF::Lock and WTF::Condition in other places. >> Currently, the only place we use WTF::Mutex and WTF::ThreadCondition is here. For example, our WordLock uses std::mutex and std::condition. >> If we need to use WTF::Mutex/WTF::ThreadCondition more for some reasons, we can create a WTF::Mutex and WTF::ThreadCondition as a wrapper of this std::mutex and std::thread_condition, and put this handling in this wrapper. >> But I don't think it is required. WTF::Lock and WTF::Condition should be sufficient. Anyway, basically, we can forget about the platform-specific synchronization primitives at all. >> >> In C++ std::condition_variable, we have std::condition_variable::wait_until. So we can use absolute time. >> But I used wait_for here because of the following three reasons. >> >> 1. If we use wait_for, we can forget about the kind of times. And if std::chrono::duration<double> works well here, we can just use it here. >> 2. We do not need to know how to convert our Time to std::chrono::time_point. >> 3. In libc++, wait_until is a proxy over wait_for. So rather than calculating std::chrono::time_point here, passing duration is more efficient. >> >> But maybe, third reason is not much important since the thread is about to sleep. > > Got it. I agree that this is a benefit. Based on this, I’m ok with this change. OK, I've just checked the code (And I'll add a test for this in TestWTF). libc++ is fine. It correctly handles std::chrono::duration<double> { infinity }. libstdc++ is also fine :), that's totally good. The problem is VC2017. I've just opened my Windows machine, looked into the STL code in VC++, and found that this does not handle infinity/NaN well :( It uses std::chrono::duration_cast<> without any checks, it is not good since non-finite duration<double> with duration_cast<integer_types> is UB. And I found some QAs in stackoverflow about this overflow unawareness[1]. Luckily, the fix is simple. We can add special care of this here. And forget everything about std::chrono in the other places. I believe this should be the last place to consider about std::chrono. I really want to redesigned std::chrono which has overflow awareness... [1]: https://stackoverflow.com/questions/42080709/stdcondition-variable-wait-for-infinite
Comment on attachment 330196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330196&action=review >>>>>>> Source/WTF/wtf/ParkingLot.cpp:601 >>>>>>> + me->parkingCondition.wait_for(locker, std::chrono::duration<double> { (timeout - timeout.nowWithSameClock()).value() }); >>>>>> >>>>>> This is only place we contact with std::condition_variable with std::chrono types. So, we should handle this extra carefully. >>>>>> I think this is correct. And per implementation perspective, this is OK in libstdc++ and libc++. >>>>>> But I would like to ask the opinion of C++ expert about this. >>>>>> The question is simple, what happens if we use, >>>>>> >>>>>> condition.wait_for(locker, std::chrono::duration<double> { std::numeric_limits<double>::infinity() }); >>>>>> >>>>>> OR >>>>>> >>>>>> condition.wait_for(locker, std::chrono::duration<double> { Really Large Double Number }); >>>>> >>>>> If this assumption does not met, we should insert extra careful conversion here. >>>>> But even if we do this, it is worth introducing this patch because, >>>>> >>>>> 1: Such careful conversion already exists in old ThreadCondition::timedWait implementation. We removed it now in this patch. But if it is necessary, we can just insert it here. >>>>> 2: This is only place we contact with std::condition_variable with std::chrono types. So, if we can handle std::chrono types well here, we can just forget all the chrono types in the other places. >>>> >>>> It would be best if you used an absolute wait here. pthread_cond lets you do it. If C++ doesn’t, then maybe we should keep using pthread_cond. It seems weird to be introducing relative-absolute time conversions because of a desire to use a different-but-not-better set of locking primitives. >>>> >>>> At some point, I converted a bunch of code away from std::mutex and condition_variable because: >>>> >>>> - chrono is terrible. We have had multiple bugs because of it. One time it was because of a buggy implementation of some chrono function on some platform. I believe your code could trigger some bug like this if the timeout was Infinity. I think your code is UB if Infinity is used. >>>> >>>> - C++ requires you to use unique_lock when waiting, which ia pointless from a functional standpoint. >>>> >>>> Overall, I would prefer for WebKit to move away from chrono, mutex, and other std:: concurrency stuff, since it’s buggy and poorly designed. >>>> >>>> Is there some benefit to this change that is greater than these downsides? >>> >>> Yes, I completely agree that std::chrono's overflow unawareness is terrible. And I'm not sure why std::condition_variable requires std::unique_lock for waitxxx methods. >>> >>> The largest benefit is removing large code base in ThreadingPthread.cpp and ThreadingWin.cpp. Especially, removing code in ThreadingWin.cpp is nice. We do not have many contributors in Windows WebKit. >>> So if we can remove Windows specific code, it significantly reduces the maintenance burden. >>> The second tiny benefit is these primitives will be optimized by the platform. Actually, in Windows, std::mutex backend is replaced from CRITICAL_SECTION to SRWLock. >>> >>> I think this is basically the only place to consider about std::mutex and std::condition_variable since we use WTF::Lock and WTF::Condition in other places. >>> Currently, the only place we use WTF::Mutex and WTF::ThreadCondition is here. For example, our WordLock uses std::mutex and std::condition. >>> If we need to use WTF::Mutex/WTF::ThreadCondition more for some reasons, we can create a WTF::Mutex and WTF::ThreadCondition as a wrapper of this std::mutex and std::thread_condition, and put this handling in this wrapper. >>> But I don't think it is required. WTF::Lock and WTF::Condition should be sufficient. Anyway, basically, we can forget about the platform-specific synchronization primitives at all. >>> >>> In C++ std::condition_variable, we have std::condition_variable::wait_until. So we can use absolute time. >>> But I used wait_for here because of the following three reasons. >>> >>> 1. If we use wait_for, we can forget about the kind of times. And if std::chrono::duration<double> works well here, we can just use it here. >>> 2. We do not need to know how to convert our Time to std::chrono::time_point. >>> 3. In libc++, wait_until is a proxy over wait_for. So rather than calculating std::chrono::time_point here, passing duration is more efficient. >>> >>> But maybe, third reason is not much important since the thread is about to sleep. >> >> Got it. I agree that this is a benefit. Based on this, I’m ok with this change. > > OK, I've just checked the code (And I'll add a test for this in TestWTF). > libc++ is fine. It correctly handles std::chrono::duration<double> { infinity }. libstdc++ is also fine :), that's totally good. > > The problem is VC2017. I've just opened my Windows machine, looked into the STL code in VC++, and found that this does not handle infinity/NaN well :( > It uses std::chrono::duration_cast<> without any checks, it is not good since non-finite duration<double> with duration_cast<integer_types> is UB. > And I found some QAs in stackoverflow about this overflow unawareness[1]. > > Luckily, the fix is simple. We can add special care of this here. And forget everything about std::chrono in the other places. I believe this should be the last place to consider about std::chrono. I really want to redesigned std::chrono which has overflow awareness... > > [1]: https://stackoverflow.com/questions/42080709/stdcondition-variable-wait-for-infinite Lol. I dug C++ spec and existing implementations. And I finally concluded that we have no way to use infinite timeout in a portable manner! The spec does not say the actual clock and duration types used in std::condition_variable. So we cannot clamp the given value safely to the std::condition_variable's internal clock and duration. And it seems that the spec does not care about overflow of std::chrono types. As a result, libc++ and libstdc++ support this as quality-of-implementation. And VC++'s implementation does not handle it well, but it is OK since it is not required. So we cannot rely on this since it is not specified in the spec. It is quite surprising...... http://eel.is/c++draft/thread.req.timing
(In reply to Yusuke Suzuki from comment #9) > Comment on attachment 330196 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=330196&action=review > > >>>>>>> Source/WTF/wtf/ParkingLot.cpp:601 > >>>>>>> + me->parkingCondition.wait_for(locker, std::chrono::duration<double> { (timeout - timeout.nowWithSameClock()).value() }); > >>>>>> > >>>>>> This is only place we contact with std::condition_variable with std::chrono types. So, we should handle this extra carefully. > >>>>>> I think this is correct. And per implementation perspective, this is OK in libstdc++ and libc++. > >>>>>> But I would like to ask the opinion of C++ expert about this. > >>>>>> The question is simple, what happens if we use, > >>>>>> > >>>>>> condition.wait_for(locker, std::chrono::duration<double> { std::numeric_limits<double>::infinity() }); > >>>>>> > >>>>>> OR > >>>>>> > >>>>>> condition.wait_for(locker, std::chrono::duration<double> { Really Large Double Number }); > >>>>> > >>>>> If this assumption does not met, we should insert extra careful conversion here. > >>>>> But even if we do this, it is worth introducing this patch because, > >>>>> > >>>>> 1: Such careful conversion already exists in old ThreadCondition::timedWait implementation. We removed it now in this patch. But if it is necessary, we can just insert it here. > >>>>> 2: This is only place we contact with std::condition_variable with std::chrono types. So, if we can handle std::chrono types well here, we can just forget all the chrono types in the other places. > >>>> > >>>> It would be best if you used an absolute wait here. pthread_cond lets you do it. If C++ doesn’t, then maybe we should keep using pthread_cond. It seems weird to be introducing relative-absolute time conversions because of a desire to use a different-but-not-better set of locking primitives. > >>>> > >>>> At some point, I converted a bunch of code away from std::mutex and condition_variable because: > >>>> > >>>> - chrono is terrible. We have had multiple bugs because of it. One time it was because of a buggy implementation of some chrono function on some platform. I believe your code could trigger some bug like this if the timeout was Infinity. I think your code is UB if Infinity is used. > >>>> > >>>> - C++ requires you to use unique_lock when waiting, which ia pointless from a functional standpoint. > >>>> > >>>> Overall, I would prefer for WebKit to move away from chrono, mutex, and other std:: concurrency stuff, since it’s buggy and poorly designed. > >>>> > >>>> Is there some benefit to this change that is greater than these downsides? > >>> > >>> Yes, I completely agree that std::chrono's overflow unawareness is terrible. And I'm not sure why std::condition_variable requires std::unique_lock for waitxxx methods. > >>> > >>> The largest benefit is removing large code base in ThreadingPthread.cpp and ThreadingWin.cpp. Especially, removing code in ThreadingWin.cpp is nice. We do not have many contributors in Windows WebKit. > >>> So if we can remove Windows specific code, it significantly reduces the maintenance burden. > >>> The second tiny benefit is these primitives will be optimized by the platform. Actually, in Windows, std::mutex backend is replaced from CRITICAL_SECTION to SRWLock. > >>> > >>> I think this is basically the only place to consider about std::mutex and std::condition_variable since we use WTF::Lock and WTF::Condition in other places. > >>> Currently, the only place we use WTF::Mutex and WTF::ThreadCondition is here. For example, our WordLock uses std::mutex and std::condition. > >>> If we need to use WTF::Mutex/WTF::ThreadCondition more for some reasons, we can create a WTF::Mutex and WTF::ThreadCondition as a wrapper of this std::mutex and std::thread_condition, and put this handling in this wrapper. > >>> But I don't think it is required. WTF::Lock and WTF::Condition should be sufficient. Anyway, basically, we can forget about the platform-specific synchronization primitives at all. > >>> > >>> In C++ std::condition_variable, we have std::condition_variable::wait_until. So we can use absolute time. > >>> But I used wait_for here because of the following three reasons. > >>> > >>> 1. If we use wait_for, we can forget about the kind of times. And if std::chrono::duration<double> works well here, we can just use it here. > >>> 2. We do not need to know how to convert our Time to std::chrono::time_point. > >>> 3. In libc++, wait_until is a proxy over wait_for. So rather than calculating std::chrono::time_point here, passing duration is more efficient. > >>> > >>> But maybe, third reason is not much important since the thread is about to sleep. > >> > >> Got it. I agree that this is a benefit. Based on this, I’m ok with this change. > > > > OK, I've just checked the code (And I'll add a test for this in TestWTF). > > libc++ is fine. It correctly handles std::chrono::duration<double> { infinity }. libstdc++ is also fine :), that's totally good. > > > > The problem is VC2017. I've just opened my Windows machine, looked into the STL code in VC++, and found that this does not handle infinity/NaN well :( > > It uses std::chrono::duration_cast<> without any checks, it is not good since non-finite duration<double> with duration_cast<integer_types> is UB. > > And I found some QAs in stackoverflow about this overflow unawareness[1]. > > > > Luckily, the fix is simple. We can add special care of this here. And forget everything about std::chrono in the other places. I believe this should be the last place to consider about std::chrono. I really want to redesigned std::chrono which has overflow awareness... > > > > [1]: https://stackoverflow.com/questions/42080709/stdcondition-variable-wait-for-infinite > > Lol. I dug C++ spec and existing implementations. And I finally concluded > that we have no way to use infinite timeout in a portable manner! > > The spec does not say the actual clock and duration types used in > std::condition_variable. So we cannot clamp the given value safely to the > std::condition_variable's internal clock and duration. > And it seems that the spec does not care about overflow of std::chrono types. > As a result, libc++ and libstdc++ support this as quality-of-implementation. > And VC++'s implementation does not handle it well, but it is OK since it is > not required. > So we cannot rely on this since it is not specified in the spec. It is quite > surprising...... > > http://eel.is/c++draft/thread.req.timing I’m happy with that so long as all of the following work: - waiting until some huge time (any time greater than UINT64_MAX) waits just for UINT64_MAX or Infinity (your choice). - waiting until negative Times does the right thing. I suppose you could either park and immediately unpark or just return early. Maybe there I already a check that covers this. - waiting until NaN time does something sensible (like immediate timeout or infinity timeout) rather than waiting for a random amount of time.
Comment on attachment 330196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330196&action=review >>>>>>>>> Source/WTF/wtf/ParkingLot.cpp:601 >>>>>>>>> + me->parkingCondition.wait_for(locker, std::chrono::duration<double> { (timeout - timeout.nowWithSameClock()).value() }); >>>>>>>> >>>>>>>> This is only place we contact with std::condition_variable with std::chrono types. So, we should handle this extra carefully. >>>>>>>> I think this is correct. And per implementation perspective, this is OK in libstdc++ and libc++. >>>>>>>> But I would like to ask the opinion of C++ expert about this. >>>>>>>> The question is simple, what happens if we use, >>>>>>>> >>>>>>>> condition.wait_for(locker, std::chrono::duration<double> { std::numeric_limits<double>::infinity() }); >>>>>>>> >>>>>>>> OR >>>>>>>> >>>>>>>> condition.wait_for(locker, std::chrono::duration<double> { Really Large Double Number }); >>>>>>> >>>>>>> If this assumption does not met, we should insert extra careful conversion here. >>>>>>> But even if we do this, it is worth introducing this patch because, >>>>>>> >>>>>>> 1: Such careful conversion already exists in old ThreadCondition::timedWait implementation. We removed it now in this patch. But if it is necessary, we can just insert it here. >>>>>>> 2: This is only place we contact with std::condition_variable with std::chrono types. So, if we can handle std::chrono types well here, we can just forget all the chrono types in the other places. >>>>>> >>>>>> It would be best if you used an absolute wait here. pthread_cond lets you do it. If C++ doesn’t, then maybe we should keep using pthread_cond. It seems weird to be introducing relative-absolute time conversions because of a desire to use a different-but-not-better set of locking primitives. >>>>>> >>>>>> At some point, I converted a bunch of code away from std::mutex and condition_variable because: >>>>>> >>>>>> - chrono is terrible. We have had multiple bugs because of it. One time it was because of a buggy implementation of some chrono function on some platform. I believe your code could trigger some bug like this if the timeout was Infinity. I think your code is UB if Infinity is used. >>>>>> >>>>>> - C++ requires you to use unique_lock when waiting, which ia pointless from a functional standpoint. >>>>>> >>>>>> Overall, I would prefer for WebKit to move away from chrono, mutex, and other std:: concurrency stuff, since it’s buggy and poorly designed. >>>>>> >>>>>> Is there some benefit to this change that is greater than these downsides? >>>>> >>>>> Yes, I completely agree that std::chrono's overflow unawareness is terrible. And I'm not sure why std::condition_variable requires std::unique_lock for waitxxx methods. >>>>> >>>>> The largest benefit is removing large code base in ThreadingPthread.cpp and ThreadingWin.cpp. Especially, removing code in ThreadingWin.cpp is nice. We do not have many contributors in Windows WebKit. >>>>> So if we can remove Windows specific code, it significantly reduces the maintenance burden. >>>>> The second tiny benefit is these primitives will be optimized by the platform. Actually, in Windows, std::mutex backend is replaced from CRITICAL_SECTION to SRWLock. >>>>> >>>>> I think this is basically the only place to consider about std::mutex and std::condition_variable since we use WTF::Lock and WTF::Condition in other places. >>>>> Currently, the only place we use WTF::Mutex and WTF::ThreadCondition is here. For example, our WordLock uses std::mutex and std::condition. >>>>> If we need to use WTF::Mutex/WTF::ThreadCondition more for some reasons, we can create a WTF::Mutex and WTF::ThreadCondition as a wrapper of this std::mutex and std::thread_condition, and put this handling in this wrapper. >>>>> But I don't think it is required. WTF::Lock and WTF::Condition should be sufficient. Anyway, basically, we can forget about the platform-specific synchronization primitives at all. >>>>> >>>>> In C++ std::condition_variable, we have std::condition_variable::wait_until. So we can use absolute time. >>>>> But I used wait_for here because of the following three reasons. >>>>> >>>>> 1. If we use wait_for, we can forget about the kind of times. And if std::chrono::duration<double> works well here, we can just use it here. >>>>> 2. We do not need to know how to convert our Time to std::chrono::time_point. >>>>> 3. In libc++, wait_until is a proxy over wait_for. So rather than calculating std::chrono::time_point here, passing duration is more efficient. >>>>> >>>>> But maybe, third reason is not much important since the thread is about to sleep. >>>> >>>> Got it. I agree that this is a benefit. Based on this, I’m ok with this change. >>> >>> OK, I've just checked the code (And I'll add a test for this in TestWTF). >>> libc++ is fine. It correctly handles std::chrono::duration<double> { infinity }. libstdc++ is also fine :), that's totally good. >>> >>> The problem is VC2017. I've just opened my Windows machine, looked into the STL code in VC++, and found that this does not handle infinity/NaN well :( >>> It uses std::chrono::duration_cast<> without any checks, it is not good since non-finite duration<double> with duration_cast<integer_types> is UB. >>> And I found some QAs in stackoverflow about this overflow unawareness[1]. >>> >>> Luckily, the fix is simple. We can add special care of this here. And forget everything about std::chrono in the other places. I believe this should be the last place to consider about std::chrono. I really want to redesigned std::chrono which has overflow awareness... >>> >>> [1]: https://stackoverflow.com/questions/42080709/stdcondition-variable-wait-for-infinite >> >> Lol. I dug C++ spec and existing implementations. And I finally concluded that we have no way to use infinite timeout in a portable manner! >> >> The spec does not say the actual clock and duration types used in std::condition_variable. So we cannot clamp the given value safely to the std::condition_variable's internal clock and duration. >> And it seems that the spec does not care about overflow of std::chrono types. >> As a result, libc++ and libstdc++ support this as quality-of-implementation. And VC++'s implementation does not handle it well, but it is OK since it is not required. >> So we cannot rely on this since it is not specified in the spec. It is quite surprising...... >> >> http://eel.is/c++draft/thread.req.timing > > I’m happy with that so long as all of the following work: > > - waiting until some huge time (any time greater than UINT64_MAX) waits just for UINT64_MAX or Infinity (your choice). > > - waiting until negative Times does the right thing. I suppose you could either park and immediately unpark or just return early. Maybe there I already a check that covers this. > > - waiting until NaN time does something sensible (like immediate timeout or infinity timeout) rather than waiting for a random amount of time. 1. I think the first one is not reliably possible in the C++ standard library. The spec says nothing about the valid range of std::chrono::duration. So we cannot know whether the given value is considered as huge in std::condition_variable. So we cannot clamp it. Even if we pass 1 second (1s), we cannot guarantee this works well: if the internal system manages time as picoseconds or more small units, and/or the current timepoint is very close to the maximum value of std::chrono::time_point used internally, then overflow can happen. I'm not sure whether we have a way to meet this requirement in std::chrono & std::condition_variable. 2. This one is already handled in this `while` loop. Negative duration / old time_point just returns immediately. 3. It is not specified. But we can easily detect NaN in our side. In the current implementation, we only enter the body of this while loop if `timeout.nowWithSameClock() < timeout` condition is met. NaN does not meet this test. So basically, (1) is the problem. Yeah, we can handle infinity in our side. For example, if (std::isinf(timeout)) condition.wait(...); else condition.wait_for(...); But we cannot reliably handle a huge value since we do not know the range which the system considers it is not huge.
(In reply to Yusuke Suzuki from comment #11) > Comment on attachment 330196 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=330196&action=review > > >>>>>>>>> Source/WTF/wtf/ParkingLot.cpp:601 > >>>>>>>>> + me->parkingCondition.wait_for(locker, std::chrono::duration<double> { (timeout - timeout.nowWithSameClock()).value() }); > >>>>>>>> > >>>>>>>> This is only place we contact with std::condition_variable with std::chrono types. So, we should handle this extra carefully. > >>>>>>>> I think this is correct. And per implementation perspective, this is OK in libstdc++ and libc++. > >>>>>>>> But I would like to ask the opinion of C++ expert about this. > >>>>>>>> The question is simple, what happens if we use, > >>>>>>>> > >>>>>>>> condition.wait_for(locker, std::chrono::duration<double> { std::numeric_limits<double>::infinity() }); > >>>>>>>> > >>>>>>>> OR > >>>>>>>> > >>>>>>>> condition.wait_for(locker, std::chrono::duration<double> { Really Large Double Number }); > >>>>>>> > >>>>>>> If this assumption does not met, we should insert extra careful conversion here. > >>>>>>> But even if we do this, it is worth introducing this patch because, > >>>>>>> > >>>>>>> 1: Such careful conversion already exists in old ThreadCondition::timedWait implementation. We removed it now in this patch. But if it is necessary, we can just insert it here. > >>>>>>> 2: This is only place we contact with std::condition_variable with std::chrono types. So, if we can handle std::chrono types well here, we can just forget all the chrono types in the other places. > >>>>>> > >>>>>> It would be best if you used an absolute wait here. pthread_cond lets you do it. If C++ doesn’t, then maybe we should keep using pthread_cond. It seems weird to be introducing relative-absolute time conversions because of a desire to use a different-but-not-better set of locking primitives. > >>>>>> > >>>>>> At some point, I converted a bunch of code away from std::mutex and condition_variable because: > >>>>>> > >>>>>> - chrono is terrible. We have had multiple bugs because of it. One time it was because of a buggy implementation of some chrono function on some platform. I believe your code could trigger some bug like this if the timeout was Infinity. I think your code is UB if Infinity is used. > >>>>>> > >>>>>> - C++ requires you to use unique_lock when waiting, which ia pointless from a functional standpoint. > >>>>>> > >>>>>> Overall, I would prefer for WebKit to move away from chrono, mutex, and other std:: concurrency stuff, since it’s buggy and poorly designed. > >>>>>> > >>>>>> Is there some benefit to this change that is greater than these downsides? > >>>>> > >>>>> Yes, I completely agree that std::chrono's overflow unawareness is terrible. And I'm not sure why std::condition_variable requires std::unique_lock for waitxxx methods. > >>>>> > >>>>> The largest benefit is removing large code base in ThreadingPthread.cpp and ThreadingWin.cpp. Especially, removing code in ThreadingWin.cpp is nice. We do not have many contributors in Windows WebKit. > >>>>> So if we can remove Windows specific code, it significantly reduces the maintenance burden. > >>>>> The second tiny benefit is these primitives will be optimized by the platform. Actually, in Windows, std::mutex backend is replaced from CRITICAL_SECTION to SRWLock. > >>>>> > >>>>> I think this is basically the only place to consider about std::mutex and std::condition_variable since we use WTF::Lock and WTF::Condition in other places. > >>>>> Currently, the only place we use WTF::Mutex and WTF::ThreadCondition is here. For example, our WordLock uses std::mutex and std::condition. > >>>>> If we need to use WTF::Mutex/WTF::ThreadCondition more for some reasons, we can create a WTF::Mutex and WTF::ThreadCondition as a wrapper of this std::mutex and std::thread_condition, and put this handling in this wrapper. > >>>>> But I don't think it is required. WTF::Lock and WTF::Condition should be sufficient. Anyway, basically, we can forget about the platform-specific synchronization primitives at all. > >>>>> > >>>>> In C++ std::condition_variable, we have std::condition_variable::wait_until. So we can use absolute time. > >>>>> But I used wait_for here because of the following three reasons. > >>>>> > >>>>> 1. If we use wait_for, we can forget about the kind of times. And if std::chrono::duration<double> works well here, we can just use it here. > >>>>> 2. We do not need to know how to convert our Time to std::chrono::time_point. > >>>>> 3. In libc++, wait_until is a proxy over wait_for. So rather than calculating std::chrono::time_point here, passing duration is more efficient. > >>>>> > >>>>> But maybe, third reason is not much important since the thread is about to sleep. > >>>> > >>>> Got it. I agree that this is a benefit. Based on this, I’m ok with this change. > >>> > >>> OK, I've just checked the code (And I'll add a test for this in TestWTF). > >>> libc++ is fine. It correctly handles std::chrono::duration<double> { infinity }. libstdc++ is also fine :), that's totally good. > >>> > >>> The problem is VC2017. I've just opened my Windows machine, looked into the STL code in VC++, and found that this does not handle infinity/NaN well :( > >>> It uses std::chrono::duration_cast<> without any checks, it is not good since non-finite duration<double> with duration_cast<integer_types> is UB. > >>> And I found some QAs in stackoverflow about this overflow unawareness[1]. > >>> > >>> Luckily, the fix is simple. We can add special care of this here. And forget everything about std::chrono in the other places. I believe this should be the last place to consider about std::chrono. I really want to redesigned std::chrono which has overflow awareness... > >>> > >>> [1]: https://stackoverflow.com/questions/42080709/stdcondition-variable-wait-for-infinite > >> > >> Lol. I dug C++ spec and existing implementations. And I finally concluded that we have no way to use infinite timeout in a portable manner! > >> > >> The spec does not say the actual clock and duration types used in std::condition_variable. So we cannot clamp the given value safely to the std::condition_variable's internal clock and duration. > >> And it seems that the spec does not care about overflow of std::chrono types. > >> As a result, libc++ and libstdc++ support this as quality-of-implementation. And VC++'s implementation does not handle it well, but it is OK since it is not required. > >> So we cannot rely on this since it is not specified in the spec. It is quite surprising...... > >> > >> http://eel.is/c++draft/thread.req.timing > > > > I’m happy with that so long as all of the following work: > > > > - waiting until some huge time (any time greater than UINT64_MAX) waits just for UINT64_MAX or Infinity (your choice). > > > > - waiting until negative Times does the right thing. I suppose you could either park and immediately unpark or just return early. Maybe there I already a check that covers this. > > > > - waiting until NaN time does something sensible (like immediate timeout or infinity timeout) rather than waiting for a random amount of time. > > 1. I think the first one is not reliably possible in the C++ standard > library. The spec says nothing about the valid range of > std::chrono::duration. So we cannot know whether the given value is > considered as huge in std::condition_variable. So we cannot clamp it. Even > if we pass 1 second (1s), we cannot guarantee this works well: if the > internal system manages time as picoseconds or more small units, and/or the > current timepoint is very close to the maximum value of > std::chrono::time_point used internally, then overflow can happen. I'm not > sure whether we have a way to meet this requirement in std::chrono & > std::condition_variable. Really? You cannot use numeric_limits on the storage type of the duration type taken by condition_variable? Since wait_for is templatized on duration type, I think we can just choose a type to use and bet that this is the type they use internally. Internally they will probably use 64-bit nanoseconds in the worst case (pico would be insane while micro has an effectively higher limit since a UINT64_MAX in microseconds is a longer duration than in nanos). If you do this, you can first use double math to determine if te duration-as-double would overflow when cast to duration-as-64-bit-nanos. Then pass that to the condition variable, hoping that it won’t get cast to picos. > > 2. This one is already handled in this `while` loop. Negative duration / old > time_point just returns immediately. > > 3. It is not specified. But we can easily detect NaN in our side. In the > current implementation, we only enter the body of this while loop if > `timeout.nowWithSameClock() < timeout` condition is met. NaN does not meet > this test. > > So basically, (1) is the problem. Yeah, we can handle infinity in our side. > For example, > > if (std::isinf(timeout)) > condition.wait(...); > else > condition.wait_for(...); > > But we cannot reliably handle a huge value since we do not know the range > which the system considers it is not huge.
Comment on attachment 330196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330196&action=review >>>>>>>>>>> Source/WTF/wtf/ParkingLot.cpp:601 >>>>>>>>>>> + me->parkingCondition.wait_for(locker, std::chrono::duration<double> { (timeout - timeout.nowWithSameClock()).value() }); >>>>>>>>>> >>>>>>>>>> This is only place we contact with std::condition_variable with std::chrono types. So, we should handle this extra carefully. >>>>>>>>>> I think this is correct. And per implementation perspective, this is OK in libstdc++ and libc++. >>>>>>>>>> But I would like to ask the opinion of C++ expert about this. >>>>>>>>>> The question is simple, what happens if we use, >>>>>>>>>> >>>>>>>>>> condition.wait_for(locker, std::chrono::duration<double> { std::numeric_limits<double>::infinity() }); >>>>>>>>>> >>>>>>>>>> OR >>>>>>>>>> >>>>>>>>>> condition.wait_for(locker, std::chrono::duration<double> { Really Large Double Number }); >>>>>>>>> >>>>>>>>> If this assumption does not met, we should insert extra careful conversion here. >>>>>>>>> But even if we do this, it is worth introducing this patch because, >>>>>>>>> >>>>>>>>> 1: Such careful conversion already exists in old ThreadCondition::timedWait implementation. We removed it now in this patch. But if it is necessary, we can just insert it here. >>>>>>>>> 2: This is only place we contact with std::condition_variable with std::chrono types. So, if we can handle std::chrono types well here, we can just forget all the chrono types in the other places. >>>>>>>> >>>>>>>> It would be best if you used an absolute wait here. pthread_cond lets you do it. If C++ doesn’t, then maybe we should keep using pthread_cond. It seems weird to be introducing relative-absolute time conversions because of a desire to use a different-but-not-better set of locking primitives. >>>>>>>> >>>>>>>> At some point, I converted a bunch of code away from std::mutex and condition_variable because: >>>>>>>> >>>>>>>> - chrono is terrible. We have had multiple bugs because of it. One time it was because of a buggy implementation of some chrono function on some platform. I believe your code could trigger some bug like this if the timeout was Infinity. I think your code is UB if Infinity is used. >>>>>>>> >>>>>>>> - C++ requires you to use unique_lock when waiting, which ia pointless from a functional standpoint. >>>>>>>> >>>>>>>> Overall, I would prefer for WebKit to move away from chrono, mutex, and other std:: concurrency stuff, since it’s buggy and poorly designed. >>>>>>>> >>>>>>>> Is there some benefit to this change that is greater than these downsides? >>>>>>> >>>>>>> Yes, I completely agree that std::chrono's overflow unawareness is terrible. And I'm not sure why std::condition_variable requires std::unique_lock for waitxxx methods. >>>>>>> >>>>>>> The largest benefit is removing large code base in ThreadingPthread.cpp and ThreadingWin.cpp. Especially, removing code in ThreadingWin.cpp is nice. We do not have many contributors in Windows WebKit. >>>>>>> So if we can remove Windows specific code, it significantly reduces the maintenance burden. >>>>>>> The second tiny benefit is these primitives will be optimized by the platform. Actually, in Windows, std::mutex backend is replaced from CRITICAL_SECTION to SRWLock. >>>>>>> >>>>>>> I think this is basically the only place to consider about std::mutex and std::condition_variable since we use WTF::Lock and WTF::Condition in other places. >>>>>>> Currently, the only place we use WTF::Mutex and WTF::ThreadCondition is here. For example, our WordLock uses std::mutex and std::condition. >>>>>>> If we need to use WTF::Mutex/WTF::ThreadCondition more for some reasons, we can create a WTF::Mutex and WTF::ThreadCondition as a wrapper of this std::mutex and std::thread_condition, and put this handling in this wrapper. >>>>>>> But I don't think it is required. WTF::Lock and WTF::Condition should be sufficient. Anyway, basically, we can forget about the platform-specific synchronization primitives at all. >>>>>>> >>>>>>> In C++ std::condition_variable, we have std::condition_variable::wait_until. So we can use absolute time. >>>>>>> But I used wait_for here because of the following three reasons. >>>>>>> >>>>>>> 1. If we use wait_for, we can forget about the kind of times. And if std::chrono::duration<double> works well here, we can just use it here. >>>>>>> 2. We do not need to know how to convert our Time to std::chrono::time_point. >>>>>>> 3. In libc++, wait_until is a proxy over wait_for. So rather than calculating std::chrono::time_point here, passing duration is more efficient. >>>>>>> >>>>>>> But maybe, third reason is not much important since the thread is about to sleep. >>>>>> >>>>>> Got it. I agree that this is a benefit. Based on this, I’m ok with this change. >>>>> >>>>> OK, I've just checked the code (And I'll add a test for this in TestWTF). >>>>> libc++ is fine. It correctly handles std::chrono::duration<double> { infinity }. libstdc++ is also fine :), that's totally good. >>>>> >>>>> The problem is VC2017. I've just opened my Windows machine, looked into the STL code in VC++, and found that this does not handle infinity/NaN well :( >>>>> It uses std::chrono::duration_cast<> without any checks, it is not good since non-finite duration<double> with duration_cast<integer_types> is UB. >>>>> And I found some QAs in stackoverflow about this overflow unawareness[1]. >>>>> >>>>> Luckily, the fix is simple. We can add special care of this here. And forget everything about std::chrono in the other places. I believe this should be the last place to consider about std::chrono. I really want to redesigned std::chrono which has overflow awareness... >>>>> >>>>> [1]: https://stackoverflow.com/questions/42080709/stdcondition-variable-wait-for-infinite >>>> >>>> Lol. I dug C++ spec and existing implementations. And I finally concluded that we have no way to use infinite timeout in a portable manner! >>>> >>>> The spec does not say the actual clock and duration types used in std::condition_variable. So we cannot clamp the given value safely to the std::condition_variable's internal clock and duration. >>>> And it seems that the spec does not care about overflow of std::chrono types. >>>> As a result, libc++ and libstdc++ support this as quality-of-implementation. And VC++'s implementation does not handle it well, but it is OK since it is not required. >>>> So we cannot rely on this since it is not specified in the spec. It is quite surprising...... >>>> >>>> http://eel.is/c++draft/thread.req.timing >>> >>> I’m happy with that so long as all of the following work: >>> >>> - waiting until some huge time (any time greater than UINT64_MAX) waits just for UINT64_MAX or Infinity (your choice). >>> >>> - waiting until negative Times does the right thing. I suppose you could either park and immediately unpark or just return early. Maybe there I already a check that covers this. >>> >>> - waiting until NaN time does something sensible (like immediate timeout or infinity timeout) rather than waiting for a random amount of time. >> >> 1. I think the first one is not reliably possible in the C++ standard library. The spec says nothing about the valid range of std::chrono::duration. So we cannot know whether the given value is considered as huge in std::condition_variable. So we cannot clamp it. Even if we pass 1 second (1s), we cannot guarantee this works well: if the internal system manages time as picoseconds or more small units, and/or the current timepoint is very close to the maximum value of std::chrono::time_point used internally, then overflow can happen. I'm not sure whether we have a way to meet this requirement in std::chrono & std::condition_variable. >> >> 2. This one is already handled in this `while` loop. Negative duration / old time_point just returns immediately. >> >> 3. It is not specified. But we can easily detect NaN in our side. In the current implementation, we only enter the body of this while loop if `timeout.nowWithSameClock() < timeout` condition is met. NaN does not meet this test. >> >> So basically, (1) is the problem. Yeah, we can handle infinity in our side. For example, >> >> if (std::isinf(timeout)) >> condition.wait(...); >> else >> condition.wait_for(...); >> >> But we cannot reliably handle a huge value since we do not know the range which the system considers it is not huge. > > Really? You cannot use numeric_limits on the storage type of the duration type taken by condition_variable? > > Since wait_for is templatized on duration type, I think we can just choose a type to use and bet that this is the type they use internally. Internally they will probably use 64-bit nanoseconds in the worst case (pico would be insane while micro has an effectively higher limit since a UINT64_MAX in microseconds is a longer duration than in nanos). > > If you do this, you can first use double math to determine if te duration-as-double would overflow when cast to duration-as-64-bit-nanos. Then pass that to the condition variable, hoping that it won’t get cast to picos. Basically, time_point::max() / duration::max() to wait_for / wait_until do not work well. For example, MSVC's `wait_for(locker, duration<uint64_t, nanoseconds>::max())` immediately performs the conversion `wait_until(locker, time_point + duration)`. And it can lead to time_point overflow[1]. And time_point::max() + wait_until also can lead to overflow since it can adjust time_point's internal value with arithmetic operations (the epoch of the time would be different from UNIX epoch). For example, `wait_until(locker, std::steady_clock::time_point::max())` leads to immediate returns since internally it would perform clock conversion from steady_clock to system_clock and it can lead to time_point overflow[2]. The possible way to handle these issues are following. 1. We first define the valid range of duration conservatively. And we need to believe this range is valid in internal system implementation even if clock time_point change happens. 2. And if we found that the given duration is out of range, we just call `wait()` instead of `wait_for` or `wait_until` since the latter ones are unreliable if we pass huge values. 3. Otherwise, we can use `wait_for` or `wait_until` super carefully. Personally I think this is a design flaw of the current spec. [1]: https://stackoverflow.com/questions/27726818/stdcondition-variablewait-for-exits-immediately-when-given-stdchronodura [2]: https://stackoverflow.com/questions/39723413/wait-until-behavior-for-time-pointmax