Bug 160338 – Reduce the number of ResourceRequest copies in DocumentThreadableLoader

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

DocumentThreadableLoader is making several ResourceRequest copies.
We should try to reduce the number of copies.

[youenn fablet]

Created attachment 284860 [details]
Patch

[Darin Adler]

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.

[youenn fablet]

Created attachment 284986 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 284986 [details]
Patch for landing

Clearing flags on attachment: 284986

Committed r203971: <http://trac.webkit.org/changeset/203971>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

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

[youenn fablet]

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