Bug 113422 – Mem mapped resource data improvements

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

Mem mapped resource data improvements.

These include:
-Using a notification API for a resource becoming file backed instead of using timers
-Using a much more explicit API to determine if the data for an object is mem mapped.

In radar as <rdar://problem/13196481>

[Brady Eidson]

Created attachment 195353 [details]
Patch v1

[Mark Rowe (bdash)]

Comment on attachment 195353 [details]
Patch v1

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

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:42
> +    static void (*softResponseSetBecameFileBackedCallBackBlock)(CFCachedURLResponseRef, CFCachedURLResponseCallBackBlock, dispatch_queue_t) = (void (*)(CFCachedURLResponseRef, CFCachedURLResponseCallBackBlock, dispatch_queue_t)) dlsym(CFNetworkLibrary(), "_CFCachedURLResponseSetBecameFileBackedCallBackBlock");

A typedef would make this a heck of a lot more readable.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp', u'Source/WebKit2/NetworkProcess/NetworkResourceLoader.h', u'Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.h', u'Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm', u'Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj']" exit_code: 1
Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.h:68:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Total errors found: 1 in 6 files


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

[Brady Eidson]

(In reply to comment #3)
> Attachment 195353 [details] did not pass style-queue:
> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.h:68:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
> Total errors found: 1 in 6 files

Fixed locally.

[Build Bot]

Comment on attachment 195353 [details]
Patch v1

Attachment 195353 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17338203

[Brady Eidson]

Wow, a build failure due to the lack of newline.  Yikes.

[Alexey Proskuryakov]

Comment on attachment 195353 [details]
Patch v1

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

I think that I'd like to see a version of this patch with threading cleaned up a bit more. I'm not sure if I can fully understand it now.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:-211
> -    ASSERT(!isMainThread());

Should the condition be just reverted instead of removing the check?

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.h:30
> +#include "NetworkResourceLoader.h"

Seems unneeded (and you already have a forward declaration in place).

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:36
> +
> +
> +SOFT_LINK_FRAMEWORK(CFNetwork)

Seems like an abuse of the macro if you are only doing this to get a CFNetworkLibrary() function.

But then, you'll probably be deleting the dynamic loading code soon.

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:52
> +static const double DiskCachingMonitorTimeout = 10;

I don;t think that starting constants with upper case is WebKit style.

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:56
> +    static uint64_t currentID = 1;

Maybe ASSERT(isMainThread())?

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:64
> +    DEFINE_STATIC_LOCAL(DiskCachingMonitorMap, map, ());

Maybe ASSERT(!diskCachingMonitorMapMutex().tryLock())?

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:81
> +    OwnPtr<DiskCachingMonitor> request = adoptPtr(new DiskCachingMonitor(cachedResponse, loader));

We usually hide the constructor and expose a create function for this.

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:93
> +    // Step 1: Set up this request's data members on the main thread.
> +    dispatch_async(dispatch_get_main_queue(), ^{

If it's dispatch_async, then it's not step 1, it's step "some time".

Also, I don't understand whether this constructor is only called on main thread anyway. Calling generateUniqueDiskCachingMonitorID() seems to require that, and all client code is main thread now.

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:98
> +    // Step2: Set up the disk caching callback so this request can be destroyed upon successfull disk caching.

No space in "Step2".

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:83
> +    DEFINE_STATIC_LOCAL(RetainPtr<CFURLCacheRef>, cache, (AdoptCF, CFURLCacheCopySharedURLCache()));
> +    if (!cache)
> +        return;

Don't we change the cache when enabling private browsing? I'm also unsure how this will work with cache partitioning.

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:99
> +void NetworkResourceLoader::willCacheResponseAsync(WebCore::ResourceHandle*, NSCachedURLResponse* nsResponse)

Please no WebCore:: prefix. This also has a misplaced star.

[Brady Eidson]

Created attachment 195365 [details]
Patch v2 - Fix build failure by adding a newline (!)

[Geoffrey Garen]

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

r- because I think the dispatch_async initialization may be a bug. Please consider the other stuff, too.

> Source/WebKit2/ChangeLog:17
> +        It both listens for a callback on the cached response indicating it has been cached
> +        or sets a timer so we don't keep objects alive waiting forever.

Logic: "or" should be "and".

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.h:41
> +class DiskCachingMonitor : public CoreIPC::MessageSender<DiskCachingMonitor> {

We usually use nouns and not gerunds as class names. How about "DiskCacheMonitor", since this object monitors the disk cache? It's awkward to say "This cache monitors the disk caching".

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.h:64
> +void monitorCachedResponse(CFCachedURLResponseRef, NetworkResourceLoader*);

This function name is a bit vague because it doesn't say what we're monitoring for. How about "monitorFileBackingStoreCreation"?

>> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:42
>> +    static void (*softResponseSetBecameFileBackedCallBackBlock)(CFCachedURLResponseRef, CFCachedURLResponseCallBackBlock, dispatch_queue_t) = (void (*)(CFCachedURLResponseRef, CFCachedURLResponseCallBackBlock, dispatch_queue_t)) dlsym(CFNetworkLibrary(), "_CFCachedURLResponseSetBecameFileBackedCallBackBlock");
> 
> A typedef would make this a heck of a lot more readable.

+1

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:52
> +// The maximum number of seconds we'll try to wait for a resource to be disk cached before we forget the request.
> +static const double DiskCachingMonitorTimeout = 10;

This seems really low. Since the CFNetwork initial delay is 7s, this means that we'll ignore any resource that gets postponed. How about 60s?

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:96
> +    dispatch_async(dispatch_get_main_queue(), ^{
> +        m_connectionToWebProcess = loader->connectionToWebProcess();
> +        m_resourceRequest = loader->request();
> +    });

How do we know it's valid to set up our data members asynchronously? What happens if we get a callback before our data members are set up?

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:106
> +    // Step2: Set up the disk caching callback so this request can be destroyed upon successfull disk caching.
> +    uint64_t identifier = m_requestID;
> +    CFCachedURLResponseCallBackBlock block = ^(CFCachedURLResponseRef cachedResponse)
> +    {
> +        DiskCachingMonitor* rawRequest = 0;
> +        {
> +            MutexLocker locker(diskCachingMonitorMapMutex());
> +            rawRequest = diskCachingMonitorMap().take(identifier);
> +        }

Let's call the object "monitor" and not "request", since request sounds like you're talking about the resource request.

You can eliminate the need for all that map, UUID, and timer complexity like so:

__block DiskCachingMonitor* rawMonitor = this;

dispatch_after(dispatch_time(DISPATCH_TIME_NOW, NSEC_PER_SEC * DiskCachingMonitorTimeout), dispatch_get_main_queue(), ^( adoptPtr(rawMonitor); rawMonitor = 0; });

CFCachedURLResponseCallBackBlock block = ^(CFCachedURLResponseRef cachedResponse)
{
    if (!rawMonitor)
        return;
    ....
    OwnPtr<DiskCachingMonitor> monitor = adoptPtr(rawMonitor);
    rawMonitor = 0;
}

[Brady Eidson]

(In reply to comment #9)
> View in context: https://bugs.webkit.org/attachment.cgi?id=195353&action=review
> 
> r- because I think the dispatch_async initialization may be a bug. Please consider the other stuff, too.

I disagree it's a bug, you'll have to convince me.  (Further discussion below)

> > Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.h:41
> > +class DiskCachingMonitor : public CoreIPC::MessageSender<DiskCachingMonitor> {
> 
> We usually use nouns and not gerunds as class names. How about "DiskCacheMonitor", since this object monitors the disk cache? It's awkward to say "This cache monitors the disk caching".

Okay.

> >> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:42
> >> +    static void (*softResponseSetBecameFileBackedCallBackBlock)(CFCachedURLResponseRef, CFCachedURLResponseCallBackBlock, dispatch_queue_t) = (void (*)(CFCachedURLResponseRef, CFCachedURLResponseCallBackBlock, dispatch_queue_t)) dlsym(CFNetworkLibrary(), "_CFCachedURLResponseSetBecameFileBackedCallBackBlock");
> > 
> > A typedef would make this a heck of a lot more readable.
> 
> +1

Okay.

> > Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:52
> > +// The maximum number of seconds we'll try to wait for a resource to be disk cached before we forget the request.
> > +static const double DiskCachingMonitorTimeout = 10;
> 
> This seems really low. Since the CFNetwork initial delay is 7s, this means that we'll ignore any resource that gets postponed. How about 60s?
> 
> > Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:96
> > +    dispatch_async(dispatch_get_main_queue(), ^{
> > +        m_connectionToWebProcess = loader->connectionToWebProcess();
> > +        m_resourceRequest = loader->request();
> > +    });
> 
> How do we know it's valid to set up our data members asynchronously? What happens if we get a callback before our data members are set up?

Since the dispatch_async to setup the members goes out before we even setup the callback, and since a "dispatch queue" is by definition a queue, are we not guaranteed this async dispatch is handled before any possible callback comes in?  Perhaps you're familiar with some exception to the "queue" part if dispatch_queue that I'm not familiar with.

> 
> > Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:106
> > +    // Step2: Set up the disk caching callback so this request can be destroyed upon successfull disk caching.
> > +    uint64_t identifier = m_requestID;
> > +    CFCachedURLResponseCallBackBlock block = ^(CFCachedURLResponseRef cachedResponse)
> > +    {
> > +        DiskCachingMonitor* rawRequest = 0;
> > +        {
> > +            MutexLocker locker(diskCachingMonitorMapMutex());
> > +            rawRequest = diskCachingMonitorMap().take(identifier);
> > +        }
> 
> Let's call the object "monitor" and not "request", since request sounds like you're talking about the resource request.

Originally the class was called "DiskCachingRequest", and I missed this in my rename.  Will fix.

> You can eliminate the need for all that map, UUID, and timer complexity like so:
> 
> __block DiskCachingMonitor* rawMonitor = this;
> 
> dispatch_after(dispatch_time(DISPATCH_TIME_NOW, NSEC_PER_SEC * DiskCachingMonitorTimeout), dispatch_get_main_queue(), ^( adoptPtr(rawMonitor); rawMonitor = 0; });
> 
> CFCachedURLResponseCallBackBlock block = ^(CFCachedURLResponseRef cachedResponse)
> {
>     if (!rawMonitor)
>         return;
>     ....
>     OwnPtr<DiskCachingMonitor> monitor = adoptPtr(rawMonitor);
>     rawMonitor = 0;
> }

That is clever.  Now You're Thinking With Closures ™

[Build Bot]

Comment on attachment 195365 [details]
Patch v2 - Fix build failure by adding a newline (!)

Attachment 195365 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17314414

[Geoffrey Garen]

> > How do we know it's valid to set up our data members asynchronously? What happens if we get a callback before our data members are set up?

> Since the dispatch_async to setup the members goes out before we even setup the callback, and since a "dispatch queue" is by definition a queue, are we not guaranteed this async dispatch is handled before any possible callback comes in?  Perhaps you're familiar with some exception to the "queue" part if dispatch_queue that I'm not familiar with.

No, you're totally right. I missed the fact that all of these blocks are scheduled on the main queue.

Side note: This logic applies to the main queue because the main queue is a serial queue. It would not apply to a parallel queue, such as dispatch_get_global_queue(...).

[Brady Eidson]

(In reply to comment #7)
> (From update of attachment 195353 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195353&action=review
> 
> I think that I'd like to see a version of this patch with threading cleaned up a bit more. I'm not sure if I can fully understand it now.

Much simpler with Geoff's block improvement idea.
> 
> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:-211
> > -    ASSERT(!isMainThread());
> 
> Should the condition be just reverted instead of removing the check?

Fixed

> > Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.h:30
> > +#include "NetworkResourceLoader.h"
> 
> Seems unneeded (and you already have a forward declaration in place)

Yup.

> 
> > Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:36
> > +
> > +
> > +SOFT_LINK_FRAMEWORK(CFNetwork)
> 
> Seems like an abuse of the macro if you are only doing this to get a CFNetworkLibrary() function.
> 
> But then, you'll probably be deleting the dynamic loading code soon.

It is abuse, and yes it'll be deleting soonish.

> 
> > Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:52
> > +static const double DiskCachingMonitorTimeout = 10;
> 
> I don;t think that starting constants with upper case is WebKit style.

It's not!  Style bot fail!

> 
> > Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:56
> > +    static uint64_t currentID = 1;
> 
> Maybe ASSERT(isMainThread())?
> 
> > Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:64
> > +    DEFINE_STATIC_LOCAL(DiskCachingMonitorMap, map, ());
> 
> Maybe ASSERT(!diskCachingMonitorMapMutex().tryLock())?

These are both now gone.

> 
> > Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:81
> > +    OwnPtr<DiskCachingMonitor> request = adoptPtr(new DiskCachingMonitor(cachedResponse, loader));
> 
> We usually hide the constructor and expose a create function for this.

Because of the somewhat atypical role and use of the DiskCacheMonitor object, I'm not sure we have an applicable pattern to build on.

> > Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:93
> > +    // Step 1: Set up this request's data members on the main thread.
> > +    dispatch_async(dispatch_get_main_queue(), ^{
> 
> If it's dispatch_async, then it's not step 1, it's step "some time".

It's "some time," but in a series of asynchronous queued steps, it's still step 1.  The comment pattern will hold more true in the new version of the patch.

> > Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:98
> > +    // Step2: Set up the disk caching callback so this request can be destroyed upon successfull disk caching.
> 
> No space in "Step2".

Fixed.

> 
> > Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:83
> > +    DEFINE_STATIC_LOCAL(RetainPtr<CFURLCacheRef>, cache, (AdoptCF, CFURLCacheCopySharedURLCache()));
> > +    if (!cache)
> > +        return;
> 
> Don't we change the cache when enabling private browsing? I'm also unsure how this will work with cache partitioning.

I can't find anyplace where we ever set a new shared cache, and that is the only context we ever have at this site.

Cache partitioning is done with a partition string as an argument to the shared cache object, which is we have to preserve the original resource request which contains this string.

> 
> > Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:99
> > +void NetworkResourceLoader::willCacheResponseAsync(WebCore::ResourceHandle*, NSCachedURLResponse* nsResponse)
> 
> Please no WebCore:: prefix. This also has a misplaced star.

Will fix.

[Alexey Proskuryakov]

> I can't find anyplace where we ever set a new shared cache, and that is the only context we ever have at this site.

It would be done indirectly, via switching to a new storage session.

[Geoffrey Garen]

/Volumes/Data/EWS/WebKit/Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:115:32: error: no member named 'tryGetShareableHandleFromCFURLCachedResponse' in 'WebKit::NetworkResourceLoader'
        NetworkResourceLoader::tryGetShareableHandleFromCFURLCachedResponse(handle, cachedResponse);
        ~~~~~~~~~~~~~~~~~~~~~~~^
1 error generated.

[Brady Eidson]

Created attachment 195384 [details]
Patch v3 - Much better, I think!

[Alexey Proskuryakov]

Comment on attachment 195384 [details]
Patch v3 - Much better, I think!

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

> Source/WebKit2/NetworkProcess/mac/DiskCacheMonitor.h:30
> +#include "NetworkResourceLoader.h"

Still here?

> Source/WebKit2/NetworkProcess/mac/DiskCacheMonitor.h:47
> +    NetworkConnectionToWebProcess* connectionToWebProcess() const { return m_connectionToWebProcess.get(); }

I don't see this used.

> Source/WebKit2/NetworkProcess/mac/DiskCacheMonitor.h:59
> +void monitorFileBackingStoreCreation(CFCachedURLResponseRef, NetworkResourceLoader*);

Why not make this a static function in the class, to avoid friending it?

> Source/WebKit2/NetworkProcess/mac/DiskCacheMonitor.h:61
> +} // namspace WebKit

namespace, if you insist on adding a comment.

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:53
> +@interface NSCachedURLResponse (NSCachedURLResponseInternals)

We now say Details, not Internals.

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:81
> +    RetainPtr<CFURLCacheRef> cache(AdoptCF, CFURLCacheCopySharedURLCache()); 

I think our preferred pattern is "= adoptCF(...)" these days.

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:99
> +void NetworkResourceLoader::willCacheResponseAsync(ResourceHandle*, NSCachedURLResponse *nsResponse)

I'd ASSERT(handle == m_handle).

[Brady Eidson]

I'll meet Geoff partway on the delay and bump it to 20s before landing (this patch had it at 0.2 still from some debugging - yikes!)

[Brady Eidson]

And addressed Alexey's new feedback, of course.

[Brady Eidson]

http://trac.webkit.org/changeset/147006