Bug 158251

Summary: Reduce ResourceRequest copying in loading code
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, cdumez, cgarcia, commit-queue, danw, gns, japhet, mcatanzaro, mrobinson
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Fix the soup build
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Alex Christensen 2016-05-31 23:12:12 PDT
Reduce ResourceRequest copying in loading code
Comment 1 Alex Christensen 2016-05-31 23:33:04 PDT
Created attachment 280216 [details]
Patch
Comment 2 Alex Christensen 2016-05-31 23:41:51 PDT
Created attachment 280217 [details]
Patch
Comment 3 Alex Christensen 2016-06-01 00:01:32 PDT
Created attachment 280218 [details]
Patch
Comment 4 Carlos Garcia Campos 2016-06-01 00:43:13 PDT
Created attachment 280221 [details]
Fix the soup build

This fixes the soup build, but I'm not sure we are really reducing copies, specially in the case of the redirect response, since we have to copy before move in several places.
Comment 5 Alex Christensen 2016-06-01 09:07:22 PDT
Comment on attachment 280221 [details]
Fix the soup build

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

The number of copies of the libsoup ResourceHandle-based loading in WK2 loading is unaffected.
The number of copies of the Mac non-NetworkSession loading in WK2 is reduced by 1.
The number of copies of the Mac NetworkSession loading in WK2 is reduced by 1.
This patch is a net improvement.

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:-508
> -    ResourceRequest newRequest = request;

This is one fewer copy for Mac non-NetworkSession loading.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:-473
> -    ResourceRequest newRequest(request);

This is one fewer copy for libsoup loading.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:534
> +    ResourceResponse responseCopy = d->m_response;

This is one more copy for libsoup loading.

> Source/WebKit2/NetworkProcess/NetworkLoad.cpp:128
> +        ResourceRequest currentRequestCopy = m_currentRequest;

This is one more copy for all loads not using NetworkSession in WebKit2.

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:-85
> -    ResourceRequest newRequest = proposedRequest;

This is one fewer copy for all loads in WebKit2.
Comment 6 Alex Christensen 2016-06-01 09:18:42 PDT
Created attachment 280242 [details]
Patch
Comment 7 Alex Christensen 2016-06-01 11:16:22 PDT
Created attachment 280249 [details]
Patch
Comment 8 Darin Adler 2016-06-02 09:21:25 PDT
Comment on attachment 280249 [details]
Patch

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

Looks like the compilation error on Windows is real; seems like continueWillSendRequest in ResourceHandleCFNet.cpp needs to be updated.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:292
> +    // FIXME: There is functionality in ResourceHandleMac.mm that does not have corresponding functionality here.

Could you be more specific with this comment? This is broad and vague, and I think something more specific would have more value.

> Source/WebKit2/NetworkProcess/NetworkLoad.cpp:130
> -    } else if (m_handle)
> -        m_handle->continueWillSendRequest(m_currentRequest);
> +    } else if (m_handle) {
> +        ResourceRequest currentRequestCopy = m_currentRequest;
> +        m_handle->continueWillSendRequest(WTFMove(currentRequestCopy));
> +    }

Is this change really needed? Do we get a compile error with the old code, or do we get incorrect behavior? This seems like what I would have expected if I didn’t call WTFMove. Even if there is a need to be explicit, maybe we could do it without a local variable. Like maybe one of these:

    m_handle->continueWillSendRequest(ResourceRequest { m_currentRequest });

    m_handle->continueWillSendRequest({ m_currentRequest });

    auto currentRequestCopy = m_currentRequest;
    m_handle->continueWillSendRequest(WTFMove(currentRequestCopy));

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:83
>      RefPtr<WebResourceLoader> protect(this);

Should be Ref, not RefPtr.
Comment 9 Alex Christensen 2016-06-02 20:19:41 PDT
(In reply to comment #8)
> > Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:292
> > +    // FIXME: There is functionality in ResourceHandleMac.mm that does not have corresponding functionality here.
I'm removing this comment.  The corresponding functionality is in ResourceHandleCFURLConnectionDelegate::createResourceRequest and tests pass.
Comment 10 Alex Christensen 2016-06-02 20:21:47 PDT
(In reply to comment #8)
> Is this change really needed? Do we get a compile error with the old code,
> or do we get incorrect behavior? 
I'm pretty sure there was a compiler error.  I also wanted it to be as obvious as possible that there was a copy being made because I think this copy can also be avoided.
Comment 11 Alex Christensen 2016-06-02 20:25:05 PDT
Created attachment 280420 [details]
Patch
Comment 12 Alex Christensen 2016-06-02 23:29:44 PDT
Created attachment 280427 [details]
Patch
Comment 13 Alex Christensen 2016-06-04 22:31:45 PDT
Created attachment 280544 [details]
Patch
Comment 14 Darin Adler 2016-06-05 10:49:04 PDT
Comment on attachment 280544 [details]
Patch

In some of the code paths the ownership seems great. In some others, there are quite a few copies and it’s not clear to me they are all needed, such as in NetworkLoad::sharedWillSendRedirectedRequest and NetworkResourceLoader::willSendRedirectedRequest.
Comment 15 Alex Christensen 2016-06-06 01:03:40 PDT
(In reply to comment #14)
> Comment on attachment 280544 [details]
> Patch
> 
> In some of the code paths the ownership seems great. In some others, there
> are quite a few copies and it’s not clear to me they are all needed, such as
> in NetworkLoad::sharedWillSendRedirectedRequest and
> NetworkResourceLoader::willSendRedirectedRequest.
I agree.  I think there's room for improvement.  I think sometimes we only need to actually keep part of the original request, like just the URL.
Comment 16 WebKit Commit Bot 2016-06-06 01:23:51 PDT
Comment on attachment 280544 [details]
Patch

Clearing flags on attachment: 280544

Committed r201708: <http://trac.webkit.org/changeset/201708>
Comment 17 WebKit Commit Bot 2016-06-06 01:23:57 PDT
All reviewed patches have been landed.  Closing bug.