Closed
Bug 81724
Opened 24 years ago
Closed 23 years ago
disk cache needs stream wrappers (disk cache phase 3)
Categories
(Core :: Networking: Cache, defect, P1)
Core
Networking: Cache
Tracking
()
VERIFIED
FIXED
mozilla1.2beta
People
(Reporter: gordon, Assigned: gordon)
References
Details
(Keywords: perf, Whiteboard: [adt2])
Attachments
(1 file, 5 obsolete files)
88.64 KB,
patch
|
dougt
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
Before data can take advantage of the cache block files, we need to implement a
stream wrapper class that will be able to determine where best to store the
incoming stream.
Stream wrappers are also necessary to implement over-lapped i/o for the disk
cache.
No longer blocks: latemeta
can't make it to 0.9.2. pushing over...
Target Milestone: mozilla0.9.2 → mozilla0.9.3
I'm hoping to get time to finish this for 0.9.4.
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Won't get done for 0.9.6. Necko is focusing on performance for 0.9.7.
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Made progress, but it's unlikely this will make 0.9.9. We can pull it back if
it gets done earlier.
Target Milestone: mozilla0.9.9 → mozilla1.0
Comment 6•23 years ago
|
||
What are the chances this will be ready by 04/01? Can we get an ETA on this one?
Whiteboard: [adt3] → [adt3] [ETA ?]
Comment 7•23 years ago
|
||
Is this still on track to land on Friday, 4/5, pending approval?
This bug blocks 122597 which is adt2. Considering the impending cleanup I'm
moving this up to adt2.
Whiteboard: [adt3] [ETA ?] → [adt2] [ETA ?]
The patch still has two known minor bugs to fix, but it should be good enough
for performance testing against page-loader and iBench.
Makefile changes still need to be made to include nsDiskCacheStreams.cpp in
build.
Assignee | ||
Comment 10•23 years ago
|
||
Assignee | ||
Comment 11•23 years ago
|
||
This patch does NOT contain the reference counted lock class for the disk cache
and streamIOs, and it doesn't resolve the lock order issues to enable dooming
of partial cache entries resulting from errors.
Comment 12•23 years ago
|
||
what are the chances this is gonna make Mozilla 1.0.1?
Comment 13•23 years ago
|
||
jaimejr: i'm hoping to find some cycles to finish off this patch ASAP. i'd
really like to see it land for RTM, but i'm not sure yet if it's going to be a
reality.
Comment 14•23 years ago
|
||
If we can get a finalized patch, I can help getting some performance #s on it.
Comment 15•23 years ago
|
||
cathleen: the current patch is ready for performance testing... there are just a
couple known multithreaded race-conditions that need to be closed before this
will be ready for prime time :) the final patch shouldn't differ much
performance-wise
Updated•23 years ago
|
Whiteboard: [adt2] [ETA ?] → [adt2 rtm] [ETA ?]
Comment 16•23 years ago
|
||
spoke to darin, and he didn't think this would make 1.0.1, so nsbeta1-. plus,
Gordon's on sabatical.
should the milestone for this one be moved to mozilla1.1beta?
Comment 18•23 years ago
|
||
patch crashes on windows... so it's not yet ready.
Comment 19•23 years ago
|
||
Gordon, do you have an updated patch for us to try?
Assignee | ||
Comment 20•23 years ago
|
||
I'm back from sabbatical, and I'll probably post an updated patch in the next
day or so.
Summary: disk cache needs stream wrappers (disk cache level 3) → disk cache needs stream wrappers (disk cache phase 3)
Assignee | ||
Comment 21•23 years ago
|
||
I ran the page-load test and the minibench test with this patch, and I was able
to shutdown without crashing. I've tested on Mac, but we need to build and
test on the other platforms now. We also need timing tests for a variety of
platforms. I don't expect this will make much difference on Linux (other than
possibly startup time after a crash), but previous tests showed the a 500 MHz
Mac improving about 4%-7%. It will be interesting to see what we get on
various Windows machines.
This patch increments the cache version number so when you startup, an existing
cache with a different version number will be deleted. It works that way going
backwards too. This is a feature.
Attachment #81928 -
Attachment is obsolete: true
Attachment #84842 -
Attachment is obsolete: true
Attachment #85149 -
Attachment is obsolete: true
Assignee | ||
Comment 22•23 years ago
|
||
This patch updates the build problem Doug found, the deadlock problem Darin
found, and adds truncation for the cache block files when clearing the disk
cache.
Attachment #93668 -
Attachment is obsolete: true
Assignee | ||
Comment 23•23 years ago
|
||
Here's the latest, incorporating all changes from Darin's review.
Attachment #93768 -
Attachment is obsolete: true
Comment 24•23 years ago
|
||
Comment on attachment 93783 [details] [diff] [review]
polished patch with updates from darin's review
sr=darin (nice work!)
Attachment #93783 -
Flags: superreview+
Comment 25•23 years ago
|
||
a perf improvement sounds like a good thing to have for the next release.
nominating for nsbeta1.
Whiteboard: [adt2]
Target Milestone: mozilla1.0 → mozilla1.2beta
Comment 26•23 years ago
|
||
fwiw: i'm seeing about a 3% page-load improvement with this patch under win2k
against the cowtools page loader test (850MHz PII laptop w/ 256 MB RAM with a
"pretty fast" IDE drive). i'd expect results to vary considerably from machine
to machine.
Comment 27•23 years ago
|
||
also of interest, i spent some time browsing with this patch in my tree. i
downloaded 3669 documents taking up approximately 27.5 MB in the disk cache. of
those, only 339 ended up in separate files. that's roughly 1/10th the number of
separate files stored in the disk cache!!
Updated•23 years ago
|
Attachment #93783 -
Flags: review+
Comment 28•23 years ago
|
||
Comment on attachment 93783 [details] [diff] [review]
polished patch with updates from darin's review
I really hate having all of the platform specific code in the
nsDiskCache::Truncate(). This kind of thing probably should be supported in
nspr.
Comment 29•23 years ago
|
||
dougt: see bug 88341 (gordon says he's planning on fixing that bug when he gets
back from vacation).
Comment 30•23 years ago
|
||
fixed-on-trunk :-)
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 31•23 years ago
|
||
This commit have added a new "might be used uninitialized" warning (see also bug
59652) on brad TBox:
+netwerk/cache/src/nsDiskCacheEntry.cpp:106
+ `PRInt32 pad' might be used uninitialized in this function
Indeed, if size is > 16384, the pad will be uninitialized!
Should I file a new bug on this?
Comment 32•23 years ago
|
||
see bug 161417 ... i'll take care of fixing warning there.
Comment 33•22 years ago
|
||
Verified per comment #30.
Status: RESOLVED → VERIFIED
QA Contact: tever → junruh
You need to log in
before you can comment on or make changes to this bug.
Description
•