| Summary: | Cached file backed resources don't make it to the Web Process when NETWORK_CFDATA_ARRAY_CALLBACK is enabled | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Pratik Solanki <psolanki> | ||||||
| Component: | WebKit2 | Assignee: | Pratik Solanki <psolanki> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ap, beidson, kling, psolanki | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 135554 | ||||||||
| Attachments: |
|
||||||||
Created attachment 236251 [details]
Patch
Comment on attachment 236251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236251&action=review > Source/WebCore/ChangeLog:16 > + (WebCore::SharedBuffer::createCFData): Is there a better name I could use for getCFDataIfAvailable? I don't like the mismatch of get and create but createCFDataIfAvailable() seemed odd. Comment on attachment 236251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236251&action=review >> Source/WebCore/ChangeLog:16 >> + (WebCore::SharedBuffer::createCFData): > > Is there a better name I could use for getCFDataIfAvailable? I don't like the mismatch of get and create but createCFDataIfAvailable() seemed odd. createCFDataIfInternallyRepresentedAsCFData()? createCFDataNoCopy()? Comment on attachment 236251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236251&action=review >>> Source/WebCore/ChangeLog:16 >>> + (WebCore::SharedBuffer::createCFData): >> >> Is there a better name I could use for getCFDataIfAvailable? I don't like the mismatch of get and create but createCFDataIfAvailable() seemed odd. > > createCFDataIfInternallyRepresentedAsCFData()? createCFDataNoCopy()? copyCFDataNoCreate() ? Created attachment 236390 [details]
Patch2
I like copyCFDataNoCreate. Patch with new name.
Comment on attachment 236390 [details] Patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=236390&action=review > Source/WebCore/ChangeLog:10 > + Add SharedBuffer::copyCFDataNoCreate() which returns CFDataRef if it has one. Refactor > + this code out of createCFData(). I don’t like the grammar of the phrase “copy CFData no create”. I’d prefer copyExistingCFData or copyCFDataWithoutCreating. > Source/WebCore/platform/mac/SharedBufferMac.mm:148 > + RetainPtr<CFDataRef> cfData = copyCFDataNoCreate(); > + if (cfData) > + return cfData; Would be nice to put the definition inside the if statement. Comment on attachment 236390 [details] Patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=236390&action=review > Source/WebKit2/ChangeLog:13 > + so that SharedBuffer can take car of the data array case. typo: "take car" Comment on attachment 236390 [details] Patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=236390&action=review >> Source/WebCore/ChangeLog:10 >> + this code out of createCFData(). > > I don’t like the grammar of the phrase “copy CFData no create”. I’d prefer copyExistingCFData or copyCFDataWithoutCreating. Hmm, why copy? It should just be existingCFData and should not return a RetainPtr. (In reply to comment #9) > (From update of attachment 236390 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236390&action=review > > >> Source/WebCore/ChangeLog:10 > >> + this code out of createCFData(). > > > > I don’t like the grammar of the phrase “copy CFData no create”. I’d prefer copyExistingCFData or copyCFDataWithoutCreating. > > Hmm, why copy? It should just be existingCFData and should not return a RetainPtr. I like this! I'll change it to existingCFData and return a raw pointer. Committed r172502: <http://trac.webkit.org/changeset/172502> |
If we have a resource that is file backed in CFNetwork, then we try to send that data over to the web process so that the web process uses file backed memory. Unfortunately, this is not working on iOS because iOS uses the data array calbacks. We have this check in NetworkResourceLoader::tryGetShareableHandleFromSharedBuffer(). if (!buffer->hasPlatformData()) return; For data array callbacks, hasPlatformData() will return false since SharedBuffer has a data array and not a single CFDataRef. This breaks the optimization. If I comment out the above code and fall through, the code does RetainPtr<CFDataRef> data = buffer->createCFData(); if (_CFURLCacheIsResponseDataMemMapped(cache, data.get()) == kCFBooleanFalse) return; createCFData() will do the right thing and the CFNetwork SPI will return true which will make our optimization work again. The second mechanism - the disk cache notification works fine.