Bug 112387 - NetworkProcess should send vm_copied, mmap'ed memory to WebProcesses when a resource is already in the disk cache
Summary: NetworkProcess should send vm_copied, mmap'ed memory to WebProcesses when a r...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-03-14 16:06 PDT by Brady Eidson
Modified: 2013-03-18 21:18 PDT (History)
8 users (show)

See Also:


Attachments
Patch v1 (24.46 KB, patch)
2013-03-14 16:13 PDT, Brady Eidson
ggaren: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (EWS check) (23.93 KB, patch)
2013-03-18 09:44 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2013-03-14 16:06:31 PDT
NetworkProcess should send vm_copied, mmap'ed memory to WebProcesses when a resource is already in the disk cache

In radar as <rdar://problem/13414153>
Comment 1 Brady Eidson 2013-03-14 16:13:49 PDT
Created attachment 193197 [details]
Patch v1
Comment 2 WebKit Review Bot 2013-03-14 16:16:59 PDT
Attachment 193197 [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/platform/network/cf/ResourceRequest.h', u'Source/WebCore/platform/network/mac/ResourceRequestMac.mm', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.h', u'Source/WebKit2/NetworkProcess/mac/NetworkProcessMac.mm', 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.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp', u'Source/WebKit2/WebProcess/mac/WebProcessMac.mm']" exit_code: 1
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:119:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:120:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:121:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:122:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:123:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:124:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:126:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Platform/SharedMemory.h:87:  The parameter name "size_t" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 14 files


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

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:119:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]

Fixed this

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:120:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:121:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:122:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:123:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:124:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:126:  Use 0 instead of NULL.  [readability/null] [5]

That's preferred CF-style.  Without strong objections I'll leave it.

> Source/WebKit2/Platform/SharedMemory.h:87:  The parameter name "size_t" adds no information, so it should be removed.  [readability/parameter_name] [5]

Boneheaded mistake.  Will fix.
Comment 4 Alexey Proskuryakov 2013-03-14 17:38:49 PDT
Comment on attachment 193197 [details]
Patch v1

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

I only had a brief look at the patch. Would be helpful to talk it thorough in person (mostly for my education).

> Source/WebKit2/NetworkProcess/mac/NetworkProcessMac.mm:45
> +#import <CFNetwork/CFURLCache.h>

Nope.

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:32
> +#include <CFNetwork/CFURLCache.h>

Nope.

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:50
> +    // We have to ask the WebCore::ResourceRequest for its CFURLRequest on the main thread because
> +    // of AtomicString thread safety issues.
> +    __block CFURLRequestRef cfRequest = 0;
> +    dispatch_sync(dispatch_get_main_queue(), ^{
> +        cfRequest = request().cfURLRequest(DoNotUpdateHTTPBody);
> +    });

I don' think that this is needed. CFURLRequest is going to be in ResourceRequest already, so this is just a plain getter.
Comment 5 Geoffrey Garen 2013-03-14 17:58:51 PDT
Comment on attachment 193197 [details]
Patch v1

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

r=me

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:87
> +    static size_t fileBackedResourceThreshold();

Might be nice to call this "minimum" or "minimumSize" instead of "threshold".

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:65
> +    // We only care about the vm_copy optimization for resources that should be file backed.

This should really be a FIXME about using an API instead of a guess in future.

> Source/WebKit2/WebProcess/mac/WebProcessMac.mm:-119
> -        [nsurlCache setDiskCapacity:0];

This is no longer necessary / correct. The WebProcess no longer shares the disk cache with the network process, so it is appropriate for the WebProcess to have a zero-sized disk cache, to save SQL memory.
Comment 6 Build Bot 2013-03-14 18:29:57 PDT
Comment on attachment 193197 [details]
Patch v1

Attachment 193197 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17112224
Comment 7 Geoffrey Garen 2013-03-14 19:03:56 PDT
/Volumes/Data/EWS/WebKit/Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:32:10: fatal error: 'CFNetwork/CFURLCache.h' file not found
#include <CFNetwork/CFURLCache.h>
         ^

Looks like ap is the smart one in this crowd.
Comment 8 Build Bot 2013-03-14 19:19:34 PDT
Comment on attachment 193197 [details]
Patch v1

Attachment 193197 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17132166
Comment 9 Build Bot 2013-03-14 19:40:18 PDT
Comment on attachment 193197 [details]
Patch v1

Attachment 193197 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17126469
Comment 10 Brady Eidson 2013-03-14 20:11:44 PDT
(In reply to comment #4)
> (From update of attachment 193197 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193197&action=review
> 
> > Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:50
> > +    // We have to ask the WebCore::ResourceRequest for its CFURLRequest on the main thread because
> > +    // of AtomicString thread safety issues.
> > +    __block CFURLRequestRef cfRequest = 0;
> > +    dispatch_sync(dispatch_get_main_queue(), ^{
> > +        cfRequest = request().cfURLRequest(DoNotUpdateHTTPBody);
> > +    });
> 
> I don' think that this is needed. CFURLRequest is going to be in ResourceRequest already, so this is just a plain getter.

Unfortunately I found in testing that getting the cfURLRequest() creates atomic strings on the background loader thread, even though the ResourceRequest is already wrapping an NSURLRequest.

I could add a different mode to ResourceRequest to just fetch the CFURLRequest that exists without "updating platform object" to bypass this, if you think it'd be better.  That would open up the possibility that other people start using that accessor when what *THEY* really want is the traditional accessor that does the platform updating.
Comment 11 Brady Eidson 2013-03-14 20:12:50 PDT
(In reply to comment #7)
> /Volumes/Data/EWS/WebKit/Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:32:10: fatal error: 'CFNetwork/CFURLCache.h' file not found
> #include <CFNetwork/CFURLCache.h>
>          ^
> 
> Looks like ap is the smart one in this crowd.

YUP.

I probably won't get to fixing that up tonight, but will do over the weekend.

I like your naming suggestion, too.

Thanks for the review!
Comment 12 Alexey Proskuryakov 2013-03-14 22:51:02 PDT
> Unfortunately I found in testing that getting the cfURLRequest() creates atomic strings on the background loader thread, even though the ResourceRequest is already wrapping an NSURLRequest.

I'd like to understand the mechanics of this.

It is not necessarily wrong to create AtomicStrings on a non-main thread. It's using them on multiple threads that it wrong.
Comment 13 Brady Eidson 2013-03-15 04:42:56 PDT
(In reply to comment #12)
> > Unfortunately I found in testing that getting the cfURLRequest() creates atomic strings on the background loader thread, even though the ResourceRequest is already wrapping an NSURLRequest.
> 
> I'd like to understand the mechanics of this.
> 
> It is not necessarily wrong to create AtomicStrings on a non-main thread. It's using them on multiple threads that it wrong.

Pretty straight forward:
0 - Be on a background loading thread
1 - Call ResourceHandle::cfURLRequest()
2 - updatePlatformRequest() creates some static atomic strings
3 - Later the load finishes and the thread is destroyed.
4 - WTFThreadData is destroyed which...
5 - Destroys the AtomicStringTable which...
6 - setIsAtomic(false) on each atomic string...
7 - ... Even the static ones....
8 -  ASSERT(!isStatic());

Thread 3 Crashed:
0   com.apple.JavaScriptCore      	0x00000001091938a1 WTF::StringImpl::setIsAtomic(bool) + 97 (StringImpl.h:523)
1   com.apple.JavaScriptCore      	0x0000000109197de9 WTF::AtomicStringTable::destroy(WTF::AtomicStringTable*) + 105 (AtomicString.cpp:103)
2   com.apple.JavaScriptCore      	0x00000001091ea470 WTF::WTFThreadData::~WTFThreadData() + 48 (WTFThreadData.cpp:54)
3   com.apple.JavaScriptCore      	0x00000001091ea435 WTF::WTFThreadData::~WTFThreadData() + 21 (WTFThreadData.cpp:56)
4   com.apple.JavaScriptCore      	0x0000000108cadb87 WTF::ThreadSpecific<WTF::WTFThreadData>::destroy(void*) + 55 (ThreadSpecific.h:231)

Now, we could definitely also solve this by adding a new getExistingCFURLRequestOrNull() accessor instead of doing the thread hopping.  After half a night's sleep, I think with a scary enough name like that people wouldn't ever feel inclined to use it.
Comment 14 Alexey Proskuryakov 2013-03-15 08:42:38 PDT
> 1 - Call ResourceHandle::cfURLRequest()
> 2 - updatePlatformRequest() creates some static atomic strings

Why does it do this? We are handling a response, so request's platform data should have already been created. It should not be needing an update.

I do not think that adding thread hopping per function calls is the right way forward. We either need to make the work ResourceHandle does thread safe, or at least to minimize thread hopping by doing it once, immediately on receiving a delegate call.
Comment 15 Brady Eidson 2013-03-18 09:23:25 PDT
(In reply to comment #14)
> > 1 - Call ResourceHandle::cfURLRequest()
> > 2 - updatePlatformRequest() creates some static atomic strings
> 
> Why does it do this? We are handling a response, so request's platform data should have already been created. It should not be needing an update.
> 
> I do not think that adding thread hopping per function calls is the right way forward. We either need to make the work ResourceHandle does thread safe, or at least to minimize thread hopping by doing it once, immediately on receiving a delegate call.

Needing this was a cross between something boneheaded I was doing before asking for the cfurlresponse.  It's being removed before landing.
Comment 16 Brady Eidson 2013-03-18 09:44:55 PDT
Created attachment 193583 [details]
Patch for landing (EWS check)
Comment 17 WebKit Review Bot 2013-03-18 09:47:22 PDT
Attachment 193583 [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/platform/network/cf/ResourceRequest.h', u'Source/WebCore/platform/network/mac/ResourceRequestMac.mm', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.h', u'Source/WebKit2/NetworkProcess/mac/NetworkProcessMac.mm', 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.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp']" exit_code: 1
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:120:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:121:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:122:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:123:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:124:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:126:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 6 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Brady Eidson 2013-03-18 11:07:45 PDT
http://trac.webkit.org/changeset/146088
Comment 19 David Kilzer (:ddkilzer) 2013-03-18 21:18:24 PDT
(In reply to comment #18)
> http://trac.webkit.org/changeset/146088

iOS build fix in r146177:  http://trac.webkit.org/r146177