Bug 227966 – [curl] Use curl_multi_poll and curl_multi_wakeup instead of select

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Fujii Hironori]

[curl] Use curl_multi_poll and curl_multi_wakeup instead of select

'select' system call can be replaced by those curl API.

curl_multi_poll
https://curl.se/libcurl/c/curl_multi_poll.html

curl_multi_wakeup
https://curl.se/libcurl/c/curl_multi_wakeup.html

This is your wake up curl | daniel.haxx.se
https://daniel.haxx.se/blog/2019/12/09/this-is-your-wake-up-curl/

[Fujii Hironori]

Created attachment 433524 [details]
Patch

[Don Olmstead]

Comment on attachment 433524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433524&action=review

Going to ask Basuke to do an informal on this.

> Source/WebCore/ChangeLog:8
> +        libcurl 7.68.0 added curl_multi_poll and curl_multi_wakeup.

I checked the OptionsWinCairo.cmake and 7.71.0 is the requirement so nothing needs to change.

[Basuke Suzuki]

Comment on attachment 433524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433524&action=review

This is informal review. Thanks for the patch. I love to see curl port is modernizing in a good way. But I don't agree with the direction of the change of therad life cycle.

> Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:145
> +        Locker locker { m_multiHandleMutex };

m_multiHandleMutex is used only in this workerThread by design. There's no need to lock. I bed you've added the lock because you need to check the validity of multi handle before calling curl_multi_wakeup. If that's the reason, how about moving the instantiation of the multi handle to the thread creation timing in main thread. That's ensure multi handle existance while thread is running. Any way this also related to the design of the thread life cycle I mentioned at the latter part (*1).

> Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:-199
> -        stopThreadIfNoMoreJobRunning();

The original design is stopping the thread if there's network is running to minimize the resource usage in entire system. I don't agree to change that with any good reason. [*1]

> Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:182
> +        Locker locker { m_multiHandleMutex };

Ditto.

> Source/WebCore/platform/network/curl/CurlRequestScheduler.h:75
> +    Lock m_multiHandleMutex;

No need.

> Source/WebCore/platform/network/curl/CurlRequestScheduler.h:76
> +    std::optional<CurlMultiHandle> m_curlMultiHandle;

What is the reason to change it from unique_ptr to optional? If there's good reason, then use WTF::Optional instead.

[Fujii Hironori]

Comment on attachment 433524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433524&action=review

>> Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:145
>> +        Locker locker { m_multiHandleMutex };
> 
> m_multiHandleMutex is used only in this workerThread by design. There's no need to lock. I bed you've added the lock because you need to check the validity of multi handle before calling curl_multi_wakeup. If that's the reason, how about moving the instantiation of the multi handle to the thread creation timing in main thread. That's ensure multi handle existance while thread is running. Any way this also related to the design of the thread life cycle I mentioned at the latter part (*1).

I also has a same idea, and tried the idea, but it doesn't work at all.
I didn't look into it, but I guess that's because a multi handle might be fixed to the thread of creation time.

>> Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:-199
>> -        stopThreadIfNoMoreJobRunning();
> 
> The original design is stopping the thread if there's network is running to minimize the resource usage in entire system. I don't agree to change that with any good reason. [*1]

I think the original design stops the thread because it didn't implement the wake up by using self-piping technique. It runs busily while no active handles (Bug 227942).
The network thread is almost always necessary for a browser. I don't agree it can minimize the resource usage.

>> Source/WebCore/platform/network/curl/CurlRequestScheduler.h:76
>> +    std::optional<CurlMultiHandle> m_curlMultiHandle;
> 
> What is the reason to change it from unique_ptr to optional? If there's good reason, then use WTF::Optional instead.

Heap allocation have a cost of allocation and fragmentation, but useful for memory saving by freeing unused objects.
However, CurlMultiHandle isn't big and CurlRequestScheduler almost always has a CurlMultiHandle.
It doesn't make sense to separately allocate a CurlMultiHandle .
WTF::Optional was removed in favor of std::optional (Bug 226705).

[Basuke Suzuki]

Comment on attachment 433524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433524&action=review

>>> Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:145
>>> +        Locker locker { m_multiHandleMutex };
>> 
>> m_multiHandleMutex is used only in this workerThread by design. There's no need to lock. I bed you've added the lock because you need to check the validity of multi handle before calling curl_multi_wakeup. If that's the reason, how about moving the instantiation of the multi handle to the thread creation timing in main thread. That's ensure multi handle existance while thread is running. Any way this also related to the design of the thread life cycle I mentioned at the latter part (*1).
> 
> I also has a same idea, and tried the idea, but it doesn't work at all.
> I didn't look into it, but I guess that's because a multi handle might be fixed to the thread of creation time.

Okay. If I have time to investigate more, I can dig into this, but for now, safer is better. Let's add this mutex and lock area.

>>> Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:-199
>>> -        stopThreadIfNoMoreJobRunning();
>> 
>> The original design is stopping the thread if there's network is running to minimize the resource usage in entire system. I don't agree to change that with any good reason. [*1]
> 
> I think the original design stops the thread because it didn't implement the wake up by using self-piping technique. It runs busily while no active handles (Bug 227942).
> The network thread is almost always necessary for a browser. I don't agree it can minimize the resource usage.

> It runs busily while no active handles (Bug 227942).

That's be solved by this patch :) and not related to the life cycle issue we are discussing. You know WebView is not only for browser. Our use case requires that design. On the other hand I understand the your design. It is okay to add a compiler flag to choose which implementation. On WinCairo, no objection to enable your design. On PlayStation, we need current one.

>>> Source/WebCore/platform/network/curl/CurlRequestScheduler.h:76
>>> +    std::optional<CurlMultiHandle> m_curlMultiHandle;
>> 
>> What is the reason to change it from unique_ptr to optional? If there's good reason, then use WTF::Optional instead.
> 
> Heap allocation have a cost of allocation and fragmentation, but useful for memory saving by freeing unused objects.
> However, CurlMultiHandle isn't big and CurlRequestScheduler almost always has a CurlMultiHandle.
> It doesn't make sense to separately allocate a CurlMultiHandle .
> WTF::Optional was removed in favor of std::optional (Bug 226705).

Shame on me, I didn't follow that change. And the reason is very clear. Great.

[Fujii Hironori]

(In reply to Basuke Suzuki from comment #5)
> That's be solved by this patch :) and not related to the life cycle issue we
> are discussing. You know WebView is not only for browser. Our use case
> requires that design.

I got it. Will fix.

[Fujii Hironori]

Created attachment 433656 [details]
Patch

[Fujii Hironori]

Comment on attachment 433524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433524&action=review

>>>> Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:145
>>>> +        Locker locker { m_multiHandleMutex };
>>> 
>>> m_multiHandleMutex is used only in this workerThread by design. There's no need to lock. I bed you've added the lock because you need to check the validity of multi handle before calling curl_multi_wakeup. If that's the reason, how about moving the instantiation of the multi handle to the thread creation timing in main thread. That's ensure multi handle existance while thread is running. Any way this also related to the design of the thread life cycle I mentioned at the latter part (*1).
>> 
>> I also has a same idea, and tried the idea, but it doesn't work at all.
>> I didn't look into it, but I guess that's because a multi handle might be fixed to the thread of creation time.
> 
> Okay. If I have time to investigate more, I can dig into this, but for now, safer is better. Let's add this mutex and lock area.

Oops. My bad. It was just a deadlock. CurlContext::singleton() was called as re-entrant.
CurlMultiHandle ctor calls CurlContext::singleton().
I hope you wil fix the problem as a follow-up.

[Basuke Suzuki]

Comment on attachment 433656 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433656&action=review

Thanks for understanding. And I'll try reducing the mutex after this bug.

> Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:-99
> -        m_runThread = false;

This is still required for the case when there's no running handle.

[Fujii Hironori]

Comment on attachment 433656 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433656&action=review

>> Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:-99
>> -        m_runThread = false;
> 
> This is still required for the case when there's no running handle.

Why? CurlRequestScheduler::stopThreadIfNoMoreJobRunning set m_runThread = false.

[Basuke Suzuki]

Comment on attachment 433656 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433656&action=review

>>> Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:-99
>>> -        m_runThread = false;
>> 
>> This is still required for the case when there's no running handle.
> 
> Why? CurlRequestScheduler::stopThreadIfNoMoreJobRunning set m_runThread = false.

Oh, that's right. Sorry, my bad. You are right. This isn't needed here.

[Fujii Hironori]

Committed r279993 (239736@main): <https://commits.webkit.org/239736@main>

[Radar WebKit Bug Importer]

<rdar://problem/80700058>

[Fujii Hironori]

*** Bug 227942 has been marked as a duplicate of this bug. ***