Bug 144380 – [SOUP] Network Cache: Implement ShareableResource for Soup and enable it for GTK platform

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

It improves the network cache performance, by mmaping big resources and sending only the file descriptor to the web process instead of the actual file data.

[Carlos Garcia Campos]

Created attachment 251937 [details]
Patch

NetworkCache is enabled only for EWS

[Antti Koivisto]

Comment on attachment 251937 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:130
> -    auto mappedData = Data::adoptMap(map, size);
> +    auto mappedData = Data::adoptMap(map, size, fd);

It would be good to encapsulate the whole "create disk-backed map from existing Data" operation and make it a Data function.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:42
>  namespace WebKit {
> +class SharedMemory;

empty line after namespace

> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataCocoa.mm:112
>  Data mapFile(int fd, size_t offset, size_t size)
>  {
> -    if (!size)
> +    if (!size) {
> +        close(fd);
>          return Data::empty();
> +    }

It is surprising this function now closes the file. At minimum it should be renamed to something like adoptAndMapFile. There are a bunch of clients that call this and close the file manually.

We really need better abstactions though.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheDataCocoa.mm:157
> +RefPtr<SharedMemory> Data::createSharedMemory() const

tryCreateSharedMemory?

> Source/WebKit2/Shared/soup/ShareableResourceSoup.cpp:43
> +PassRefPtr<SharedBuffer> ShareableResource::Handle::tryWrapInSharedBuffer() const

Adding this Soup-specific stuff is going to make it harder to get rid of these ugly abstractions. :(

[Carlos Garcia Campos]

(In reply to comment #2)
> Comment on attachment 251937 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251937&action=review
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:130
> > -    auto mappedData = Data::adoptMap(map, size);
> > +    auto mappedData = Data::adoptMap(map, size, fd);
> 
> It would be good to encapsulate the whole "create disk-backed map from
> existing Data" operation and make it a Data function.

Ok.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:42
> >  namespace WebKit {
> > +class SharedMemory;
> 
> empty line after namespace

Sure.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheDataCocoa.mm:112
> >  Data mapFile(int fd, size_t offset, size_t size)
> >  {
> > -    if (!size)
> > +    if (!size) {
> > +        close(fd);
> >          return Data::empty();
> > +    }
> 
> It is surprising this function now closes the file. At minimum it should be
> renamed to something like adoptAndMapFile. There are a bunch of clients that
> call this and close the file manually.

I agree it's not that obvious that mapFile takes ownership of the fd. I think, I updated all callers to not close the fd, or what clients do you mean?

> We really need better abstactions though.
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheDataCocoa.mm:157
> > +RefPtr<SharedMemory> Data::createSharedMemory() const
> 
> tryCreateSharedMemory?

It makes sense, yes.

> > Source/WebKit2/Shared/soup/ShareableResourceSoup.cpp:43
> > +PassRefPtr<SharedBuffer> ShareableResource::Handle::tryWrapInSharedBuffer() const
> 
> Adding this Soup-specific stuff is going to make it harder to get rid of
> these ugly abstractions. :(

Yes, this is because current tryWrapInSharedBuffer() method is CF specific, but implemented in the main cpp file. I just provided a soup implementation, but using its own platform file, I could move it to the cpp file though.

[Carlos Garcia Campos]

Created attachment 252375 [details]
Updated patch

Updated patch. I've moved the common code to a new cpp file (I need someone with a mac who add the xcode bits, please), merging the mapfile methods, and adding mapTofile, leaving adoptMap as the only method that requires platform specific implementation. Not asking for review to not kick the EWS bots, since network cache is still disabled in GTK+ port and it won't build in mac because of the new file.

[Antti Koivisto]

> I agree it's not that obvious that mapFile takes ownership of the fd. I
> think, I updated all callers to not close the fd, or what clients do you
> mean?

Some of the NetworkCache types have been used outside the cache code, check WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp.

[Carlos Garcia Campos]

(In reply to comment #5)
> > I agree it's not that obvious that mapFile takes ownership of the fd. I
> > think, I updated all callers to not close the fd, or what clients do you
> > mean?
> 
> Some of the NetworkCache types have been used outside the cache code, check
> WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp.

Ah!, I only grepped the cache dir indeed :-P

[Carlos Garcia Campos]

My patch indeed breaks APIUserContentExtensionStore.cpp, since I've merged the two mapFile methods. So, it seems APIUserContentExtensionStore uses mapfile in two places:

 - openAndMapContentExtension: This could use the mapFile method that receives the path, and the code would be indeed simpler.

 - compiledToFile: this creates a temp file that is written, mapped and moved to the final destination. In this case, maybe we could add a parameter to mapToFile that receives a flag to indicate whether we want to write to the file atomically (write to temp and then move), and move all the temp file logic there. This would also simplify the UserContentExtensionStore code a lot.

What do you think?

[Carlos Garcia Campos]

I've split the patch in the end, moving the Data refactoring to a new bug, see #144950

[Carlos Garcia Campos]

Created attachment 253309 [details]
New patch

[WebKit Commit Bot]

Attachment 253309 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/ShareableResource.cpp:92:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 253310 [details]
Try to fix IOS build

[WebKit Commit Bot]

Attachment 253310 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/ShareableResource.cpp:92:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Comment on attachment 253310 [details]
Try to fix IOS build

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

> Source/WebKit2/Platform/SharedMemory.h:88
> +    static RefPtr<SharedMemory> wrapMap(void*, size_t, int fileDescriptor);

This should be #if'd for the platforms it is implemented only.

> Source/WebKit2/Shared/ShareableResource.cpp:85
> +PassRefPtr<SharedBuffer> ShareableResource::sharedBuffer()
> +{
> +    ref(); // Balanced by deref when SharedBuffer is deallocated.

This is named like an accessor but does some non-accessor like ref tricks. Maybe wrapInSharedBuffer()?

[Carlos Garcia Campos]

(In reply to comment #13)
> Comment on attachment 253310 [details]
> Try to fix IOS build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253310&action=review

Thanks!
 
> > Source/WebKit2/Platform/SharedMemory.h:88
> > +    static RefPtr<SharedMemory> wrapMap(void*, size_t, int fileDescriptor);
> 
> This should be #if'd for the platforms it is implemented only.

hmm, right, I guess we should do the same for the create() that is mac only.

> > Source/WebKit2/Shared/ShareableResource.cpp:85
> > +PassRefPtr<SharedBuffer> ShareableResource::sharedBuffer()
> > +{
> > +    ref(); // Balanced by deref when SharedBuffer is deallocated.
> 
> This is named like an accessor but does some non-accessor like ref tricks.
> Maybe wrapInSharedBuffer()?

I thought about createSharedBuffer but didn't like it either, wrapInSharedBuffer() sounds good to me.

[Carlos Garcia Campos]

Committed r184620: <http://trac.webkit.org/changeset/184620>