Last Comment Bug 674869 - Delay corrupt cache deletion to avoid startup I/O contention
: Delay corrupt cache deletion to avoid startup I/O contention
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla8
Assigned To: Jason Duell [:jduell] (needinfo? me)
:
Mentors:
: 676451 (view as bug list)
Depends on: 670911
Blocks: http_cache
  Show dependency treegraph
 
Reported: 2011-07-28 04:25 PDT by Jason Duell [:jduell] (needinfo? me)
Modified: 2011-08-03 18:56 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Delays Cache.trash delete for 1 minute at startup (7.53 KB, patch)
2011-07-28 16:49 PDT, Jason Duell [:jduell] (needinfo? me)
no flags Details | Diff | Review
Fixed from Michal's comments (7.23 KB, patch)
2011-08-01 16:25 PDT, Jason Duell [:jduell] (needinfo? me)
michal.novotny: review+
Details | Diff | Review

Description Jason Duell [:jduell] (needinfo? me) 2011-07-28 04:25:44 PDT
Followup to bug 670911.  Now that we've avoided UI hang, we should also be polite and avoid doing the (very significant for large HTTP caches) I/O to delete the cache, so that it doesn't block other IO done during startup.

I'm currently thinking about 2 minutes is a reasonable time.

I'm also leaning towards just deleting the cache in one go at that time.  It will be noticable (see bug 670911 comment 20: may be 20 seconds or more of IO), but amortizing it slowly enough that it's not noticeable has other bad effects (re-spinning up notebook hard drives), and it's not clear to me that, say, 16 noticeable bursts of drive activity is going to be less irritating than one big one.

The correct long-term fix, of course, is to not have so many small files in the tree, and/or reuse existing files like the cache map files.
Comment 1 Jason Duell [:jduell] (needinfo? me) 2011-07-28 16:49:50 PDT
Created attachment 549273 [details] [diff] [review]
Delays Cache.trash delete for 1 minute at startup

This patch also does some extra effort to make sure that users who get stuck in a cycle of crashes (with a stack of bad Cache dirs) can still make progress deleting the trash dirs if/when their browser stops the early crashing pattern.
Comment 2 Michal Novotny (:michal) 2011-08-01 08:57:30 PDT
Comment on attachment 549273 [details] [diff] [review]
Delays Cache.trash delete for 1 minute at startup

> +static void CreateDeleterThread(nsITimer *aTimer, void *arg)
> +{
> +  if (aTimer)
> +    aTimer->Release();

Could you please explain the logic? IMO if you don't call timer.forget() in DeleteDir(), you won't need to call Release() here.


> +nsresult DeleteDir(nsIFile *dirIn, PRBool moveToTrash, PRBool sync,
> +                   unsigned long delay)

Why not PRUint32?
Comment 3 Jason Duell [:jduell] (needinfo? me) 2011-08-01 16:25:25 PDT
Created attachment 549962 [details] [diff] [review]
Fixed from Michal's comments

> IMO if you don't call timer.forget() in DeleteDir(), you won't need to call Release() here.

You're right--I didn't realize the timer thread takes care of ensuring the timer is alive until it finishes firing.

> Why not PRUint32?

I spaced out and used the IDL type.  Fixed.

thanks.
Comment 4 Jason Duell [:jduell] (needinfo? me) 2011-08-02 17:05:59 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/feda6619295c
Comment 5 Marco Bonardo [::mak] 2011-08-03 02:25:21 PDT
has been pushed with the wrong bug number, luckily bugzilla has fulltext search...
http://hg.mozilla.org/mozilla-central/rev/feda6619295c
Comment 6 Jason Duell [:jduell] (needinfo? me) 2011-08-03 18:56:33 PDT
*** Bug 676451 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.