Bug 183016

Created attachment 334429 [details] patch

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

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?

Yes, one instance per process. I didn’t change that because it was a singleton.

Created attachment 335344 [details] Patch

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 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.

Created attachment 335369 [details] PATCH Move instantiation of scheduler into constructor.

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.

(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.

> Youenn Fablet. Thanks for r+.

Created attachment 335392 [details] FIX

Comment on attachment 335392 [details] FIX Clearing flags on attachment: 335392 Committed r229471: <https://trac.webkit.org/changeset/229471>

All reviewed patches have been landed. Closing bug.

<rdar://problem/38308596>