Closed Bug 780922 Opened 12 years ago Closed 11 years ago

Remove code to allow the old max size of the disk cache

Categories

(Core :: Networking: Cache, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: u408661, Assigned: u408661)

References

Details

Attachments

(2 files)

+++ This bug was initially created as a clone of Bug #709297 +++

In bug 709297, we added a way to reduce the max size of the cache without having to delete people's data when we wouldn't have already done so. At some point (likely around firefox 20), we need to just bite the bullet and not allow the old max size any more. This is so we don't have this adaptive code sitting around just waiting to break in some new and unexpected way.
So looking at telemetry, 93% of users on release and beta, 95% on aurora, and 97% on nightly are now using the new max smart size from natural causes (crash, new profile, etc). I think it's time we bite the bullet and have the final 3%-7% switch over. Patches incoming.
This patch backs out the code that switches from 1GB max to 350MB max smart size when the cache starts fresh, leaving us in the state we were in before any of those patches landed. The next patch changes the max from 1GB to 350MB for everyone.
Attachment #730292 - Flags: review?(michal.novotny)
This is the meat - make all profiles use the new max smart size. This means that any users who are still on the old size will have a large amount of their cache entries evicted when they upgrade to a build with this patch applied. Requesting sr from Patrick to make sure we want to do this now, as opposed to waiting to see if we get more people on the new size via natural causes.
Attachment #730295 - Flags: superreview?(mcmanus)
Attachment #730295 - Flags: review?(michal.novotny)
Try looks good:

https://tbpl.mozilla.org/?tree=Try&rev=2498ed657a68
and
https://tbpl.mozilla.org/?tree=Try&rev=3bfe046d11f8

(the second is because my backout accidentally backed out a static_cast that windows requires to successfully compile nsCacheService.cpp, so the first try push failed on windows)
Comment on attachment 730295 [details] [diff] [review]
Part 2 - make max smart size 350MB for all profiles

Review of attachment 730295 [details] [diff] [review]:
-----------------------------------------------------------------

meh.

This is going to cause some kind of startup I/O annoyance for 7% of release users, right? (aiui that's why it was deferred in the first place when it would have effected 100% of users).

Is there a compelling need to do this right now? Won't the new cache work highly likely invalidate all caches anyhow and this code path can go away then? Or is it really getting in the way of just doing that work?
Attachment #730295 - Flags: superreview?(mcmanus)
Attachment #730292 - Flags: review?(michal.novotny)
Attachment #730295 - Flags: review?(michal.novotny)
(In reply to Patrick McManus [:mcmanus] from comment #5)
> meh.

Tell me how you really feel ;)

> This is going to cause some kind of startup I/O annoyance for 7% of release
> users, right? (aiui that's why it was deferred in the first place when it
> would have effected 100% of users).

Yes (though looking at telemetry for the last week versus the last month, it's down to 6% for release, so the number of users affected is definitely dropping, who knows how low the affected percentage would get by the time this hit release if we landed it).

The original idea was to leave this in until Firefox 20 or so, and just bite the bullet then (unless there was some unbelievably low number of people on the new max smart size). Not that that's a good excuse for doing this now as opposed to waiting, but it came up on my dashboard, I had some cycles, and I decided to make the patches.

> Is there a compelling need to do this right now? Won't the new cache work
> highly likely invalidate all caches anyhow and this code path can go away
> then? Or is it really getting in the way of just doing that work?

If the new cache work doesn't invalidate all caches, I'll be highly surprised, so yes, we can wait. My only (very weak) argument is that we don't know exactly when we'll have a new cache ready (and I'm not even sure I'm using it as an argument). I'm fine with not landing this, like I said, I just had the cycles, and wanted to get it out there, so I didn't feel bad about this bug being on my plate for 8 months with nothing to show for it :)
WONTFIXing this based on comment 5 and comment 6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: