Pass CachedMetadata on ResourceResponse instead of in a separate call
Created attachment 67084 [details] Patch
You were right, this is cleaner and should be more efficient.
Attachment 67084 [details] did not build on qt: Build output: http://queues.webkit.org/results/3914353
Attachment 67084 [details] did not build on mac: Build output: http://queues.webkit.org/results/3947336
Attachment 67084 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3932338
Created attachment 67118 [details] Build fix
Attachment 67118 [details] did not build on mac: Build output: http://queues.webkit.org/results/3897338
*** Bug 45483 has been marked as a duplicate of this bug. ***
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!
Created attachment 68403 [details] Patch
Attachment 68403 [details] did not build on mac: Build output: http://queues.webkit.org/results/3999092
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.
What's the status on this patch, Tony?
(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.
Wait, why is this a good thing? This seems to be going in a wrong direction of adding non-network data to network classes.
(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.
But that's exactly what CachedScript is doing.
(In reply to comment #17) > But that's exactly what CachedScript is doing. Right, but we want to store it on disk.
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.
> 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.
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.
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?