84945 - nsDiskCacheMap::ReadDiskCacheEntry should use a cached buffer

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect, P3)

Description

[Darin Fisher]

It looks like nsDiskCacheMap::ReadDiskCacheEntry could be modified to use
memory mapped i/o which would in this case reduce a malloc and buffer copy.

ReadDiskCacheEntry returns a pointer to a block of memory which is, with the
exception of byte swapping, identical to its on-disk representation, so it
should be possible to make ReadDiskCacheEntry simply return the result from
PR_MemMap().  There would need to be a corresponding ReleaseDiskCacheEntry
function to call PR_MemUnmap().

Currently, this is only possible on XP_UNIX and XP_WIN, since NSPR doesn't
implement memory mapped i/o for the Mac (though it is available for recent 
versions of MacOS).

I think this might be a big win for the disk cache, since this code is
frequently called from the UI thread.

Comment 1

[Darin Fisher]

I should add that the result from ReadDiskCacheEntry is only used temporarily.

Comment 2

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 3

[Vedran Miletic]

retargeting

Comment 4

[Alfred Kayser]

Instead of doing PR_MemMap (which is reported to be very slow on Windows), one could also cache the DiskCacheEntry buffer (as it is generaly 256 or 512 bytes large, and only used very shortly (to either only verify the 'client id' or to create a real nsCacheEntry from it. nsDiskCacheMap could be the owner of this buffer.

Comment 5

[Alfred Kayser]

Attachment: V1: Let the diskcache use a cached buffer for DiskCacheEntry reads & writes

This patch makes it so that nsDiskCacheMap maintains one buffer for all nsDiskCacheEntry reads and writes. In all reads and writes the buffer was allocated and destroyed in very short timeframe. By keeping the buffer (generally 512 bytes), further allocations are prevented.

This patch also cleans up some error handling and some goto's.
(Note, in the Read() function, when failing to read from the open file, the open file wasn't closed, because of the double 'fp' definition)
Also moved the CreateDiskCacheEntry function to the right place, as it is only used within nsDiskCacheMap

Comment 6

[Alfred Kayser]

Attachment: V2: Removed the printf

Comment 7

[Damon Sicore (:damons)]

+'ing.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Darin's gone and not doing reviews anymore. roc suggests asking biesi or dcamp to review it instead.

Comment 9

[Damon Sicore (:damons)]

Please request dcamp review this unless totally inappropriate.  Want to minimize Biesi's work load.

Comment 10

[Dave Camp (:dcamp)]

Comment on attachment 288641 [details] [diff] [review]
V2: Removed the printf

>+++ mozilla/netwerk/cache/src/nsDiskCacheDevice.cpp	14 Nov 2007 10:13:45 -0000
>@@ -130,17 +130,16 @@ nsDiskCacheEvictor::VisitRecord(nsDiskCa
>     
>         // Compare clientID's without malloc
>         if ((diskEntry->mKeySize <= mClientIDSize) ||
>             (diskEntry->Key()[mClientIDSize] != ':') ||
>             (memcmp(diskEntry->Key(), mClientID, mClientIDSize) != 0)) {
>             delete [] (char *)diskEntry;
>             return kVisitNextRecord;  // clientID doesn't match, skip it

That delete needs to be removed too.

>+++ mozilla/netwerk/cache/src/nsDiskCacheMap.cpp	14 Nov 2007 10:13:45 -0000
> nsresult
> nsDiskCacheMap::ReadDiskCacheEntry(nsDiskCacheRecord * record, nsDiskCacheEntry ** result)

This method is now breaking pretty common ownership conventions, there should probably at least be a comment explaining that.  Same thing for CreateDiskCacheEntry.

>     } else {
>         // pass ownership to caller
>         *result = diskEntry;
>         diskEntry = nsnull;
>     }

This comment is now kinda wrong.
        
>+++ mozilla/netwerk/cache/src/nsDiskCacheMap.h	14 Nov 2007 10:13:45 -0000

>+    ~nsDiskCacheMap() {
>+        (void) Close(PR_TRUE);
>+    }

Should probably free mBuffer here.

Comment 11

[Dave Camp (:dcamp)]

Comment on attachment 288641 [details] [diff] [review]
V2: Removed the printf

biesi should do the sr here

Comment 12

[Alfred Kayser]

Attachment: V3: Addressed comments of Dave

dcamp1: That delete needs to be removed too.
Done

dcamp2: This method is now breaking pretty common ownership conventions, there should
probably at least be a comment explaining that.  Same thing for
CreateDiskCacheEntry.
Made both functions return the pointer to the read/constructed diskEntry, and added comment to explain so.

dcamp3: This comment is now kinda wrong.
Fixed comment

dcamp4: Should probably free mBuffer here.
Fixed letting the Close() function free the mBuffer, like the other allocations

Comment 13

[Alfred Kayser]

From http://people.mozilla.com/%7Epavlov/frag/allocs8.txt.gz (from http://blog.pavlov.net/2007/11/13/allocation-data/), the entries that are covered by this bug are:
              XUL`nsDiskCacheMap::ReadDiskCacheEntry(nsDiskCacheRecord*, nsDiskCacheEntry**)+0x199

           value  ------------- Distribution ------------- count    
             256 |                                         2        
             512 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@ 631      

and:
              XUL`CreateDiskCacheEntry(nsDiskCacheBinding*, unsigned int*)+0x4c

           value  ------------- Distribution ------------- count    
              64 |                                         1        
             128 |                                         4        
             256 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@    796      
             512 |@@@                                      58       
            1024 |                                         6        

So, by keeping the buffer around, about 1500 allocations of 256 or 512 bytes are saved.

Comment 14

[Worcester12345]

perf key word?

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 289370 [details] [diff] [review]
V3: Addressed comments of Dave

+    memcpy(diskEntry->Key(), entry->Key()->get(),keySize);

while you're here, please add a space before keySize

+        char * buf = (char *)PR_REALLOC(mBuffer, bufSize);

hm... why not just call PR_Realloc? (or even realloc())

Comment 16

[Alfred Kayser]

Attachment: V4: Address comments of cbiesinger, ready to be checked in

Comment 17

[Alfred Kayser]

Note, there are other locations where PR_REALLOC (and MALLOC/CALLOC) are used?

Comment 18

[Reed Loden [:reed]]

Checking in netwerk/cache/src/nsDiskCacheDevice.cpp;
/cvsroot/mozilla/netwerk/cache/src/nsDiskCacheDevice.cpp,v  <--  nsDiskCacheDevice.cpp
new revision: 1.107; previous revision: 1.106
done
Checking in netwerk/cache/src/nsDiskCacheEntry.cpp;
/cvsroot/mozilla/netwerk/cache/src/nsDiskCacheEntry.cpp,v  <--  nsDiskCacheEntry.cpp
new revision: 1.24; previous revision: 1.23
done
Checking in netwerk/cache/src/nsDiskCacheEntry.h;
/cvsroot/mozilla/netwerk/cache/src/nsDiskCacheEntry.h,v  <--  nsDiskCacheEntry.h
new revision: 1.15; previous revision: 1.14
done
Checking in netwerk/cache/src/nsDiskCacheMap.cpp;
/cvsroot/mozilla/netwerk/cache/src/nsDiskCacheMap.cpp,v  <--  nsDiskCacheMap.cpp
new revision: 1.27; previous revision: 1.26
done
Checking in netwerk/cache/src/nsDiskCacheMap.h;
/cvsroot/mozilla/netwerk/cache/src/nsDiskCacheMap.h,v  <--  nsDiskCacheMap.h
new revision: 1.19; previous revision: 1.18
done

Comment 19

[Alfred Kayser]

Attachment: Restore the Save/Restore security info pieces

Oops, The previous patch accidentally removed the "Save and Restore security info, if present" parts.

Comment 20

[Alfred Kayser]

Attachment: Right version

Comment 21

[Stephen Donner [:stephend] Not actively reading bugmail]

I can't prove that this caused bug 410457, but it does appear likely.

Comment 22

[[e-mail]]

Comment on attachment 294822 [details] [diff] [review]
Right version

no need to request review to fix a mistake made in the patch after reviews were granted. i've landed the fix to add the security info stuff back - thanks!