Bug 132464 - Reduce calls to CFURLCacheCopySharedURLCache
Summary: Reduce calls to CFURLCacheCopySharedURLCache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pratik Solanki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-05-01 23:27 PDT by Pratik Solanki
Modified: 2014-05-04 17:43 PDT (History)
4 users (show)

See Also:

Attachments
Patch (3.75 KB, patch)
2014-05-01 23:31 PDT, Pratik Solanki 		no flags Details | Formatted Diff | Diff
Patch with assert and raw pointer (4.19 KB, patch)
2014-05-02 10:57 PDT, Pratik Solanki 		ap: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Pratik Solanki 2014-05-01 23:27:04 PDT 
Reduce calls to CFURLCacheCopySharedURLCache
Comment 1 Pratik Solanki 2014-05-01 23:31:28 PDT 
Created attachment 230655 [details]
Patch
Comment 2 Pratik Solanki 2014-05-01 23:33:50 PDT 
My only concern here would be if someone was calling CFURLCacheSetSharedCache or [NSURLCache setSharedURLCache] after we cache the url ref in the static. I could't find any such callers though.
Comment 3 Alexey Proskuryakov 2014-05-02 00:51:43 PDT 
Comment on attachment 230655 [details]
Patch

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

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:89
> +    static RetainPtr<CFURLCacheRef> cache = adoptCF(CFURLCacheCopySharedURLCache());

This can't be a RetainPtr, because we don't want exit time destructors. A plain pointer would be appropriate.

An ASSERT in debug builds would ensure that we don't break this optimization, although that may be overkill.
Comment 4 Pratik Solanki 2014-05-02 10:57:49 PDT 
Created attachment 230675 [details]
Patch with assert and raw pointer
Comment 5 Alexey Proskuryakov 2014-05-02 11:30:10 PDT 
Comment on attachment 230675 [details]
Patch with assert and raw pointer

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

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:89
> +    static CFURLCacheRef cache = CFURLCacheCopySharedURLCache();

I'd also add ASSERT(isMainThread()) here, as this initialization is not thread safe.
Comment 6 Pratik Solanki 2014-05-03 17:03:35 PDT 
<rdar://problem/16806694>
Comment 7 Pratik Solanki 2014-05-03 17:15:25 PDT 
Committed r168231: <http://trac.webkit.org/changeset/168231>
Comment 8 Darin Adler 2014-05-03 20:28:27 PDT 
Comment on attachment 230675 [details]
Patch with assert and raw pointer

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

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:93
> +#if !ASSERT_DISABLED
> +    RetainPtr<CFURLCacheRef> currentCache = adoptCF(CFURLCacheCopySharedURLCache());
> +    ASSERT(cache == currentCache.get());
> +#endif

To avoid this ugly if, just get rid of the local variable:

    ASSERT(currentCache == adoptCF(CFURLCacheCopySharedURLCache()));

Also no need for the call to get() when doing ==.
Comment 9 Pratik Solanki 2014-05-04 17:43:44 PDT 
(In reply to comment #8)
> (From update of attachment 230675 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230675&action=review
> 
> > Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:93
> > +#if !ASSERT_DISABLED
> > +    RetainPtr<CFURLCacheRef> currentCache = adoptCF(CFURLCacheCopySharedURLCache());
> > +    ASSERT(cache == currentCache.get());
> > +#endif
> 
> To avoid this ugly if, just get rid of the local variable:
> 
>     ASSERT(currentCache == adoptCF(CFURLCacheCopySharedURLCache()));
> 
> Also no need for the call to get() when doing ==.

Yes, this looks much better. I landed this in <http://trac.webkit.org/changeset/168250>.