779461 - Do not transfer the ownership of the key string from nsCacheRequest to nsCacheEntry

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Michal Novotny [:michal]]

Attachment: fix

It doesn't seem to be a big performance win to transfer the string from nsCacheRequest to nsCacheEntry, because strings share the buffer. Having the key available in the request for its whole lifecycle is good e.g. for debugging and logging (e.g. bug #729182 needs it).

Comment 1

[Honza Bambas (:mayhemer)]

Comment on attachment 647912 [details] [diff] [review]
fix

Review of attachment 647912 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

::: netwerk/cache/nsCacheEntry.cpp
@@ +62,5 @@
>                        nsCacheStoragePolicy  storagePolicy,
>                        nsCacheDevice *       device,
>                        nsCacheEntry **       result)
>  {
> +    nsCacheEntry* entry = new nsCacheEntry(nsCString(key),

Maybe the constructor could take just const char * key, but up to you.

::: netwerk/cache/nsCacheEntry.h
@@ +208,5 @@
>      void MarkInitialized()     { mFlags |=  eInitializedMask; }
>      void MarkActive()          { mFlags |=  eActiveMask; }
>      void MarkInactive()        { mFlags &= ~eActiveMask; }
>  
> +    nsCString               mKey;            // 4  // XXX ask scc about const'ness

"// 4 and so" should probably be removed.

Comment 2

[Michal Novotny [:michal]]

Attachment: patch v2 - removed comment, r=honzab

(In reply to Honza Bambas (:mayhemer) from comment #1)
> >  {
> > +    nsCacheEntry* entry = new nsCacheEntry(nsCString(key),
> 
> Maybe the constructor could take just const char * key, but up to you.

Then mKey in nsCacheEntry and in nsCacheRequest wouldn't share the buffer when initialized at http://hg.mozilla.org/mozilla-central/annotate/f53c2abc69bd/netwerk/cache/nsCacheService.cpp#l2019, right?


> >      void MarkInactive()        { mFlags &= ~eActiveMask; }
> >  
> > +    nsCString               mKey;            // 4  // XXX ask scc about const'ness
> 
> "// 4 and so" should probably be removed.

removed

Comment 3

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db1308490142

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/db1308490142