Delay corrupt cache deletion to avoid startup I/O contention

RESOLVED FIXED in mozilla8

Status

()

Core
Networking: Cache
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jduell, Assigned: jduell)

Tracking

(Blocks: 1 bug)

unspecified
mozilla8
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 1 obsolete attachment)

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.
(Assignee)

Comment 1

6 years ago
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.
Assignee: nobody → jduell.mcbugs
Status: NEW → ASSIGNED
Attachment #549273 - Flags: review?(michal.novotny)
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?
Attachment #549273 - Flags: review?(michal.novotny)
(Assignee)

Comment 3

6 years ago
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.
Attachment #549273 - Attachment is obsolete: true
Attachment #549962 - Flags: review?(michal.novotny)
Attachment #549962 - Flags: review?(michal.novotny) → review+
(Assignee)

Comment 4

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/feda6619295c
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
has been pushed with the wrong bug number, luckily bugzilla has fulltext search...
http://hg.mozilla.org/mozilla-central/rev/feda6619295c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Duplicate of this bug: 676451
You need to log in before you can comment on or make changes to this bug.