MemoryCache: Enable MemoryCache to have multiple isolated resource maps

MemoryCache: Enable MemoryCache to have multiple isolated resource maps

This CL enables MemoryCache to store multiple resources for 1 URL so that
Service Worker can utilize the cache system.

Patch dependency
[1] Chromium(Get ServiceWorkerID): https://codereview.chromium.org/669303003/
[2] Blink(Get ServiceWorkerID): https://codereview.chromium.org/650033007/
[3] Blink(Tweak MemoryCache): THIS PATCH


BUG=423219
TEST=webkit_unit_tests --gtest_filter=MemoryCacheTest.*:ResourceFetcherTest.*:CachingCorrectnessTest.*:ImageResourceTest.*:RawResourceTest.*
TEST=run_webkit_tests.py --debug http/tests/serviceworker/chromium/memory-cache.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184613

[nhiroki, 2014-10-08 13:33:19]

Patchset #1 (id:1) has been deleted

[nhiroki, 2014-10-08 13:33:27]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2014-10-08 13:33:33]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2014-10-08 13:33:41]

Patchset #1 (id:60001) has been deleted

[nhiroki, 2014-10-08 13:33:48]

Patchset #1 (id:80001) has been deleted

[nhiroki, 2014-10-08 13:33:53]

Patchset #1 (id:100001) has been deleted

[nhiroki, 2014-10-20 16:19:37]

Patchset #1 (id:120001) has been deleted

[nhiroki, 2014-10-20 16:19:51]

Patchset #1 (id:140001) has been deleted

[nhiroki, 2014-10-20 16:20:00]

Patchset #1 (id:160001) has been deleted

[nhiroki, 2014-10-20 16:20:18]

Patchset #1 (id:180001) has been deleted

[nhiroki, 2014-10-20 16:20:44]

Patchset #1 (id:200001) has been deleted

[nhiroki, 2014-10-20 16:20:55]

Patchset #1 (id:220001) has been deleted

[nhiroki, 2014-10-20 16:26:54]

Patchset #1 (id:230001) has been deleted

[nhiroki, 2014-10-21 11:01:23]

Patchset #1 (id:250001) has been deleted

[nhiroki, 2014-10-21 11:03:18]

nhiroki@chromium.org changed reviewers:
+ japhet@chromium.org, mkwst@chromium.org

[nhiroki, 2014-10-21 11:13:30]

Hi Nate and Mike,

Could you take a look at this WIP CL? I'd like to check whether I'm on the right
track or not...

As you may know, MemoryCache is now disabled for Service Worker to avoid cache
pollution (https://codereview.chromium.org/632213002), but in terms of
performance it'd be preferable to use the memory cache.　The problem is that
resources tainted by Service Worker can be mingled with other resources in the
cache, so this CL tries to isolate cache spaces for sw-controlled-resources from
others.

I've written the design doc for this work:
https://docs.google.com/document/d/1eZz3MZ606jQtD2u1SHx8UTguGdMNk6z7iVh54G43a...

Any feedback and suggestion would be greatly appreciated. Thanks.

[Mike West, 2014-10-21 13:18:24]

Thanks for sending this out!

1. What does the performance impact look like? It seems unfortunate that we're
slowing down every request based upon the fact that some page, somewhere, might
have a Service Worker.

2. Did you consider having two paths into the API so that this hit would only
occur for requests that are actually going through a SW? That is, "regular"
requests would use the current API, and talk to `ResourceMap
m_resourcesForRegularWebsites;`, while SW requests would talk to the new
ResourceMapIndex structure?

3. Should every SW have a separate ResourceMap, or should same-origin SWs share
a ResourceMap? It's not clear to me that we can protect them from each other,
nor that we should try.

4. What are your estimates for the amount of duplication in the cache that this
might create? The deduplication strategy seems reasonable (though not
implemented here?); do you have a feel for how that might look, and how much it
mght save?

[nhiroki, 2014-10-22 11:47:31]

Thank you for your comments! Replied inline.

On 2014/10/21 13:18:24, Mike West wrote:
> Thanks for sending this out!
> 
> 1. What does the performance impact look like? It seems unfortunate that we're
> slowing down every request based upon the fact that some page, somewhere,
might
> have a Service Worker.

If MemoryCache is disabled, loading time of our demo
application　(https://github.com/jakearchibald/trained-to-thrill) is average 80
msec slower. I added an appendix about the benchmark in the design doc. Please
see it for more detailed
(https://docs.google.com/a/chromium.org/document/d/1eZz3MZ606jQtD2u1SHx8UTguGd...).

> 2. Did you consider having two paths into the API so that this hit would only
> occur for requests that are actually going through a SW? That is, "regular"
> requests would use the current API, and talk to `ResourceMap
> m_resourcesForRegularWebsites;`, while SW requests would talk to the new
> ResourceMapIndex structure?

'the API' means MemoryCache::add(),remove(),contains() etc, right? I came up
with the idea before but didn't adopt it for a prototype because apparently
another API set makes callsites of the API complicated. But when I think back on
it now, minimizing effects on the existing API looks attractive. I'll take a
close look at the approach.

> 3. Should every SW have a separate ResourceMap, or should same-origin SWs
share
> a ResourceMap? It's not clear to me that we can protect them from each other,
> nor that we should try.

Sharing a ResourceMap among same-origin SWs would be a lower risk compared with
sharing it among non-same-origin SWs. Ideally, every same-origin-SW should also
have a separate ResourceMap because they can serve a different response for a
certain URL and an unexpected cache might make application developers confused.

> 4. What are your estimates for the amount of duplication in the cache that
this
> might create? The deduplication strategy seems reasonable (though not
> implemented here?); do you have a feel for how that might look, and how much
it
> mght save?

I think most of resources are not duplicated because cache duplication happens
when controlled and non-controlled documents refer to the same resource. I guess
that images widely used over controlled and non-controlled　pages (eg. corporate
logo, navigation header) and hosted script files (eg. jQuery, AngularJS) could
be applicable to the condition, but those seem minority.

And, I'm wondering if we cannot deduplicate resources in most cases. If a
developer want to make his/her application offline-ready, the developer lets a
service worker cache resources of the app in SW's cache (ServiceWorker CacheAPI)
and respond with them. In this case, the cache system cannot deduplicate them
even if they are not modified by SW because the system is not able to ensure
that.

[nhiroki, 2014-10-24 07:10:10]

Brushed up the entire implementation and removed the WIP mark. Could you take
another look?


> > 2. Did you consider having two paths into the API so that this hit would
only
> > occur for requests that are actually going through a SW? That is, "regular"
> > requests would use the current API, and talk to `ResourceMap
> > m_resourcesForRegularWebsites;`, while SW requests would talk to the new
> > ResourceMapIndex structure?
> 
> 'the API' means MemoryCache::add(),remove(),contains() etc, right? I came up
> with the idea before but didn't adopt it for a prototype because apparently
> another API set makes callsites of the API complicated. But when I think back
on
> it now, minimizing effects on the existing API looks attractive. I'll take a
> close look at the approach.

I considered this approach, but still keep one ResourceMapIndex strategy
because...

- for readability, one ResourceMapIndex would be simpler than having a separate
ResourceMap.
- for performance, the regular request needs an extra lookup of ResourceMapIndex
compared to the existing implementation but the index shouldn't be so big (at
most the number of controlling workers in a renderer process) and I doubt if the
lookup can actually affect the performance. I'll conduct a benchmark
(page_cycler test would be suitable for this purpose?) and share the result
soon.


> > 4. What are your estimates for the amount of duplication in the cache that
> this
> > might create? The deduplication strategy seems reasonable (though not
> > implemented here?); do you have a feel for how that might look, and how much
> it
> > mght save?
> 
> I think most of resources are not duplicated because cache duplication happens
> when controlled and non-controlled documents refer to the same resource. I
guess
> that images widely used over controlled and non-controlled　pages (eg.
corporate
> logo, navigation header) and hosted script files (eg. jQuery, AngularJS) could
> be applicable to the condition, but those seem minority.
> 
> And, I'm wondering if we cannot deduplicate resources in most cases. If a
> developer want to make his/her application offline-ready, the developer lets a
> service worker cache resources of the app in SW's cache (ServiceWorker
CacheAPI)
> and respond with them. In this case, the cache system cannot deduplicate them
> even if they are not modified by SW because the system is not able to ensure
> that.

The patchset2 doesn't include the deduplication mechanism (I think we don't have
to implement it until we actually recognize the need of that).

[nhiroki, 2014-10-24 07:12:32]

nhiroki@chromium.org changed reviewers:
+ dominicc@chromium.org, horo@chromium.org

[nhiroki, 2014-10-24 07:12:33]

+dominicc@ and horo@ for reviews from ServiceWorker POV.

[nhiroki, 2014-10-24 08:02:18]

nhiroki@chromium.org changed reviewers:
- dominicc@chromium.org, horo@chromium.org

[Mike West, 2014-10-24 12:43:00]

The code change looks pretty reasonable, thanks. I'm still quite curious about
performance numbers.

I'm also curious as to how you intend to test this change. It appears that both
the *Test files, as well as the internals change, all use the `emptyString()`
cache identifier all the time, which means that you're not running through
substantial portions of the new code.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
File Source/core/fetch/MemoryCache.cpp (right):

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:164: m_resources.add(cacheIdentifier,
adoptPtrWillBeNoop(new ResourceMap()));
`add()` can fail, can't it? That could cause a null-deref on line 173.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:195: if (!contains(resource))
Rather than doing the lookup twice, it might make sense to phrase it as:

`if (ResourceCacheEntry* entry = getEntryForResource(resource)) { ... }`

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:204: ResourceMap* resources =
m_resources.get(resource->cacheIdentifier());
It's odd to have `getResourceMap` and not use it here. I understand why; you
don't want the side-effect of creating the resourceMap if it doesn't exist.
Perhaps you could rename `getResourceMap` to `ensureResourceMap` or something
similar to resolve the confusion?

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:234: for (const auto& resources : m_resources)
{
"ResourceMap" seems clearer than "auto".

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:428: if (resource->url().isNull())
isNull or isEmpty?

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:641: for (const auto& resource : resources)
"Resource" seems clearer than "auto".

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:663: for (const auto& resources : m_resources)
{
"ResourceMap* resourceMap" seems clearer.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:664: for (const auto& resourceIter :
*resources.value) {
Resource rather than auto.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:698: for (;;) {
Would you mind changing this, and line 693, to `while (true)`? The latter is
much more common in Blink code.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
File Source/core/fetch/MemoryCache.h (right):

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.h:172: Resource* resourceForURL(const String&
cacheIdentifier, const KURL&);
If you make |cacheIdentifier| the second argument, you can make it optional, and
avoid modifications at the various call sites that don't care about SW.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.h:289: typedef WillBeHeapHashMap<String,
OwnPtrWillBeMember<ResourceMap>> ResourceMapIndex;
Nit: These should be converted to the new `using` syntax.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.h:291: ResourceMapIndex m_resources;
Nit: Perhaps `m_resourceMaps`, now?

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Resou...
File Source/core/fetch/Resource.cpp (left):

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Resou...
Source/core/fetch/Resource.cpp:145: ASSERT(url().isNull() ||
memoryCache()->resourceForURL(KURL(ParsedURLString, url())) != this);
Why is it safe to drop this ASSERT?

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:338: if (Resource* resource =
resourceFromDataURIRequest(request.resourceRequest(), request.options(),
cacheIdentifier)) {
cacheIdentifier should always be either first or last. Right now it's last here,
and first in resourceForURL.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:1513: String
ResourceFetcher::createCacheIdentifier() const
"create" seems wrong here. "get"?

https://codereview.chromium.org/640463003/diff/290001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/640463003/diff/290001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:358: // only used for testing, so the default
identifier should be ok for now.
Doesn't this mean you're not going to get the correct test results when running
this for resources loaded via SW? How do you intend to test this otherwise?

[nhiroki, 2014-10-27 14:26:19]

Patchset #5 (id:350001) has been deleted

[nhiroki, 2014-10-27 14:27:10]

Patchset #4 (id:330001) has been deleted

[nhiroki, 2014-10-27 14:28:58]

Patchset #4 (id:370001) has been deleted

[nhiroki, 2014-10-27 14:30:14]

Thank you for reviewing! Updated.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
File Source/core/fetch/MemoryCache.cpp (right):

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:164: m_resources.add(cacheIdentifier,
adoptPtrWillBeNoop(new ResourceMap()));
On 2014/10/24 12:42:59, Mike West wrote:
> `add()` can fail, can't it? That could cause a null-deref on line 173.

Hmmm... I think a null-deref never happens because "add()" can fail (can do
nothing) only if "m_resources" has already contained a key "cacheIdentifier" and
it's confirmed at line 163. You mean other cases? (eg. out-of-memory??)

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:195: if (!contains(resource))
On 2014/10/24 12:42:59, Mike West wrote:
> Rather than doing the lookup twice, it might make sense to phrase it as:
> 
> `if (ResourceCacheEntry* entry = getEntryForResource(resource)) { ... }`

Good point! Removed redundant contains() calls.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:204: ResourceMap* resources =
m_resources.get(resource->cacheIdentifier());
On 2014/10/24 12:43:00, Mike West wrote:
> It's odd to have `getResourceMap` and not use it here. I understand why; you
> don't want the side-effect of creating the resourceMap if it doesn't exist.

Yes, that's right.

> Perhaps you could rename `getResourceMap` to `ensureResourceMap` or something
> similar to resolve the confusion?

Sounds reasonable. Renamed it to ensureResourceMap.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:234: for (const auto& resources : m_resources)
{
On 2014/10/24 12:42:59, Mike West wrote:
> "ResourceMap" seems clearer than "auto".

"m_resources" is a HashMap object, so this "auto" is not inferred as
"ResourceMap" but "ResourceMapIndex::ValueType". You mean ...

for (const OwnPtr<ResourceMap>& resources : m_resources.values()) {...}

? Or am I missing something?

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:428: if (resource->url().isNull())
On 2014/10/24 12:43:00, Mike West wrote:
> isNull or isEmpty?

Done.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:641: for (const auto& resource : resources)
On 2014/10/24 12:42:59, Mike West wrote:
> "Resource" seems clearer than "auto".

Done.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:663: for (const auto& resources : m_resources)
{
On 2014/10/24 12:42:59, Mike West wrote:
> "ResourceMap* resourceMap" seems clearer.

Ditto ("const OwnPtr<ResourceMap>& resources"?).

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:664: for (const auto& resourceIter :
*resources.value) {
On 2014/10/24 12:42:59, Mike West wrote:
> Resource rather than auto.

Ditto.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.cpp:698: for (;;) {
On 2014/10/24 12:42:59, Mike West wrote:
> Would you mind changing this, and line 693, to `while (true)`? The latter is
> much more common in Blink code.

Done.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
File Source/core/fetch/MemoryCache.h (right):

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.h:172: Resource* resourceForURL(const String&
cacheIdentifier, const KURL&);
On 2014/10/24 12:43:00, Mike West wrote:
> If you make |cacheIdentifier| the second argument, you can make it optional,
and
> avoid modifications at the various call sites that don't care about SW.

Done.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.h:289: typedef WillBeHeapHashMap<String,
OwnPtrWillBeMember<ResourceMap>> ResourceMapIndex;
On 2014/10/24 12:43:00, Mike West wrote:
> Nit: These should be converted to the new `using` syntax.

Done.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Memor...
Source/core/fetch/MemoryCache.h:291: ResourceMapIndex m_resources;
On 2014/10/24 12:43:00, Mike West wrote:
> Nit: Perhaps `m_resourceMaps`, now?

Done.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Resou...
File Source/core/fetch/Resource.cpp (left):

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Resou...
Source/core/fetch/Resource.cpp:145: ASSERT(url().isNull() ||
memoryCache()->resourceForURL(KURL(ParsedURLString, url())) != this);
On 2014/10/24 12:43:00, Mike West wrote:
> Why is it safe to drop this ASSERT?

This case would be covered by ASSERT(!memoryCache()->contains(this)) at line 143

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:338: if (Resource* resource =
resourceFromDataURIRequest(request.resourceRequest(), request.options(),
cacheIdentifier)) {
On 2014/10/24 12:43:00, Mike West wrote:
> cacheIdentifier should always be either first or last. Right now it's last
here,
> and first in resourceForURL.

Done. I made "resourceForURL" receive "cacheIdentifier" as the last argument.

https://codereview.chromium.org/640463003/diff/290001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:1513: String
ResourceFetcher::createCacheIdentifier() const
On 2014/10/24 12:43:00, Mike West wrote:
> "create" seems wrong here. "get"?

Done.

https://codereview.chromium.org/640463003/diff/290001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/640463003/diff/290001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:358: // only used for testing, so the default
identifier should be ok for now.
On 2014/10/24 12:43:00, Mike West wrote:
> Doesn't this mean you're not going to get the correct test results when
running
> this for resources loaded via SW? How do you intend to test this otherwise?

I found the way to get the key here, so removed this comment.

[nhiroki, 2014-10-27 14:43:57]

On 2014/10/24 12:43:00, Mike West wrote:
> The code change looks pretty reasonable, thanks. I'm still quite curious about
> performance numbers.

Measured page loading time on non-controlled pages and controlled pages:
https://docs.google.com/a/chromium.org/document/d/1eZz3MZ606jQtD2u1SHx8UTguGd...

I think this change works well for controlled-pages and its effect on
non-controlled pages would be very small. Wdyt?

> I'm also curious as to how you intend to test this change. It appears that
both
> the *Test files, as well as the internals change, all use the `emptyString()`
> cache identifier all the time, which means that you're not running through
> substantial portions of the new code.

Added basic unit tests for MemoryCache (MemoryCacheTest.cpp) and tweaked an
existing layout test (memory-cache.html) which tries to access the same URL from
non-controlled document and controlled iframe via JSONP and compare their
results.

And I'll investigate how to effectively test resource eviction in multiple
resource maps case.

[nhiroki, 2014-10-28 15:10:50]

On 2014/10/27 14:43:57, nhiroki wrote:
> On 2014/10/24 12:43:00, Mike West wrote:
> > I'm also curious as to how you intend to test this change. It appears that
> both
> > the *Test files, as well as the internals change, all use the
`emptyString()`
> > cache identifier all the time, which means that you're not running through
> > substantial portions of the new code.
> 
> Added basic unit tests for MemoryCache (MemoryCacheTest.cpp) and tweaked an
> existing layout test (memory-cache.html) which tries to access the same URL
from
> non-controlled document and controlled iframe via JSONP and compare their
> results.
> 
> And I'll investigate how to effectively test resource eviction in multiple
> resource maps case.

Added more unittests and confirmed that those work fine on my local env. Could
you take another look? Thanks.

[Mike West, 2014-10-29 07:24:16]

LGTM, thank you for both the performance numbers and the additional tests.

I still think it's worth keeping an eye on the performance bots after landing
this to make sure that your experimental results are borne out on trunk. Thanks!

[nhiroki, 2014-10-29 23:30:45]

On 2014/10/29 07:24:16, Mike West (OOO until 3rd) wrote:
> LGTM, thank you for both the performance numbers and the additional tests.
> 
> I still think it's worth keeping an eye on the performance bots after landing
> this to make sure that your experimental results are borne out on trunk.
Thanks!

Thank you! Okay, I'll be monitoring the perf bots :)

[nhiroki, 2014-10-29 23:54:06]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-10-29 23:55:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/640463003/450001

[commit-bot: I haz the power, 2014-10-30 01:47:42]

Committed patchset #7 (id:450001) as 184613