Bug 158251 – Reduce ResourceRequest copying in loading code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Reduce ResourceRequest copying in loading code

[Alex Christensen]

Created attachment 280216 [details]
Patch

[Alex Christensen]

Created attachment 280217 [details]
Patch

[Alex Christensen]

Created attachment 280218 [details]
Patch

[Carlos Garcia Campos]

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.

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 280242 [details]
Patch

[Alex Christensen]

Created attachment 280249 [details]
Patch

[Darin Adler]

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.

[Alex Christensen]

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

[Alex Christensen]

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

[Alex Christensen]

Created attachment 280420 [details]
Patch

[Alex Christensen]

Created attachment 280427 [details]
Patch

[Alex Christensen]

Created attachment 280544 [details]
Patch

[Darin Adler]

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.

[Alex Christensen]

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

[WebKit Commit Bot]

Comment on attachment 280544 [details]
Patch

Clearing flags on attachment: 280544

Committed r201708: <http://trac.webkit.org/changeset/201708>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.