Bug 175253 - [Curl] Improve multi-threaded networking
Summary: [Curl] Improve multi-threaded networking
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daewoong Jang
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-06 17:59 PDT by Daewoong Jang
Modified: 2017-08-17 19:18 PDT (History)
10 users (show)

See Also:


Attachments
patch (114.42 KB, patch)
2017-08-06 18:07 PDT, Daewoong Jang
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (1.30 MB, application/zip)
2017-08-06 20:41 PDT, Build Bot
no flags Details
Another patch (176.80 KB, patch)
2017-08-06 21:25 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
patch (113.52 KB, patch)
2017-08-08 17:23 PDT, Daewoong Jang
no flags Details | Formatted Diff | Diff
patch (113.50 KB, patch)
2017-08-11 15:14 PDT, Daewoong Jang
no flags Details | Formatted Diff | Diff
patch part 1 (22.28 KB, patch)
2017-08-15 19:02 PDT, Daewoong Jang
achristensen: review+
achristensen: commit-queue-
Details | Formatted Diff | Diff
patch part 1 (21.94 KB, patch)
2017-08-17 16:18 PDT, Daewoong Jang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daewoong Jang 2017-08-06 17:59:04 PDT
This patch makes multi-threaded curl network more robust by hiding away curl handles from main thread. The new class ResourceHandleCurlDelegate(named after ResourceHandleCFURLConnectionDelegate) stays between threads and manages communication between them. Important thing is that curl APIs are now no longer called from main thread(except for allocation/deallocation of handle, this also should be removed later).
Comment 1 Daewoong Jang 2017-08-06 18:00:39 PDT
Related bug: https://bugs.webkit.org/show_bug.cgi?id=174845
Comment 2 Daewoong Jang 2017-08-06 18:07:00 PDT
Created attachment 317390 [details]
patch
Comment 3 Build Bot 2017-08-06 20:41:21 PDT
Comment on attachment 317390 [details]
patch

Attachment 317390 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4267347

New failing tests:
fast/block/positioning/auto/vertical-lr/001.html
Comment 4 Build Bot 2017-08-06 20:41:22 PDT
Created attachment 317396 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 5 Basuke Suzuki 2017-08-06 21:25:12 PDT
Created attachment 317397 [details]
Another patch

Beside Sony are also improving this implementation to make the port stable, I don't agree with this direction because it ties the system with ResourceHandle too much. As we've proposed to the dev mailing list, we are developing WebKit for WinCairo and that must end up with Curl port for NetworkLoadTask. Sticking around with ResourceHandle means we cannot be away form old architecture. The better way is to make a class who doesn't directly related to ResourceHandle which means can be usable from NetworkLoadTask.

I've attached our current effort which we hesitated to send now because it seems big enough to be rejected by reviewers, but because of the delay, I have to compete with others. I am so sorry about that. Can you take a look at this patch also?
Comment 6 Daewoong Jang 2017-08-06 22:49:48 PDT
(In reply to Basuke Suzuki from comment #5)
> Created attachment 317397 [details]
> Another patch
> 
> Beside Sony are also improving this implementation to make the port stable,
> I don't agree with this direction because it ties the system with
> ResourceHandle too much. As we've proposed to the dev mailing list, we are
> developing WebKit for WinCairo and that must end up with Curl port for
> NetworkLoadTask. Sticking around with ResourceHandle means we cannot be away
> form old architecture. The better way is to make a class who doesn't
> directly related to ResourceHandle which means can be usable from
> NetworkLoadTask.
> 
> I've attached our current effort which we hesitated to send now because it
> seems big enough to be rejected by reviewers, but because of the delay, I
> have to compete with others. I am so sorry about that. Can you take a look
> at this patch also?

What I'm trying to do is keeping current status stable - I don't want to get across your architectural goals. My patch is basically based on your recent work. They're not competing each other, I believe.

I have to fix curl backend if it seems unstable and it doesn't mean to interfere you. If you like to avoid that, I hope you to keep curl backend always stable while you're working on it. I'll try your patch.
Comment 7 Basuke Suzuki 2017-08-08 09:38:56 PDT
Okay, I understand. Stability is one of the most important thing than adding new feature. I agree your patch. We will try to rebase our code on top of your patch.
Comment 8 Basuke Suzuki 2017-08-08 10:11:38 PDT
Also this time, please remind that your patch blocks us to submit our patch. We want you to make this patch reviewed as quickly as possible.
Comment 9 Basuke Suzuki 2017-08-08 10:21:17 PDT
I don't agree removing callOnJobThread() from CurlJobManager. This is required from whole clients of CurlJobManager and it should be in the class.

'update()' may not happen so many times. I cannot see any positive meaning to call update() to each job every time of job's run loop.
Comment 10 Daewoong Jang 2017-08-08 17:23:04 PDT
Created attachment 317654 [details]
patch
Comment 11 Per Arne Vollan 2017-08-09 11:23:59 PDT
Are there any regressions in the http layout tests with this patch?
Comment 12 Basuke Suzuki 2017-08-09 11:38:54 PDT
Comment on attachment 317654 [details]
patch

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

This patch makes the curl port stable so that I totally +1 to this patch except those trivial things I mentioned inline.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:85
> +    CurlJobTicket job = m_job;

use auto.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:107
> +}

Why do you define alias of def/deref? I think def/deref is better inside WebCore.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:189
> +            callOnMainThread([protectedThis = makeRef(*this), url = url.isolatedCopy()] {

I've got a review from Alex the other day to capture this explicitly even if protectedThis exists.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:199
> +        callOnMainThread([protectedThis = makeRef(*this), header = header.isolatedCopy()] {

ditto.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:213
> +        protectedThis->didReceiveDataOfLength(size);

ditto.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:-286
> -

These are unused method. Safe to remove.
Comment 13 Daewoong Jang 2017-08-09 14:28:54 PDT
(In reply to Per Arne Vollan from comment #11)
> Are there any regressions in the http layout tests with this patch?

I believe there is none.
Comment 14 Daewoong Jang 2017-08-09 14:29:48 PDT
(In reply to Basuke Suzuki from comment #12)
> Comment on attachment 317654 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317654&action=review
> 
> This patch makes the curl port stable so that I totally +1 to this patch
> except those trivial things I mentioned inline.
> 
> > Source/WebCore/platform/network/curl/CurlDownload.cpp:85
> > +    CurlJobTicket job = m_job;
> 
> use auto.
> 
> > Source/WebCore/platform/network/curl/CurlDownload.cpp:107
> > +}
> 
> Why do you define alias of def/deref? I think def/deref is better inside
> WebCore.
> 
> > Source/WebCore/platform/network/curl/CurlDownload.cpp:189
> > +            callOnMainThread([protectedThis = makeRef(*this), url = url.isolatedCopy()] {
> 
> I've got a review from Alex the other day to capture this explicitly even if
> protectedThis exists.
> 
> > Source/WebCore/platform/network/curl/CurlDownload.cpp:199
> > +        callOnMainThread([protectedThis = makeRef(*this), header = header.isolatedCopy()] {
> 
> ditto.
> 
> > Source/WebCore/platform/network/curl/CurlDownload.cpp:213
> > +        protectedThis->didReceiveDataOfLength(size);
> 
> ditto.
> 
> > Source/WebCore/platform/network/curl/CurlDownload.cpp:-286
> > -
> 
> These are unused method. Safe to remove.

I left comments on review page. Thanks!
Comment 15 Basuke Suzuki 2017-08-10 10:29:14 PDT
Ah, did you submit your comments? Unless that, they're gone unfortunately...
Comment 16 Daewoong Jang 2017-08-10 15:43:39 PDT
Comment on attachment 317654 [details]
patch

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

>>> Source/WebCore/platform/network/curl/CurlDownload.cpp:85
>>> +    CurlJobTicket job = m_job;
>> 
>> use auto.
> 

CurlJobTicket is a trivial type. Are there any good reasons to use auto?

>> Source/WebCore/platform/network/curl/CurlDownload.cpp:107
>> +}
> 
> Why do you define alias of def/deref? I think def/deref is better inside WebCore.

retain() and release() are callbacks used for keeping references from worker thread.

>> Source/WebCore/platform/network/curl/CurlDownload.cpp:189
>> +            callOnMainThread([protectedThis = makeRef(*this), url = url.isolatedCopy()] {
> 
> I've got a review from Alex the other day to capture this explicitly even if protectedThis exists.

Please refer to ResourceHandleCFURLConnectionDelegate. It doesn't use this in its lambdas and I think it is better way. I think Alex told you so because <this> was being used.

>> Source/WebCore/platform/network/curl/CurlDownload.cpp:-286
>> -
> 
> These are unused method. Safe to remove.

Thank you for review.
Comment 17 Daewoong Jang 2017-08-11 04:43:33 PDT
Ran http layout tests.

base: 1637 passes
patched: 1676 passes

It seems slightly got better.
Comment 18 Daewoong Jang 2017-08-11 15:14:42 PDT
Created attachment 317969 [details]
patch
Comment 19 Per Arne Vollan 2017-08-15 09:04:41 PDT
(In reply to Daewoong Jang from comment #18)
> Created attachment 317969 [details]
> patch

This seems to be a big patch. Is it possible to break it down into smaller, independent patches?
Comment 20 Daewoong Jang 2017-08-15 19:02:34 PDT
Created attachment 318215 [details]
patch part 1
Comment 21 Daewoong Jang 2017-08-15 19:04:31 PDT
(In reply to Per Arne Vollan from comment #19)
> (In reply to Daewoong Jang from comment #18)
> > Created attachment 317969 [details]
> > patch
> 
> This seems to be a big patch. Is it possible to break it down into smaller,
> independent patches?

The original patch is divided into two pieces as you suggested.
Comment 22 Alex Christensen 2017-08-17 09:28:59 PDT
Comment on attachment 318215 [details]
patch part 1

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

r=me with a few comments.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:64
> +    m_url = url.isolatedCopy();

We make an isolated copy when we are actually going to another thread.  This seems unnecessary.

> Source/WebCore/platform/network/curl/ResourceError.h:59
> -    unsigned m_sslErrors { 0 };
> +    unsigned m_sslErrors;

Initializer lists are all the new c++11 coolness.  Removing it in favor of initializers in each constructor is a step in the wrong direction.

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:148
> +        m_curlHandle.setSslCertType("P12");

Is anyone actually testing client certificate authentication with curl?
Comment 23 Daewoong Jang 2017-08-17 16:18:40 PDT
Created attachment 318435 [details]
patch part 1
Comment 24 Daewoong Jang 2017-08-17 16:28:16 PDT
(In reply to Alex Christensen from comment #22)
> Comment on attachment 318215 [details]
> patch part 1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318215&action=review
> 
> r=me with a few comments.
> 
> > Source/WebCore/platform/network/curl/CurlDownload.cpp:64
> > +    m_url = url.isolatedCopy();
> 
> We make an isolated copy when we are actually going to another thread.  This
> seems unnecessary.
> 
> > Source/WebCore/platform/network/curl/ResourceError.h:59
> > -    unsigned m_sslErrors { 0 };
> > +    unsigned m_sslErrors;
> 
> Initializer lists are all the new c++11 coolness.  Removing it in favor of
> initializers in each constructor is a step in the wrong direction.
> 

Right. Fixed.


> > Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:148
> > +        m_curlHandle.setSslCertType("P12");
> 
> Is anyone actually testing client certificate authentication with curl?

I'm not sure. I think we need nightly build and layout test to keep wincairo right. In this patch, this is just a simple code move so I believe testing its functionality is beyond the objective of this patch.
Comment 25 WebKit Commit Bot 2017-08-17 19:17:31 PDT
Comment on attachment 318435 [details]
patch part 1

Clearing flags on attachment: 318435

Committed r220897: <http://trac.webkit.org/changeset/220897>
Comment 26 WebKit Commit Bot 2017-08-17 19:17:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 Radar WebKit Bug Importer 2017-08-17 19:18:13 PDT
<rdar://problem/33955494>