289352 - nsDiskCacheBlockFile: remove redundant Seek, and optimize bitmap usage

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Alfred Kayser]

In nsDiskCacheBlockFile the bitmaps are handled per byte, which means that:
* when allocating 2, 3 or 4 blocks only entries which fit on byte boundary can
be used
* in current code also assumes 'quadbit' boundaries, so that even less fitting
holes are used.
* this means that the blockfile bitmaps will get fragmented faster than needed.
* search for free space, using 32bit words is faster than 8bit (like strdup uses
memcpy as it is faster than strcpy because of 32bit access).

Other little things:
* Remove redundent PR_Seek and PR_Available (during open we allready know the
file size, so Validate should not get it again), by inlining ValidateFile
* Remove 'nsCRT.h' include
* Change LastBlock to return block count based 1, 0 meaning no blocks used, so
that instead 'if (lastBlock >=0) estimatedSize += (lastBlock+1)*blockSize' you
can do: 'estimatedSize += lastBlock*blockSize'

Summary of benefits:
* More efficient use of space in blockfiles, less fragmentation, smaller file
* Faster searching for bits (32bits iso 8bits)
* Less code (about 700bytes...)

Comment 1

[Alfred Kayser]

Attachment: Patch as described above

Comment 2

[Alfred Kayser]

Current measures point out that this code is at least as fast as the old one,
but the timings fluctuate enormously. However the more efficient bitmap usage
indeed result in about 11-12% _CACHE_00x_ file size reduction. This adds to more
than 1MB of diskspace savings...

Comment 3

[Darin Fisher]

Does this change impact the format of the cache block files?  Will we need to
bump the disk cache version if we take this patch?

Comment 4

[Alfred Kayser]

The format doesn't 'officially' change. However the old diskcachemap bitmap code
expects chunks of 2,3 or 4 bits to be byte aligned, instead of word aligned.
So the version number should be increased, as the old code won't be able read
the new cachemap files (but new code can read and use cachemap's written by old
code).

Comment 5

[Darin Fisher]

Comment on attachment 179881 [details] [diff] [review]
Patch as described above

The patch looks good to me, but could you please post a revised patch that
bumps the disk cache version.

Also, it seems that ValidateFile used to fail if the file had been deleted
unexpectedly.  Should we worry that not having that check at this point could
be trouble?

Comment 6

[Alfred Kayser]

Concerning 'ValidateFile': it is only called in the init code, directly after
the 'read', so checking for deletion of file in between is not needed.
Updated patch for cacheVersion coming up...

Comment 7

[Alfred Kayser]

Attachment: V2: Increase also the CacheVersion number, because of the changed bits usage

Comment 8

[Darin Fisher]

Comment on attachment 190248 [details] [diff] [review]
V2: Increase also the CacheVersion number, because of the changed bits usage

r=darin w/ some tweaks

Comment 9

[Darin Fisher]

Attachment: v2.1 patch

Here's the version that I checked in on the trunk.  I cleaned up some
whitespace, and replaced PR_htonl with htonl (same for PR_ntohl).  I also
eliminated the copying of mBitMap on BIG endian platforms.

Comment 10

[Darin Fisher]

fixed-on-trunk

Thanks for the patch Alfred!  Sorry it took me so long to get your patch into
the tree :(

Comment 11

[Alfred Kayser]

Attachment: Fix to also swap back the words from network to host format [fixed-on-trunk]

Darin, somehow this was lost somewhere.
In the 'Open' call, when reading a bitmap back from file, we need to also swap
the words back from network to host format.

Comment 12

[Darin Fisher]

Comment on attachment 199047 [details] [diff] [review]
Fix to also swap back the words from network to host format [fixed-on-trunk]

r=darin