| Summary: | NetworkProcess should send vm_copied, mmap'ed memory to WebProcesses when a resource is already in the disk cache | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||
| Component: | WebKit2 | Assignee: | Brady Eidson <beidson> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | andersca, ap, buildbot, ddkilzer, ggaren, rniwa, sam, webkit.review.bot | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
Brady Eidson
2013-03-14 16:06:31 PDT
Created attachment 193197 [details]
Patch v1
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.
(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 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 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 on attachment 193197 [details] Patch v1 Attachment 193197 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17112224 /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 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 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 (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. (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! > 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.
(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. > 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.
(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. Created attachment 193583 [details]
Patch for landing (EWS check)
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.
(In reply to comment #18) > http://trac.webkit.org/changeset/146088 iOS build fix in r146177: http://trac.webkit.org/r146177 |