DocumentThreadableLoader is making several ResourceRequest copies. We should try to reduce the number of copies.
Created attachment 284860 [details] Patch
Comment on attachment 284860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284860&action=review Not sure if the comments about copies are all that helpful unless we have a concrete idea of how to cut down on the copying. How much does this cut down on copying? Did you run some test and count how many times we made copies? > Source/WebCore/loader/DocumentLoader.cpp:1514 > + CachedResourceRequest cachedResourceRequest(ResourceRequest(request), mainResourceLoadOptions); Is this needed? Do we get some kind of compiler warning if you don’t explicitly copy it like this? > Source/WebCore/loader/DocumentThreadableLoader.cpp:250 > + // FIXME: Can we find a way to remove that copy? Grammatically this should be "this copy". > Source/WebCore/loader/MediaResourceLoader.cpp:68 > + // FIXME: We should try to remove the copy of request ehwn crearing cacheRequest. Typo: "ehwn". > Source/WebCore/loader/ThreadableLoader.cpp:61 > +RefPtr<ThreadableLoader> ThreadableLoader::create(ScriptExecutionContext* context, ThreadableLoaderClient* client, ResourceRequest&& request, const ThreadableLoaderOptions& options) Interesting to note that both client and context should be references here and I think the result should be a Ref, not a RefPtr. > Source/WebCore/loader/ThreadableLoader.cpp:72 > +void ThreadableLoader::loadResourceSynchronously(ScriptExecutionContext* context, ResourceRequest&& request, ThreadableLoaderClient& client, const ThreadableLoaderOptions& options) And context should be a reference here.
Created attachment 284986 [details] Patch for landing
Comment on attachment 284986 [details] Patch for landing Clearing flags on attachment: 284986 Committed r203971: <http://trac.webkit.org/changeset/203971>
All reviewed patches have been landed. Closing bug.
(In reply to comment #2) > Comment on attachment 284860 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=284860&action=review > > Not sure if the comments about copies are all that helpful unless we have a > concrete idea of how to cut down on the copying. I removed the one for DTL. For MediaResourceLoader, this should be doable. > How much does this cut down on copying? Did you run some test and count how > many times we made copies? I did not the counting, this patch comes from other patch discussions on fetch API we had Alex and I. Fetch algorithm requires one request cloning to replay it if needed (redirections, authentication...). We probably need 1 or 2 more for caching purposes. Ideally, we should not do may copies. But we are also making copies because we are doing changes to the request at various places. We should try to reduce these places. > > Source/WebCore/loader/DocumentLoader.cpp:1514 > > + CachedResourceRequest cachedResourceRequest(ResourceRequest(request), mainResourceLoadOptions); > > Is this needed? Do we get some kind of compiler warning if you don’t > explicitly copy it like this? request is a ResourceRequest& so we will get a compilation error. > > Source/WebCore/loader/DocumentThreadableLoader.cpp:250 > > + // FIXME: Can we find a way to remove that copy? > > Grammatically this should be "this copy". OK. I removed it since it might be difficult to fix that. > > Source/WebCore/loader/MediaResourceLoader.cpp:68 > > + // FIXME: We should try to remove the copy of request ehwn crearing cacheRequest. > > Typo: "ehwn". OK. I kept this one since it should be ok to fix it. > > Source/WebCore/loader/ThreadableLoader.cpp:61 > > +RefPtr<ThreadableLoader> ThreadableLoader::create(ScriptExecutionContext* context, ThreadableLoaderClient* client, ResourceRequest&& request, const ThreadableLoaderOptions& options) > > Interesting to note that both client and context should be references here > and I think the result should be a Ref, not a RefPtr. > > > Source/WebCore/loader/ThreadableLoader.cpp:72 > > +void ThreadableLoader::loadResourceSynchronously(ScriptExecutionContext* context, ResourceRequest&& request, ThreadableLoaderClient& client, const ThreadableLoaderOptions& options) > > And context should be a reference here. I'll try to do this refactoring.
> > > Source/WebCore/loader/ThreadableLoader.cpp:61 > > > +RefPtr<ThreadableLoader> ThreadableLoader::create(ScriptExecutionContext* context, ThreadableLoaderClient* client, ResourceRequest&& request, const ThreadableLoaderOptions& options) > > > > Interesting to note that both client and context should be references here > > and I think the result should be a Ref, not a RefPtr. > > > > > Source/WebCore/loader/ThreadableLoader.cpp:72 > > > +void ThreadableLoader::loadResourceSynchronously(ScriptExecutionContext* context, ResourceRequest&& request, ThreadableLoaderClient& client, const ThreadableLoaderOptions& options) > > > > And context should be a reference here. > > I'll try to do this refactoring. Patch for bug 160404 includes this refactoring.