Bug 112943

Summary: If a previously loaded resource is later stored to the disk cache, replace the buffer with MMAP'ed memory
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, buildbot, ggaren, japhet, rniwa, sam, webkit-ews, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: All   
Attachments:
Description Flags
Patch v1
webkit-ews: commit-queue-
Patch v2 - Style and build fixes
ggaren: review-
Patch v3
ggaren: review+
Updated patch for EWS run none

Description Brady Eidson 2013-03-21 11:43:12 PDT 
If a previously loaded resource is later stored to the disk cached, replace the buffer with MMAP'ed memory.

This gives us a memory win multiplied by the number of times the resource is referenced in a different WebProcess.

<rdar://problem/13414154>
Comment 1 Brady Eidson 2013-03-21 11:57:42 PDT 
Created attachment 194310 [details]
Patch v1
Comment 2 WebKit Review Bot 2013-03-21 12:00:10 PDT 
Attachment 194310 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/loader/ResourceBuffer.cpp', u'Source/WebCore/loader/ResourceBuffer.h', u'Source/WebCore/loader/cache/CachedResource.cpp', u'Source/WebCore/loader/cache/CachedResource.h', u'Source/WebCore/platform/SharedBuffer.h', u'Source/WebCore/platform/cf/SharedBufferCF.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.h', u'Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm', u'Source/WebKit2/Platform/SharedMemory.h', u'Source/WebKit2/Platform/mac/SharedMemoryMac.cpp', u'Source/WebKit2/Shared/ShareableResource.cpp', u'Source/WebKit2/Shared/ShareableResource.h', u'Source/WebKit2/Shared/WebCoreArgumentCoders.cpp', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.cpp', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.messages.in', u'Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp']" exit_code: 1
Source/WebKit2/Platform/mac/SharedMemoryMac.cpp:113:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:111:  The parameter name "handle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/Shared/ShareableResource.cpp:68:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Shared/ShareableResource.cpp:69:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Shared/ShareableResource.cpp:70:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Shared/ShareableResource.cpp:71:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Shared/ShareableResource.cpp:72:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Shared/ShareableResource.cpp:74:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:262:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 9 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Brady Eidson 2013-03-21 12:04:43 PDT 
(In reply to comment #2)
> Attachment 194310 [details] did not pass style-queue:

> Source/WebKit2/Platform/mac/SharedMemoryMac.cpp:113:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:111:  The parameter name "handle" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Source/WebKit2/Shared/ShareableResource.cpp:68:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebKit2/Shared/ShareableResource.cpp:69:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebKit2/Shared/ShareableResource.cpp:70:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebKit2/Shared/ShareableResource.cpp:71:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebKit2/Shared/ShareableResource.cpp:72:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebKit2/Shared/ShareableResource.cpp:74:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:262:  One space before end of line comments  [whitespace/comments] [5]
> Total errors found: 9 in 21 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Fixed the 3 that should've been fixed, leaving standard CF-style in place for the 6 others.
Comment 4 Early Warning System Bot 2013-03-21 12:09:33 PDT 
Comment on attachment 194310 [details]
Patch v1

Attachment 194310 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17250145
Comment 5 Early Warning System Bot 2013-03-21 12:14:25 PDT 
Comment on attachment 194310 [details]
Patch v1

Attachment 194310 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17290097
Comment 6 Build Bot 2013-03-21 12:31:02 PDT 
Comment on attachment 194310 [details]
Patch v1

Attachment 194310 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17161975
Comment 7 Brady Eidson 2013-03-21 12:37:53 PDT 
Created attachment 194319 [details]
Patch v2 - Style and build fixes
Comment 8 WebKit Review Bot 2013-03-21 12:41:07 PDT 
Attachment 194319 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/loader/ResourceBuffer.cpp', u'Source/WebCore/loader/ResourceBuffer.h', u'Source/WebCore/loader/cache/CachedResource.cpp', u'Source/WebCore/loader/cache/CachedResource.h', u'Source/WebCore/platform/SharedBuffer.h', u'Source/WebCore/platform/cf/SharedBufferCF.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.h', u'Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm', u'Source/WebKit2/Platform/SharedMemory.h', u'Source/WebKit2/Platform/mac/SharedMemoryMac.cpp', u'Source/WebKit2/Shared/ShareableResource.cpp', u'Source/WebKit2/Shared/ShareableResource.h', u'Source/WebKit2/Shared/WebCoreArgumentCoders.cpp', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.cpp', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.messages.in', u'Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp']" exit_code: 1
Source/WebKit2/Shared/ShareableResource.cpp:68:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Shared/ShareableResource.cpp:69:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Shared/ShareableResource.cpp:70:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Shared/ShareableResource.cpp:71:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Shared/ShareableResource.cpp:72:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Shared/ShareableResource.cpp:74:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 6 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Geoffrey Garen 2013-03-21 12:56:04 PDT 
View in context: https://bugs.webkit.org/attachment.cgi?id=194310&action=review

> Source/WebCore/loader/cache/CachedResource.cpp:979
> +    // Verify the buffers are the same as if the encoded data is different than what we're replacing,
> +    // we don't want to replace it.

I think it's almost self-evident that we wouldn't want to replace our content with something that didn't match -- what's not self-evident is how that could ever happen. I think that's a better thing to comment about. Something like:

This function supports replacing an item with its value in the disk cache. Since the disk cache is race-y, we can't assume that 'newBuffer' actually matches m_data.

> Source/WebCore/loader/cache/CachedResource.cpp:981
> +    if (m_data->size() != newBuffer->size() || !memcmp(m_data->data(), newBuffer->data(), m_data->size()))
> +        return;

This is wrong. memcmp returns 0 when the two buffer are equal.

> Source/WebCore/loader/cache/CachedResource.h:268
> +    void tryReplacingEncodedData(PassRefPtr<SharedBuffer>);

Your other function is named "tryWrap", in the imperative form, so I think we should do the same here: "tryReplace".

> Source/WebCore/platform/cf/SharedBufferCF.cpp:103
> +    else
> +        append(newContents);

I don't think this else case makes sense. We don't have a use case where we would want to append 'newContents', since the optimization will fail if we do. I think it would be better to rename this function to 'tryReplaceContents', and move the buffer comparison into this function as well. Then, we can just return early if 'newContents' lacks an optimized form.

>> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:262
>> +        ref();  // Balanced by a deref() in diskCacheTimerFired().
> 
> One space before end of line comments  [whitespace/comments] [5]

Style bot is right, for once.

Nit: It's adoptRef(), not deref().

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:44
> +static bool CFURLCacheIsMemMappedData(CFURLCacheRef cache, CFDataRef data)

Can we use the SOFT_LINK_OPTIONAL macro here instead of rolling our own?

> Source/WebKit2/Platform/mac/SharedMemoryMac.cpp:110
> +    sharedMemory->m_shouldVMDeallocateData = true;

What is the case where m_shouldVMDeallocateData is false? As far as I can tell, it always ends up true.

> Source/WebKit2/Shared/ShareableResource.cpp:61
> +    resource->deref();

For your timer, you chose adoptRef() instead of an explicit deref(). Let's pick one style and use it everywhere.

> Source/WebKit2/Shared/ShareableResource.cpp:67
> +        resource.leakRef(),

For your timer, you chose ref() instead of leakRef(). Let's pick one style and use it everywhere.
Comment 10 Brady Eidson 2013-03-21 13:47:28 PDT 
(In reply to comment #9)
> View in context: https://bugs.webkit.org/attachment.cgi?id=194310&action=review
> 
> > Source/WebCore/loader/cache/CachedResource.cpp:979
> > +    // Verify the buffers are the same as if the encoded data is different than what we're replacing,
> > +    // we don't want to replace it.
> 
> I think it's almost self-evident that we wouldn't want to replace our content with something that didn't match -- what's not self-evident is how that could ever happen. I think that's a better thing to comment about. Something like:
> 
> This function supports replacing an item with its value in the disk cache. Since the disk cache is race-y, we can't assume that 'newBuffer' actually matches m_data.

Good suggestion.

> > Source/WebCore/loader/cache/CachedResource.cpp:981
> > +    if (m_data->size() != newBuffer->size() || !memcmp(m_data->data(), newBuffer->data(), m_data->size()))
> > +        return;
> 
> This is wrong. memcmp returns 0 when the two buffer are equal.

Yikes

> > Source/WebCore/loader/cache/CachedResource.h:268
> > +    void tryReplacingEncodedData(PassRefPtr<SharedBuffer>);
> 
> Your other function is named "tryWrap", in the imperative form, so I think we should do the same here: "tryReplace".

Done

> > Source/WebCore/platform/cf/SharedBufferCF.cpp:103
> > +    else
> > +        append(newContents);
> 
> I don't think this else case makes sense. We don't have a use case where we would want to append 'newContents', since the optimization will fail if we do. I think it would be better to rename this function to 'tryReplaceContents', and move the buffer comparison into this function as well. Then, we can just return early if 'newContents' lacks an optimized form.

Since SharedBuffer is completely divorced from the mechanism we're adding, I was attempting to keep it general purpose.

I sympathize to your suggestion but think it is flawed itself - "we can just return early if 'newContents' lacks an optimized form" is impossible to implement as we know there's a cfData but we don't know it's optimized.

Maybe "tryReplaceContentsWithPlatformBuffer" is both a sufficiently general and sufficiently specific name to make us both happy?

> >> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:262
> >> +        ref();  // Balanced by a deref() in diskCacheTimerFired().
> Nit: It's adoptRef(), not deref().

Fixed

> > Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:44
> > +static bool CFURLCacheIsMemMappedData(CFURLCacheRef cache, CFDataRef data)
> 
> Can we use the SOFT_LINK_OPTIONAL macro here instead of rolling our own?

That was the first thing I tried, at which point I discovered:
A - It doesn't do what you think it does
and
B - It doesn't work at all on Mac, despite the comment above it claiming it might be used on Mac.

> > Source/WebKit2/Platform/mac/SharedMemoryMac.cpp:110
> > +    sharedMemory->m_shouldVMDeallocateData = true;
> 
> What is the case where m_shouldVMDeallocateData is false? As far as I can tell, it always ends up true.

I think you've misread - When an MMAPed buffer is passed in to ::createFromVMBuffer() directly, SharedMemory didn't allocate it with vm_allocate and therefore should've deallocate it with vm_deallocate.

> > Source/WebKit2/Shared/ShareableResource.cpp:61
> > +    resource->deref();
> 
> For your timer, you chose adoptRef() instead of an explicit deref(). Let's pick one style and use it everywhere.
> 
> > Source/WebKit2/Shared/ShareableResource.cpp:67
> > +        resource.leakRef(),
> 
> For your timer, you chose ref() instead of leakRef(). Let's pick one style and use it everywhere.

Normally we do this type of thing with ref()/deref().

It's easy to make ShareableResource use ref()/deref() and I've made that chance locally.

In the case of the timer, there are multiple code paths to return from the function, so relying on RefPtr<> destruction to do the cleanup is both less code and more foolproof.
Comment 11 Brady Eidson 2013-03-21 14:21:29 PDT 
Created attachment 194337 [details]
Patch v3
Comment 12 Geoffrey Garen 2013-03-21 14:23:56 PDT 
> Maybe "tryReplaceContentsWithPlatformBuffer" is both a sufficiently general and sufficiently specific name to make us both happy?

Sounds good. (My main goal is just removing the call to append(). Putting "try" in the name is just a way to give us the freedom to return early without calling append().)


> > What is the case where m_shouldVMDeallocateData is false? As far as I can tell, it always ends up true.

> I think you've misread - When an MMAPed buffer is passed in to ::createFromVMBuffer() directly,
> SharedMemory didn't allocate it with vm_allocate and therefore should've deallocate it with
>  vm_deallocate.

Very possible! :)

The call to createFromVMBuffer that I see is:

RefPtr<SharedMemory> sharedMemory = createFromVMBuffer(toPointer(address), size);

Five lines later I see:

sharedMemory->m_shouldVMDeallocateData = true;

I don't see any other calls to createFromVMBuffer(). That's what made me think that the flag is always logically true.

> Normally we do this type of thing with ref()/deref().

OK.
Comment 13 Brady Eidson 2013-03-21 14:25:19 PDT 
(In reply to comment #12)

> > > What is the case where m_shouldVMDeallocateData is false? As far as I can tell, it always ends up true.
> 
> > I think you've misread - When an MMAPed buffer is passed in to ::createFromVMBuffer() directly,
> > SharedMemory didn't allocate it with vm_allocate and therefore should've deallocate it with
> >  vm_deallocate.
> 
> Very possible! :)
> 
> The call to createFromVMBuffer that I see is:
> 
> RefPtr<SharedMemory> sharedMemory = createFromVMBuffer(toPointer(address), size);
> 
> Five lines later I see:
> 
> sharedMemory->m_shouldVMDeallocateData = true;
> 
> I don't see any other calls to createFromVMBuffer(). That's what made me think that the flag is always logically true.

That caller is NetworkResourceLoader::tryGetShareableHandleForResource
Comment 14 Geoffrey Garen 2013-03-21 14:50:32 PDT 
Comment on attachment 194337 [details]
Patch v3

r=me
Comment 15 Brady Eidson 2013-03-21 15:44:20 PDT 
Created attachment 194367 [details]
Updated patch for EWS run
Comment 16 WebKit Review Bot 2013-03-21 15:48:39 PDT 
Attachment 194367 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/loader/ResourceBuffer.h', u'Source/WebCore/loader/cache/CachedResource.cpp', u'Source/WebCore/loader/cache/CachedResource.h', u'Source/WebCore/loader/mac/ResourceBuffer.mm', u'Source/WebCore/platform/SharedBuffer.h', u'Source/WebCore/platform/cf/SharedBufferCF.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.h', u'Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm', u'Source/WebKit2/Platform/SharedMemory.h', u'Source/WebKit2/Platform/mac/SharedMemoryMac.cpp', u'Source/WebKit2/Shared/ShareableResource.cpp', u'Source/WebKit2/Shared/ShareableResource.h', u'Source/WebKit2/Shared/WebCoreArgumentCoders.cpp', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.cpp', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h', u'Source/WebKit2/WebProcess/Network/NetworkProcessConnection.messages.in', u'Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp']" exit_code: 1
Source/WebKit2/Shared/ShareableResource.cpp:69:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Shared/ShareableResource.cpp:70:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Shared/ShareableResource.cpp:71:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Shared/ShareableResource.cpp:72:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Shared/ShareableResource.cpp:73:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Shared/ShareableResource.cpp:75:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 6 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Brady Eidson 2013-03-21 17:08:16 PDT 
EWS is running *EXTREMELY* slowly this afternoon.  Enough have passed that I'm pulling the trigger on landing.
Comment 18 Brady Eidson 2013-03-21 17:16:40 PDT 
http://trac.webkit.org/changeset/146544