Closed Bug 81724 Opened 24 years ago Closed 22 years ago

disk cache needs stream wrappers (disk cache phase 3)


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






(Reporter: gordon, Assigned: gordon)



(Keywords: perf, Whiteboard: [adt2])


(1 file, 5 obsolete files)

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.
Target Milestone: --- → mozilla0.9.2
Blocks: latemeta
Stream wrappers are also necessary to implement over-lapped i/o for the disk 
No longer blocks: latemeta
Blocks: 79983
Priority: -- → P2
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
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Keywords: perf
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
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Priority: P2 → P1
Keywords: nsbeta1+
Blocks: 122597
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
Blocks: 77458
No longer blocks: latemeta
Whiteboard: [adt3]
What are the chances this will be ready by 04/01? Can we get an ETA on this one?
Whiteboard: [adt3] → [adt3] [ETA ?]
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 ?]
Attached patch first rough cut at patch (obsolete) — Splinter Review
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
Attached patch updated patch (obsolete) — Splinter Review
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.
what are the chances this is gonna make Mozilla 1.0.1?
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
If we can get a finalized patch, I can help getting some performance #s on it.  
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
Whiteboard: [adt2] [ETA ?] → [adt2 rtm] [ETA ?]
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?
Keywords: nsbeta1+nsbeta1-
removing [adt2 rtm] [ETA ?], as well
Whiteboard: [adt2 rtm] [ETA ?]
patch crashes on windows... so it's not yet ready.
Gordon, do you have an updated patch for us to try?
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)
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
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
Attachment #93668 - Attachment is obsolete: true
Here's the latest, incorporating all changes from Darin's review.
Attachment #93768 - Attachment is obsolete: true
Comment on attachment 93783 [details] [diff] [review]
polished patch with updates from darin's review

sr=darin (nice work!)
Attachment #93783 - Flags: superreview+
a perf improvement sounds like a good thing to have for the next release.
nominating for nsbeta1.
Keywords: nsbeta1-mozilla1.2, nsbeta1
Whiteboard: [adt2]
Target Milestone: mozilla1.0 → mozilla1.2beta
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.
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!!
Attachment #93783 - Flags: review+
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
dougt: see bug 88341 (gordon says he's planning on fixing that bug when he gets
back from vacation).
fixed-on-trunk :-)
Closed: 22 years ago
Resolution: --- → FIXED
This commit have added a new "might be used uninitialized" warning (see also bug
59652) on brad TBox:

+ `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?

see bug 161417 ... i'll take care of fixing warning there.
Verified per comment #30.
QA Contact: tever → junruh
You need to log in before you can comment on or make changes to this bug.