Bug 132464

Summary: Reduce calls to CFURLCacheCopySharedURLCache
Product: WebKit Reporter: Pratik Solanki <psolanki>
Component: New BugsAssignee: 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   
Attachments:
Description Flags
Patch
none
Patch with assert and raw pointer ap: review+

Pratik Solanki
Reported 2014-05-01 23:27:04 PDT
Reduce calls to CFURLCacheCopySharedURLCache
Attachments
Patch (3.75 KB, patch)
2014-05-01 23:31 PDT, Pratik Solanki 		no flags
DetailsFormatted DiffDiff
Patch with assert and raw pointer (4.19 KB, patch)
2014-05-02 10:57 PDT, Pratik Solanki 		ap: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Pratik Solanki
Comment 1 2014-05-01 23:31:28 PDT
Created attachment 230655 [details] Patch
Pratik Solanki
Comment 2 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.
Alexey Proskuryakov
Comment 3 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.
Pratik Solanki
Comment 4 2014-05-02 10:57:49 PDT
Created attachment 230675 [details] Patch with assert and raw pointer
Alexey Proskuryakov
Comment 5 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.
Pratik Solanki
Comment 6 2014-05-03 17:03:35 PDT
<rdar://problem/16806694>
Pratik Solanki
Comment 7 2014-05-03 17:15:25 PDT
Committed r168231: <http://trac.webkit.org/changeset/168231>
Darin Adler
Comment 8 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 ==.
Pratik Solanki
Comment 9 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>.
Note You need to log in before you can comment on or make changes to this bug.