Bug 45486 – Pass CachedMetadata on ResourceResponse instead of in a separate call

• Status: NEW
• Component: Page Loading

[Tony Gentilcore]

Pass CachedMetadata on ResourceResponse instead of in a separate call

[Tony Gentilcore]

Created attachment 67084 [details]
Patch

[Tony Gentilcore]

You were right, this is cleaner and should be more efficient.

[Early Warning System Bot]

Attachment 67084 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3914353

[Eric Seidel (no email)]

Attachment 67084 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3947336

[WebKit Review Bot]

Attachment 67084 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3932338

[Tony Gentilcore]

Created attachment 67118 [details]
Build fix

[Eric Seidel (no email)]

Attachment 67118 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3897338

[Tony Gentilcore]

*** Bug 45483 has been marked as a duplicate of this bug. ***

[Darin Fisher (:fishd, Google)]

Comment on attachment 67118 [details]
Build fix

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

> WebCore/loader/CachedMetadata.h:64
> +        RefPtr<CachedMetadata> cachedMetadata = create(dataTypeID(), data(), size());
don't create functions normally return PassRefPtr?  why bother with the intermediate cachedMetadata variable?

> WebCore/loader/CachedMetadata.h:68
> +    bool operator==(const CachedMetadata& other) const
not that it really matters here, but normally it is better to define operator== and operator!= at global scope so that
type coercion works properly.  as member functions, only the right-hand-side argument can be coerced.

> WebKit/chromium/public/WebCachedMetadata.h:41
> +class WebCachedMetadata {
would it make sense to use WebData in place of defining this new class?  note: WebData can be converted to a RefPtr<SharedBuffer> without any cost.

i ask because this class just looks like a wrapper for an array of bytes.  that's what WebData is.


otherwise, looks great to me!

[Tony Gentilcore]

Created attachment 68403 [details]
Patch

[Eric Seidel (no email)]

Attachment 68403 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3999092

[Adam Barth]

Comment on attachment 68403 [details]
Patch

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

> WebCore/loader/CachedMetadata.h:132
> +    return a.serialize() == b.serialize();

Woah, that looks expensive.

[Eric Seidel (no email)]

What's the status on this patch, Tony?

[Tony Gentilcore]

(In reply to comment #13)
> What's the status on this patch, Tony?

Thanks for the ping. I'm missing a file needed a on mac. I'll get it synced up and landed this week.

[Alexey Proskuryakov]

Wait, why is this a good thing? This seems to be going in a wrong direction of adding non-network data to network classes.

[Adam Barth]

(In reply to comment #15)
> Wait, why is this a good thing? This seems to be going in a wrong direction of adding non-network data to network classes.

The CachedMetadata is meta data about the response that we'd like to store in the HTTP cache.  For example, it might contain a post-parsing representation of a JavaScript script so we don't have to parse it again if we get a cache hit.

[Alexey Proskuryakov]

But that's exactly what CachedScript is doing.

[Adam Barth]

(In reply to comment #17)
> But that's exactly what CachedScript is doing.

Right, but we want to store it on disk.

[Alexey Proskuryakov]

I've been told that we want to store CachedResource to disk, in fact. I think that moving CachedResource behaviors down to ResourceRequest/Response is going in a wrong direction, and conflicts with work being done in CachedResource land.

[Adam Barth]

> I've been told that we want to store CachedResource to disk, in fact. I think that moving CachedResource behaviors down to ResourceRequest/Response is going in a wrong direction, and conflicts with work being done in CachedResource land.

The approach in this work is to integrate the disk storage with the HTTP disk cache so that the two objects can be managed together.  For example, when the we evict the resource from the HTTP disk cache, we want to evict the metadata too.

If we were to re-implement disk storage at the MemoryCache layer, we'd just be re-inventing all the mechanisms of the HTTP disk cache.

[Alexey Proskuryakov]

Comment on attachment 68403 [details]
Patch

This seems to need deeper discussion. I've already Cc'ed Antti; marking r- to reflect that discussion is needed.

[Tony Gentilcore]

I can explain some background. V8's PreCompile() method generates some metadata that makes future calls to Compile() faster (it is basically just the offset of each brace). On a particular collection of web pages, we found that about 7% of the overall page load time in chromium is spent preparsing JavaScript. So to speed up future loads we store this as metadata on CachedResource. It is stored as a generic bucket of bytes so that other CachedResource types may use this mechanism as well.

But just saving it in the memory cache is of limited usefulness. To increase the number of cases where this metadata is available, we notify the embedding application via didReceiveCachedMetadata() so that it may save it into its disk cache.

However, for chromium that ends up resulting in two IPC calls: didReceiveResponse and didReceiveCachedMetadata. The goal of this patch was to merge those into a single IPC by simply making the metadata available on the ResourceResponse which is already passed to didReceiveResponse.

I'm completely open to other ways to accomplish the same thing. Do you have time to describe the changes that are happening regarding saving CachedResources to disk?