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

RESOLVED FIXED in mozilla17

Status

()

Core
Networking: Cache
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: michal, Assigned: michal)

Tracking

11 Branch
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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).
Attachment #647912 - Flags: review?(honzab.moz)
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.
Attachment #647912 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 2

5 years ago
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
Attachment #647912 - Attachment is obsolete: true
(Assignee)

Comment 3

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

Comment 4

5 years ago
https://hg.mozilla.org/mozilla-central/rev/db1308490142
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.