Closed
Bug 289352
Opened 20 years ago
Closed 19 years ago
nsDiskCacheBlockFile: remove redundant Seek, and optimize bitmap usage
Categories
(Core :: Networking: Cache, defect)
Core
Networking: Cache
Tracking
()
RESOLVED
FIXED
People
(Reporter: alfredkayser, Assigned: darin.moz)
Details
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
darin.moz
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
darin.moz
:
review+
|
Details | Diff | Splinter Review |
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...)
Reporter | ||
Comment 1•20 years ago
|
||
Reporter | ||
Comment 2•19 years ago
|
||
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...
Assignee | ||
Comment 3•19 years ago
|
||
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?
Assignee | ||
Updated•19 years ago
|
Attachment #179881 -
Flags: review?(darin)
Reporter | ||
Comment 4•19 years ago
|
||
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).
Assignee | ||
Comment 5•19 years ago
|
||
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?
Attachment #179881 -
Flags: review?(darin) → review-
Reporter | ||
Comment 6•19 years ago
|
||
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...
Reporter | ||
Comment 7•19 years ago
|
||
Attachment #179881 -
Attachment is obsolete: true
Reporter | ||
Updated•19 years ago
|
Attachment #190248 -
Flags: review?(darin)
Assignee | ||
Comment 8•19 years ago
|
||
Comment on attachment 190248 [details] [diff] [review]
V2: Increase also the CacheVersion number, because of the changed bits usage
r=darin w/ some tweaks
Attachment #190248 -
Flags: review?(darin) → review+
Assignee | ||
Comment 9•19 years ago
|
||
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.
Assignee | ||
Comment 10•19 years ago
|
||
fixed-on-trunk
Thanks for the patch Alfred! Sorry it took me so long to get your patch into
the tree :(
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 11•19 years ago
|
||
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.
Reporter | ||
Updated•19 years ago
|
Attachment #199047 -
Flags: review?(darin)
Assignee | ||
Comment 12•19 years ago
|
||
Comment on attachment 199047 [details] [diff] [review]
Fix to also swap back the words from network to host format [fixed-on-trunk]
r=darin
Attachment #199047 -
Flags: review?(darin) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #199047 -
Attachment description: Fix to also swap back the words from network to host format → Fix to also swap back the words from network to host format [fixed-on-trunk]
You need to log in
before you can comment on or make changes to this bug.
Description
•