Reduce ResourceRequest copying in loading code
Created attachment 280216 [details] Patch
Created attachment 280217 [details] Patch
Created attachment 280218 [details] Patch
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 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.
Created attachment 280242 [details] Patch
Created attachment 280249 [details] Patch
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.
(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.
(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.
Created attachment 280420 [details] Patch
Created attachment 280427 [details] Patch
Created attachment 280544 [details] Patch
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.
(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 on attachment 280544 [details] Patch Clearing flags on attachment: 280544 Committed r201708: <http://trac.webkit.org/changeset/201708>
All reviewed patches have been landed. Closing bug.