Bug 183016

Summary: [Curl] Implement connection limit.
Product: WebKit Reporter: Basuke Suzuki <Basuke.Suzuki>
Component: WebCore Misc.Assignee: Basuke Suzuki <Basuke.Suzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, Basuke.Suzuki, bfulgham, commit-queue, don.olmstead, ews-watchlist, galpeter, lethalman88, pvollan, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
Patch
none
PATCH
youennf: review+
FIX none

Description Basuke Suzuki 2018-02-21 14:18:03 PST
Will add max total connections and max connections per domain. Also for Curl specific, the option for internal limitation for resource usage will be added.
Comment 1 Basuke Suzuki 2018-02-21 17:29:10 PST
Created attachment 334429 [details]
patch
Comment 2 Basuke Suzuki 2018-02-21 17:32:58 PST
*** Bug 16364 has been marked as a duplicate of this bug. ***
Comment 3 Don Olmstead 2018-02-21 18:03:18 PST
Comment on attachment 334429 [details]
patch

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

> Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:-41
> -#include <wtf/NeverDestroyed.h>
>  
>  namespace WebCore {
>  
> -CurlRequestScheduler& CurlRequestScheduler::singleton()
> +CurlRequestScheduler::CurlRequestScheduler(long maxConnects, long maxTotalConnections, long maxHostConnections)
> +    : m_maxConnects(maxConnects)
> +    , m_maxTotalConnections(maxTotalConnections)
> +    , m_maxHostConnections(maxHostConnections)
>  {
> -    static NeverDestroyed<CurlRequestScheduler> sharedInstance;
> -    return sharedInstance;

Would we really ever need more than one instance? This is more or less a global property right?
Comment 4 Basuke Suzuki 2018-02-21 19:21:37 PST
Yes, one instance per process. I didn’t change that because it was  a singleton.
Comment 5 Basuke Suzuki 2018-03-08 14:15:36 PST
Created attachment 335344 [details]
Patch
Comment 6 youenn fablet 2018-03-08 14:48:02 PST
Comment on attachment 335344 [details]
Patch

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

> Source/WebCore/platform/network/curl/CurlContext.cpp:143
> +        m_scheduler = std::make_unique<CurlRequestScheduler>(maxConnects, maxTotalConnections, maxHostConnections);

This seems to require CurlContext to be used as a singleton.
This is probably fine but I don't think you gain much at this design compared to allocating m_scheduler in CurlContext constructor as a regular member (?) and the latter approach is probably easier to read.
You could also make m_scheduler a UniqueRef<>.
Comment 7 Basuke Suzuki 2018-03-08 15:41:36 PST
Comment on attachment 335344 [details]
Patch

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

Thanks.

>> Source/WebCore/platform/network/curl/CurlContext.cpp:143
>> +        m_scheduler = std::make_unique<CurlRequestScheduler>(maxConnects, maxTotalConnections, maxHostConnections);
> 
> This seems to require CurlContext to be used as a singleton.
> This is probably fine but I don't think you gain much at this design compared to allocating m_scheduler in CurlContext constructor as a regular member (?) and the latter approach is probably easier to read.
> You could also make m_scheduler a UniqueRef<>.

CurlContext is designed as a singleton already. It can exist only one instance per process. We need to pass dynamic parameters to Scheduler, so we cannot make it a simple member variable. So unique_ptr or UniqueRef must be used at that time and instantiation like this code will be appeared on the CurlContext constructor. I think the complexity of code is similar and this code has a benefit of the lazy instantication.

On the other hand, I didn't measure the cost of call_once() here. I assume the cost is pretty similar to GCD's dispatch_once, but still some sort of cost exists compared to instantiation at the constructor.

The actual use case will be 99.9% of chance for scheduler to be instantiated, so moving to constructor may be the better choice.

I don't think UniqueRef needed here. UniqueRef exposes exclusiveness of Ref object but in this case Scheduler is simple class. No need to switch to Ref.
Comment 8 Basuke Suzuki 2018-03-08 17:10:40 PST
Created attachment 335369 [details]
PATCH

Move instantiation of scheduler into constructor.
Comment 9 youenn fablet 2018-03-08 17:23:51 PST
Comment on attachment 335369 [details]
PATCH

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

> Source/WebCore/platform/network/curl/CurlContext.cpp:238
> +

If we call setMaxConnects once to a value and then setMaxConnects to -1, shouldn't we update m_multiHandle as well?
If we only call it once, maybe maxConnects (and below parameters as well) should be passed in CurlMultiHandle constructor.

> Source/WebCore/platform/network/curl/CurlContext.h:121
> +    CurlRequestScheduler& scheduler() { return *m_scheduler; }

If m_scheduler was a UniqueRef<CurlRequestScheduler>, there would be no need for '*'.
A UniqueRef is just a unique_ptr that cannot have a null pointer.
Comment 10 Basuke Suzuki 2018-03-08 21:57:35 PST
(In reply to youenn fablet from comment #9)
> Comment on attachment 335369 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335369&action=review
> 
> > Source/WebCore/platform/network/curl/CurlContext.cpp:238
> > +
> 
> If we call setMaxConnects once to a value and then setMaxConnects to -1,
> shouldn't we update m_multiHandle as well?
> If we only call it once, maybe maxConnects (and below parameters as well)
> should be passed in CurlMultiHandle constructor.

We design these values as configuration variables, which can be set at the initialization time. That's why they are passed by environment variable. Once our NetworkProcess implementation are in public, we will switch them via CreationParameter.

 
> > Source/WebCore/platform/network/curl/CurlContext.h:121
> > +    CurlRequestScheduler& scheduler() { return *m_scheduler; }
> 
> If m_scheduler was a UniqueRef<CurlRequestScheduler>, there would be no need
> for '*'.
> A UniqueRef is just a unique_ptr that cannot have a null pointer.

Interesting. Good to know. I miss understood about that class. Still we cannot use this because we have to set the value at constructor.
Comment 11 Basuke Suzuki 2018-03-08 22:00:32 PST
> Youenn Fablet.

Thanks for r+.
Comment 12 Basuke Suzuki 2018-03-08 22:01:01 PST
Created attachment 335392 [details]
FIX
Comment 13 WebKit Commit Bot 2018-03-09 10:51:35 PST
Comment on attachment 335392 [details]
FIX

Clearing flags on attachment: 335392

Committed r229471: <https://trac.webkit.org/changeset/229471>
Comment 14 WebKit Commit Bot 2018-03-09 10:51:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2018-03-09 10:52:20 PST
<rdar://problem/38308596>