[webkit-reviews] review canceled: [Bug 181161] [WTF] Use std::mutex and std::condition_variable for the lowest level locking primitives : [Attachment 330196] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has canceled Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 181161: [WTF] Use std::mutex and std::condition_variable for the lowest
level locking primitives
https://bugs.webkit.org/show_bug.cgi?id=181161

Attachment 330196: Patch

https://bugs.webkit.org/attachment.cgi?id=330196&action=review




--- Comment #9 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 330196
  --> https://bugs.webkit.org/attachment.cgi?id=330196
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-inf
inite

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