Last Comment Bug 779461 - Do not transfer the ownership of the key string from nsCacheRequest to nsCacheEntry
: Do not transfer the ownership of the key string from nsCacheRequest to nsCach...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: 11 Branch
: All All
: -- normal (vote)
: mozilla17
Assigned To: Michal Novotny (:michal)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-01 04:51 PDT by Michal Novotny (:michal)
Modified: 2012-08-08 09:30 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (12.83 KB, patch)
2012-08-01 04:51 PDT, Michal Novotny (:michal)
honzab.moz: review+
Details | Diff | Review
patch v2 - removed comment, r=honzab (12.89 KB, patch)
2012-08-07 17:02 PDT, Michal Novotny (:michal)
no flags Details | Diff | Review

Description Michal Novotny (:michal) 2012-08-01 04:51:53 PDT
Created attachment 647912 [details] [diff] [review]
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) 2012-08-07 12:42:29 PDT
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) 2012-08-07 17:02:18 PDT
Created attachment 649886 [details] [diff] [review]
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 4 Ed Morley [:emorley] 2012-08-08 09:30:41 PDT
https://hg.mozilla.org/mozilla-central/rev/db1308490142

Note You need to log in before you can comment on or make changes to this bug.