53312 - jar: implement memory flusher for zip cache

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[Chris Waterson]

Per (my recollection of) jband's investigation, this was accounting for
something like 200K per JAR file or something? Need to hook up as a memory
pressure observer.

Comment 1

[Chris Waterson]

dprice, you working on this already?

Comment 2

[dprice (gone)]

I'd like to start looking at this on Monday

Comment 3

[Warren Harris]

We need to review the zip cache code at some point too. I've been occasionally 
seeing the assertion "wacked" on line 1254 being hit. This indicates some sort 
of race condition.

Comment 4

[John Bandhauer]

hey dprice, please attach the patches we worked on.

I'll attach the race condition crasher fix I did and sent to you guys last week.

I thought you guys were going to get this stuff in.

You have the flusher enabling changes on your machine. This should include 
and superceed my changes. Please attach the patch.

The Jar protocol changes have us doing a lot of reading from these jars on 
various threads right? That race problem could be crashing in rtm as it is now, 
right? Let's get these checked into at least the trunk and get some burn in 
time. If we want them in the rtm branch we better get moving.

Comment 5

[John Bandhauer]

Attachment: proposed fix for race. also fixes debug stats (w/ CRLF)

Comment 6

[John Bandhauer]

warren, do you want to spin the race fix off to a separate (crasher?) bug?

Anyway, review needed.

Comment 7

[Warren Harris]

see bug 55893 - race in zip cache code

Comment 8

[dprice (gone)]

Attachment: fix

Comment 9

[dprice (gone)]

diff includes fix for 55893

Comment 10

[John Bandhauer]

Dave, "diff -u" is our standard.

Comment 11

[dprice (gone)]

sr=warren,jband

Comment 12

[John Bandhauer]

Dave, please attach a diff -u of exactly what you intend to checkin... Those 
non-#ifdef'd printfs are not going to get checked in, right?

Comment 13

[dprice (gone)]

checked in the fix

Comment 14

[dprice (gone)]

backed out the fix

Comment 15

[dprice (gone)]

Attachment: attaching diff -u

Comment 16

[John Bandhauer]

r=jband

Looks right. Passes the tests. flushes the files and does not crash when I 
choose 'flush' in the debug menu.

A nit: you still have 3 tabs in the file. You ought to remove them. I suggest 
you set your editor to not insert tabs. Many people have editors that actually 
read and obey the 'Mode' line at the top of each file. When tabs *are* in the 
file they had better follow the Mode line's 'tab-width' directive (2 in this 
case). Note that "indent-tabs-mode: nil" says that new tabs should not be added.

Comment 17

[dprice (gone)]

Attachment: new diff that include nsIWeakReference

Comment 18

[John Bandhauer]

r=jband

Thanks Dave.

Comment 19

[Chris Waterson]

Sorry to keep dribbling criticism :-(

- the Observe() method should be checking the "topic". Not that big of a deal
because the observer service will only notify for memory pressure stuff, but
it'll future proof the code (for when the zip cache starts spying on the next
door neighbors).

- there is no need to unregister from the observer service since you're a weak
ref (and doing so may trigger annoying "attempt to acquire service during XPCOM
shutdown" asserts)

- I don't really grok the other changes (e.g., "interval no timeout" semantics),
but trust jband. warren, maybe you could take a quick look at those to sanity check.

Make the above changes, get warren to look over the "interval no timeout" stuff,
and sr=waterson.

Comment 20

[dprice (gone)]

Attachment: new diff that checks the topic and doesn't unregister the observer

Comment 21

[John Bandhauer]

r=jband 

I agree with waterson's suggestions - wish I'd thought of them! It looks like 
you implemented the suggestions correctly.

Comment 22

[Chris Waterson]

This is unnecessary now:

 nsZipReaderCache::~nsZipReaderCache()
 {
+  nsresult rv;
   if (mLock)
     PR_DestroyLock(mLock);

And you should move the nsAutoLock inside the "if", but before the "while" (lock 
minimal sections of code):

+nsZipReaderCache::Observe(nsISupports *aSubject,
+                          const PRUnichar *aTopic, 
+                          const PRUnichar *aSomeData)
+{
+  nsAutoLock lock(mLock);
+
+  if (nsCRT::strcmp(aTopic, NS_MEMORY_PRESSURE_TOPIC) == 0) {
+    while (PR_TRUE)

Fix those two things (I don't need to see another diff). If warren is ok with 
the other stuff, sr=waterson.

thanks for your patience!

Comment 23

[Warren Harris]

Chris' last set of suggestions sound good to me. The interval no timeout stuff 
seems right too. Make it so, Dave.

Comment 24

[dprice (gone)]

marking fixed