81724 - disk cache needs stream wrappers (disk cache phase 3)

Status: VERIFIED FIXED

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

Description

[gordon]

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.

Comment 1

[gordon]

Stream wrappers are also necessary to implement over-lapped i/o for the disk 
cache.

Comment 2

[Gagan]

can't make it to 0.9.2. pushing over...

Comment 3

[gordon]

I'm hoping to get time to finish this for 0.9.4.

Comment 4

[gordon]

Won't get done for 0.9.6. Necko is focusing on performance for 0.9.7.

Comment 5

[gordon]

Made progress, but it's unlikely this will make 0.9.9.  We can pull it back if
it gets done earlier.

Comment 6

[Jaime Rodriguez, Jr.]

What are the chances this will be ready by 04/01? Can we get an ETA on this one?

Comment 7

[scottputterman]

Is this still on track to land on Friday, 4/5, pending approval?

Comment 8

[Gagan]

This bug blocks 122597 which is adt2. Considering the impending cleanup I'm
moving this up to adt2. 

Comment 9

[gordon]

Attachment: first rough cut at patch

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.

Comment 10

[gordon]

Attachment: updated patch

Comment 11

[gordon]

Attachment: incorporating some review feedback from darin and beard.

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

[Jaime Rodriguez, Jr.]

what are the chances this is gonna make Mozilla 1.0.1?

Comment 13

[Darin Fisher]

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

[Cathleen]

If we can get a finalized patch, I can help getting some performance #s on it.  

Comment 15

[Darin Fisher]

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

Comment 16

[Jaime Rodriguez, Jr.]

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 17

[Jaime Rodriguez, Jr.]

removing [adt2 rtm] [ETA ?], as well

Comment 18

[Cathleen]

patch crashes on windows... so it's not yet ready.

Comment 19

[André Dahlqvist]

Gordon, do you have an updated patch for us to try?

Comment 20

[gordon]

I'm back from sabbatical, and I'll probably post an updated patch in the next
day or so.

Comment 21

[gordon]

Attachment: patch resolving thread saftey issues left in previous patch

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.

Comment 22

[gordon]

Attachment: updated patch, fixes linux build, fixes 1 deadlock bug, truncates block files

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.

Comment 23

[gordon]

Attachment: polished patch with updates from darin's review

Here's the latest, incorporating all changes from Darin's review.

Comment 24

[Darin Fisher]

Comment on attachment 93783 [details] [diff] [review]
polished patch with updates from darin's review

sr=darin (nice work!)

Comment 25

[Jaime Rodriguez, Jr.]

a perf improvement sounds like a good thing to have for the next release.
nominating for nsbeta1.

Comment 26

[Darin Fisher]

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

[Darin Fisher]

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!!

Comment 28

[Doug Turner (:dougt)]

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

[Darin Fisher]

dougt: see bug 88341 (gordon says he's planning on fixing that bug when he gets
back from vacation).

Comment 30

[Darin Fisher]

fixed-on-trunk :-)

Comment 31

[Aleksey Nogin]

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

[Darin Fisher]

see bug 161417 ... i'll take care of fixing warning there.

Comment 33

[John Unruh]

Verified per comment #30.