559942 - Heuristic to pick default HTTP cache size

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect)

Description

[Jason Duell]

So right now the HTTP cache defaults to 50MB. We've got a patch (bug 193911) to double it to 100 MB.  I'm in favor of landing that ASAP, but then we should think about doing something a little smarter, based on how much free disk space the target machine has available.

IE is apparently using up to 1 GB of space for the cache now.  Chrome uses up to 2 GB. (see http://code.google.com/p/chromium/issues/detail?id=40079 ).  We should do something similar.

Comment 1

[Steve Chapel]

According to my notes (see bug 193911 comment 24), I've seen disk cache size level off far below 1 GB, although this may be in large part due to a limit of 8192 items in the disk cache. We should test again after the limit of items in the disk cache is raised and set the limit of disk cache size to be somewhat larger than the maximum observed disk cache size to set a reasonable upper limit of disk cache size.

Comment 3

[Byron Milligan [:byronm]]

I just looked at Chrome's source. The code can be found here: 

http://codereview.chromium.org/1566021/diff/1/2

Their dynamic cache sizing scheme works as follows: 

Let _default_ be the default size of the cache, which they set to be 80Mb, and _available_ be the amount of space available on the user's hard drive. Let _actual_ be the size that the cache will actually be set to, and let _max_ be the largest value held by a 32 bit integer. Then _actual_ is determined as follows: 

1) If _default_ > 80% of _available_, _actual_ = 80% of _available_
2) If _default_ is between 10% and 80% of _available_, _actual_ = _default_
3) If _default_ is between 4% and 10% of _available_, _actual_ = 10% of 
   _available_
4) If _default_ is between .4% and 4% of _available_, _actual_ = 2.5 * _default_
5) If 1% of _available_ is < _max_, _actual_ =  1% of _available_.
6) Otherwise, 1% of _available_ >= _max_, so let _actual_ = _max_


So the basic idea seems to be to try to use the default, suggested size of 80Mb, but scale back a little if the user's available space is limited, and grow quite a bit if the user's available space is huge. 

Growing quite a bit seems appropriate, but I think that Chrome is being too aggressive when the user's available space is limited. Consider that if the user has less than 80Mb of free space, Chrome will occupy 80% of their free space with the cache. This seems invasive to me. What do others think?

I will try to gather some metrics in response to Comment 1. The other thing that I need to figure out is how often to check the cache size against the available space. One option would be to do this every time the disk cache is initialized, but that may be inadequate for users who never close Firefox. I will look into Chrome's source more thoroughly to see what their behavior is.

Comment 4

[Boris Zbarsky [:bzbarsky]]

If a user has 80MB free on their disk nowadays, they're basically out of space....  I'm not sure there's a "good" behavior at that point.

The only way I can see of commonly getting into that situation is quota'ed accounts, but the free-space check wouldn't detect a quota anyway.

Comment 5

[Byron Milligan [:byronm]]

(In reply to comment #4)
> If a user has 80MB free on their disk nowadays, they're basically out of
> space....  I'm not sure there's a "good" behavior at that point.
> 
> The only way I can see of commonly getting into that situation is quota'ed
> accounts, but the free-space check wouldn't detect a quota anyway.

I agree 100%. The most common quota'ed accounts I know of comes up on universities. Stanford quoatas just got bumped up to 2 gigs. But I know you already discussed this on another cache-size thread, so I won't make us repeat whats already been written. Since there is no way to check quotas, it seems that we have to rely on system admins and smart users.

That said, while there may be no "good" behavior, taking up the very limited resources the user has left seems unfortunate... Could we not at least scale it back to 40%, or something like that? I think this is a detail, and does not have to block the rest of the progress on this bug.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah, fair.  I guess my point was that it doesn't seem to matter much what we do in that situation since it will almost never happen, so being more conservative is probably fine.

Comment 7

[Byron Milligan [:byronm]]

Agreed. I'll see what I can come up with. And I thought about it more--you're right. The user is kind of in trouble if they are in that situation anyways. Thanks for the help.

Comment 8

[Jason Duell]

> If a user has 80MB free on their disk nowadays, they're basically out of
space.

What about fennec devices?  They may not have lots of disk space, and it may not be kosher to grab the majority of their free space.  cc'ing dougt in case he knows.

Comment 9

[Byron Milligan [:byronm]]

Sounds good. I'm happy to play it safe when the user is low on space anyways.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree that scaling back to 40% or so seems good when low on space. Though I wonder if we also should have a bare-minimum that we'd use, say of 50MB. If the user is running low on disk space it might be better to make them deal with that rather than have their browsing slow to a crawl due to lack of cache.

Comment 11

[Byron Milligan [:byronm]]

Attachment: Determines default cache size based on disk space available.

Here is a first draft at a patch. There are still a number of issues. I need to test the patch (push to try, run it on different disk configurations, etc.), but it at least works correctly on my machine. 

Currently, there is no logic to check if the user has explicitly set the size of their cache. Which means that if the user has done this, on the next startup, we will overwrite their value with our dynamically determined "default" one. One possible option is to add a "browser.cache.disk.user_specified" preference, which can serve as preserved state across runs, informing us if the user has set the size of the cache themselves. In that case, we probably should not mess with it.

I chose to take Jonas' suggestion of making a bare minimum size of 50MB, and I capped the size to 1GB. I am in the middle of running some tests to see if the suggestion in Comment 1 that we will not fill a 1GB cache due to the entry limit is true. According to a comment in the Chromium source, a 240MB cache corresponds to under 50,000 cache entries for most users... so we can keep that as a reference point.

I'm open to any suggestions and ideas; thanks!

Comment 12

[Byron Milligan [:byronm]]

Attachment: smart_size_v2

This second draft adds logic to not dynamically size the cache based on disk space available if the user has explicitly set their cache to a certain value. This functionality will work regardless of whether the user has done this before or after the patch has landed. 

Performance testing is up next.

Comment 13

[Byron Milligan [:byronm]]

(In reply to comment #1)
> According to my notes (see bug 193911 comment 24), I've seen disk cache size
> level off far below 1 GB, although this may be in large part due to a limit of
> 8192 items in the disk cache. We should test again after the limit of items in
> the disk cache is raised and set the limit of disk cache size to be somewhat
> larger than the maximum observed disk cache size to set a reasonable upper
> limit of disk cache size.

I set my cache to 1GB, and was able to fill it to capacity. It seems that setting the max number of allowed entries to be determined dynamically based on cache size has removed the limitation that you saw.

Jason, you had expressed concerns that deleting a large cache might lock up the browser. I tried clearing the cache (through the UI) when it was filled up to 1GB of capacity. This was on OSX. It got the spinning-beach ball, but the browser only locked up for a very brief period of time. So I think we are ok there. If you want me to try doing this on Windows as well, let me know.

Finally, the diskSpaceAvailable call that my patch uses seems ok in terms of performance as well. The call takes around .2 milliseconds on Linux, and .01 on Mac. I am sure that there is a lot of noise and variation in these numbers, but they are small enough that I don't think they cause alarm. Please let me know if you think otherwise.

Comment 14

[Byron Milligan [:byronm]]

Attachment: smart_size_v3

I updated the patch so that we only need to introduce one new preference, instead of two. This was a suggestion made by Jonas, and it worked out quite nicely because the logic is significantly cleaner. 

The patch still works on the premise that if the user explicitly sets their cache size, we want to honor that (regardless of whether the user has done so before or after the patch lands). We can, however, introduce smart-sizing as a new default, and disregard what the user has expressed as their preference in the past. I'm waiting to hear back from Beltzner on this issue.

I think the final piece of work here is to consider how often we want to be running this smart sizing algorithm, and if it is ok to do it upon initialization of the cache, as opposed to more lazily. Thanks.

Comment 15

[Byron Milligan [:byronm]]

Attachment: Screen Shot of Advanced.Network Preference Window

So, it turns out its not too late to add UI features for FX4, so we decided to create one letting the user specify if they would like Firefox to manage the size of their cache for them. Here is the basic layout of how the new UI option will work. 

Details: 
*If the user has explicitly set their profile in the past, then the "Smart Size" option will default to being unchecked.
*If "Smart Size" is unchecked, then we unghost the sub-option, allowing the user to explicitly size their cache.
*Clear Cache will empty the cache regardless of which option the user has taken.
*Limi, jst and I have talked about adding how much space the cache is actually taking up right before the Clear Cache button, but I am currently battling getting it to fit/making the layout look nice. I think this part is secondary to getting everything else done.

Everyone should please feel free to jump in and let me know what they think. I am writing the front end code for this/wiring it up with the cache code, and will have that posted soon.

Thanks!

Comment 16

[Jason Duell]

Comment on attachment 468286 [details] [diff] [review]
smart_size_v3

The function for picking cache size seems fine to me for the most part, at least at a high level (maybe make the transition from 625 MB to 1 GB smoother?  Not a big deal).

We are going to need to perf test how our cache performs at large size--we may start to run into issues with storing all of our cache files in the same directory.  We have anecdotal info from the Squid cache developers (who use a tree with 256 files max per directory) that performance on some OSes/filesystems degrades if more than 2000 files exist in a single directory:  we've been shipping with 8K for a long time, and our platforms of interest may not be as affected (and/or as long as caching is still a win vs. hitting the network, we may not care about minor to moderate degradation in the short term), but we'll want to check. Let's land this and test in betas and as a followup bug.

> If you want me to try doing this on Windows as well, let me know.

We still have a few Windows users last I checked, and it uses different filesystems.  So yes, please try clearing a 1 GB cache and time the diskSpaceAvailable call, ideally on both FAT32 and NTFS.  Again, this can be followup work.


>+    //XXX: I think mDiskCacheParentDirectory should be non-null by this point.

It'd be nice to know for sure.  Michael, are you familiar with this code?  At least add an assert if you're not sure.

Comment 17

[Byron Milligan [:byronm]]

(In reply to comment #16)
> Comment on attachment 468286 [details] [diff] [review]
> smart_size_v3
> 
> The function for picking cache size seems fine to me for the most part, at
> least at a high level (maybe make the transition from 625 MB to 1 GB smoother? 
> Not a big deal).
  
  I added another tier--800MB--in between the tiers of 625MB and 1GB.

> Let's land this and test in betas and as a followup bug.
  
  Sounds great. I can file a bug on potential performance regressions as the cache size grows, and hang it off   
  the metabug ( bug 559729 ).

> We still have a few Windows users last I checked, and it uses different
> filesystems.  So yes, please try clearing a 1 GB cache and time the
> diskSpaceAvailable call, ideally on both FAT32 and NTFS.  Again, this can be
> followup work.
  
  Right you are. I will include this in the new bug as well. I did time the diskSpaceAvailable call on Windows 
  as well, (see Comment 13 for OSX and Linux) and it took under a millisecond on Windows.
 
> >+    //XXX: I think mDiskCacheParentDirectory should be non-null by this point.
> 
> It'd be nice to know for sure.  Michael, are you familiar with this code?  At
> least add an assert if you're not sure.

  I actually traced through the code paths in nsCacheProfilePrefObserver::ReadPrefs, and it looks like the profile *can* still be 
  null by that point. Though semantically, I am still not sure what all of the checks for the profile in the function ReadPrefs are 
  doing, so Michal, your input would still be valuable. I put an assertion with a comment in the patch for now.

I will put the patch with new comments/issues in the next post.

Comment 18

[Byron Milligan [:byronm]]

Attachment: Adds UI feature enabling user to turn smart sizing on/off.

Comment 15 pretty much explains this patch. There are a couple of loos-ends that remain, despite my flagging this for review: 

1) Did not add the "Actual Cache Size" value to the left of the Clear Cache button. It felt very cluttered with it. If there are strong opinions for it, or suggestions how to make it fit, let me know.

2)Not guaranteed that a profile exists by the end of ReadPrefs. I rely on there being a profile directory though. Should I just return an error code? There is a comment to this end in the patch.

3)As mentioned earlier, we honor if the user has set their cache size before introducing this feature. However, if they have not set their cache size prior to introduction, then smart-sizing defaults to on. Currnetly, the ghosted-out sub-option will show the default disk.capacity of 250MB. I left it be, but considered changing disk.capacity to instead be equal to whatever we first determine disk.smart_size to be. Thoughts? Its probably not too crucial.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 468642 [details] [diff] [review]
Adds UI feature enabling user to turn smart sizing on/off.

>--- a/browser/locales/en-US/chrome/browser/preferences/advanced.dtd
>+++ b/browser/locales/en-US/chrome/browser/preferences/advanced.dtd

>+<!ENTITY smartSizeCache.label            "Allow Firefox to manage the size of my cache.">

s/Firefox/&brandShortName;/

>--- a/modules/libpref/src/init/all.js
>+++ b/modules/libpref/src/init/all.js

>+//Indicates if disk cache is enabled at all.
> pref("browser.cache.disk.enable",           true);

Useless comment for a 100% self-explanatory pref name? :)

>+//Indicates if this is the first time Firefox is being run w/ smartsizing
>+//available
>+pref("browser.cache.disk.smart_size.first_run", true);
>+//Indicates if the user would like Firefox to manage their cache size for them.
>+pref("browser.cache.disk.smart_size.enabled", true);
>+//Dynamic size for the cache. Determined by Firefox based on disk space available.
>+pref("browser.cache.disk.smart_size",       256000); //default is not used
>+//Size explicitly set by the user. Only effective when smart_size.enabled==false

You're in Gecko land here, all the references to Firefox are bogus.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Dao's last comment means that you can't just drop "Firefox" into those strings; check the adjescent strings for a guide as to how.

FWIW, this looks good to me, but since you chatted with Limi about the design, I'll let him mark it.

Comment 21

[Byron Milligan [:byronm]]

(In reply to comment #19)
> Comment on attachment 468642 [details] [diff] [review]
> Adds UI feature enabling user to turn smart sizing on/off.
> 
> >+<!ENTITY smartSizeCache.label            "Allow Firefox to manage the size of my cache.">
> 
> s/Firefox/&brandShortName;/

Fixed it.  
 
> >+//Indicates if disk cache is enabled at all.
> > pref("browser.cache.disk.enable",           true);
> 
> Useless comment for a 100% self-explanatory pref name? :)

Removed it.

> You're in Gecko land here, all the references to Firefox are bogus.

I removed the Firefox references from the comments.

(In reply to comment #20)
> FWIW, this looks good to me, but since you chatted with Limi about the design,
> I'll let him mark it.

Sounds great. Thanks!

Comment 22

[Byron Milligan [:byronm]]

Attachment: smart_size_v5

Addresses UI string issues and removes printf's.

Comment 23

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 468733 [details] [diff] [review]
smart_size_v5

Looks good to me. I'd make a slight wording change:

"Allow &brandShortName; to manage the size of my cache."

feels better as:

"Let &brandShortName; to manage the size of my cache."

…since there is a checkbox involved, and unchecking it will make the size adjustment active.

Not worried about the cache size indicator missing, that's for the "particularly interested". ;)

Comment 24

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 468549 [details]
Screen Shot of Advanced.Network Preference Window

For great justice! (additional comments in the posted atch)

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #23)
> "Let &brandShortName; to manage the size of my cache."

Shouldn't that be

"Let &brandShortName; manage the size of my cache."

Comment 26

[Jason Duell]

Comment on attachment 468733 [details] [diff] [review]
smart_size_v5

Minor UI issue: the current label for clearing the cache is "Clear Now": it
seems like it would be improved by being "Clear Cache."

> Currently, the ghosted-out sub-option will show the default disk.capacity of
> 250MB. I left it be, but considered changing disk.capacity to instead be equal
> to whatever we first determine disk.smart_size to be

Let's always have the cache size box show the actual max cache size that's being
used, whether it's the smart size (greyed out), or the user-specified size.
This should just be a matter of setting DISK_CACHE_CAPACITY_PREF to the
smart-size if we use it.


>+  letUserSetSize: function (smartSizeEnabled)
>+  {
>+      document.getElementById("useCacheBefore").disabled = smartSizeEnabled;
>+      document.getElementById("cacheSize").disabled = smartSizeEnabled;
>+      document.getElementById("useCacheAfter").disabled = smartSizeEnabled;
>+  },

Use 2-space indents.

>+               <!--Clears the cache, regardless of whether the user has
>+               smart-sizing turned on"-->

Change comment to just "Clears the cache"

>+             <!--Sub-option which allows user to explicitly set their cache
>+             size. Ghosted when smart-size is on, unghosted when smart-size is
>+             off-->

Get rid of 2nd half of last sentence (from "unghosted...")

>+//Is this the first-time smartsizing has been introduced?
>+pref("browser.cache.disk.smart_size.first_run", true);
>+//Does the user want smart-sizing?
>+pref("browser.cache.disk.smart_size.enabled", true);
>+//Dynamic size for the cache, based on disk space available.
>+pref("browser.cache.disk.smart_size",       256000); //default is not used
>+//Size explicitly set by the user. Only effective when smart_size.enabled==false

Here and elsewhere, put a space between // and start of comment.

> private:
>+    PRBool                  PermittedToSmartSize(nsIPrefBranch*);

Use bool for new boolean vals, unless it's used in an IDL interface, which this
is not.

>+            if (NS_FAILED(rv)) return rv;
>+            PRInt32 newCapacity = 0;
>+            if(smartSizeEnabled) {//smart sizing was turned on

nit: here and elsewhere, add space between if and opening brace.  Ditch useless
comment.

>+              //Since the user just switched smart sizing on, recalculate the
>+              //capacity.

I'm a fan of squeezing comments into one line if possible, ex:

 // smart sizing switched on: recalculate capacity

>+              newCapacity = GetSmartCacheSize();//returns capacity in bytes

Lose the comment about capacity in bytes.

>+              rv = branch->SetIntPref(DISK_CACHE_SMART_SIZE_PREF, newCapacity);
>+            }else{//smart sizing was turned off

Here and elsewhere, put spaces between "} else {". Ditch useless comment.

>+              //Use the user specified/default capacity, since smart sizing was
>+              //just turned off.

 // smart sizing switched on: use user-specified size

>+  const PRInt32 MAX_SIZE = 1024 * 1024 * 1024;
>+  /* Get a handle on the disk the cache directory lives on so we can determine
>+   * how much free space the user has available.*/

prefer // comments to /* */:

    // Get handle to disk where cache lives, so we can check for free space

>+  /* If this code is executing with smart-sizing available as an option for the
>+   * first time, then we need to be careful. We only want to allow smart-sizing
>+   * as the new default if the user has not explicitly set the size of their
>+   * cache in the past. (They can of course still turn it on via the UI).
>+   */

  // If user has explicitly set cache size, do not use smart-sizing by default.

>+  if(firstRun) {
>+    //No longer first run.
>+    (void) branch->SetBoolPref(DISK_CACHE_SMART_SIZE_FIRST_RUN_PREF, PR_FALSE);
>+    PRBool userSet;
>+    (void) branch->PrefHasUserValue(DISK_CACHE_CAPACITY_PREF, &userSet);
>+    //check if user has set cache size in the past

move comment above declaration of userSet.  

>+    if(userSet) {
>+      (void) branch->SetBoolPref(DISK_CACHE_SMART_SIZE_ENABLED_PREF, PR_FALSE);

Remove all the "(void)" casts of function results. Yes, it's the convention in
the file.  But it's stupid.  Let's not add any new ones.

>+    }else{

spaces around 'else'

>+      //XXX:Do we want to set the default disk.capacity size to be whatever we
>+      //smartsize it to? Or just leave it at 250MB?

Yes, make it the smart size.  Note in a comment

>         // use file cache in build tree only if asked, to avoid cache dir litter
>         if (!directory && PR_GetEnv("NECKO_DEV_ENABLE_DISK_CACHE")) {
>             rv = NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR,
>                                         getter_AddRefs(directory));
>         }
>         if (directory)
>             mDiskCacheParentDirectory = do_QueryInterface(directory, &rv);
>     }
>+    //XXX:This assertion may fail... what should we do? if(!profile) return err?
>+    NS_ASSERTION(mDiskCacheParentDirectory, 
>+        "mDiskCacehParentDirectory is null in nsCacheProfilePrefObserver::ReadPrefs");

typo in msg.

But I don't think we want an assert here after all.   We will not have a
mDiskCacheParentDirectory if we're running in a build tree, unless
NECKO_DEV_ENABLE_DISK_CACHE is set (or we call do_get_profile() as described in
bug 584283), which means this will be the common case for developers.  But this
simply means that DiskCacheEnabled() will return false, and we're not using disk
caching at all.  So skip checking for smart size if !mDiskCacheParentDirectory.

+r with those changes.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 468733 [details] [diff] [review]
smart_size_v5

>diff --git a/browser/components/preferences/advanced.js b/browser/components/preferences/advanced.js

>+  /* Helper function that ghosts/unghosts the option allowing the user to
>+   * explicitly size their cache. Ghosts if _smartSizeEnabled_==true, unghosts
>+   * otherwise. 
>+   */

Use disables/enables rather than ghosts/unghosts (elsewhere in the patch too) - but I'd just remove this comment, it's pretty obvious what the code does.

>+  /* Called when the Network tab is loaded. 
>+   * Reads the browser.cache.disk.smart_size.enabled preference, and defaults
>+   * the "Let firefox manage my cache" checkbox to checked if enabled == true, 
>+   * not checked if it equals false.
>+   */

This one too, since it isn't quite accurate (the method is actually called when the pref is read, which can happen more than once if e.g. it changes while the dialog is open), and the purpose of the method is clear enough. Or at least shorten to "Disable the cache size UI if smart-size cache is enabled."

>+  readSmartSizeEnabled: function ()

>+    var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                          .getService(Components.interfaces.nsIPrefBranch);
>+    var enabled = prefs.getBoolPref("browser.cache.disk.smart_size.enabled");

Use document.getElementById("browser.cache.disk.smart_size.enabled").value;

>+    this.letUserSetSize(enabled);
>+    return enabled;

No need to return this value (the default behavior is to the use the pref value).

>+  /* Event handler for when the user checks or unchecks the "Smart-Size" box.
>+   * We write this change to the smart_size.enabled preference, and
>+   * ghost/unghost the suboption accordingly. 

I would remove this too.

>+  writeSmartSizeEnabled: function ()
>+  {
>+    var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                          .getService(Components.interfaces.nsIPrefBranch);

this is unused.

>diff --git a/browser/components/preferences/advanced.xul b/browser/components/preferences/advanced.xul

>+      <preference id="browser.cache.disk.smart_size"   name="browser.cache.disk.smart_size"   type="int"/>

This seems to be unused?

>+               <!--Check box indicating if user would like firefox to manage
>+               their cache size for them, based on disk space available -->

>+               <!--Clears the cache, regardless of whether the user has
>+               smart-sizing turned on"-->

>+             <!--Sub-option which allows user to explicitly set their cache
>+             size. Ghosted when smart-size is on, unghosted when smart-size is
>+             off-->

I'd remove these comments in favor of a more general comment about the purpose of the pref around line 175 of advanced.js (as with the other prefs in that file).

>+             <hbox  align="center" class="indent">
>+               <label   id="useCacheBefore" control="cacheSize"
>+                        accesskey="&useCacheBefore.accesskey;" value="&useCacheBefore.label;"/>

nit: spacing adter <label/<hbox is off.

>-               <label id="useCacheAfter" flex="1">&useCacheAfter.label;</label>
>+               <label id="useCacheAfter" flex="1" >&useCacheAfter.label;</label>

>+	           <hbox align="center">
>+	             <checkbox id="offlineNotify" flex="1"
>+			                   label="&offlineNotify.label;" accesskey="&offlineNotify.accesskey;"
>+			                   preference="browser.offline-apps.notify"
>+			                   onsyncfrompreference="return gAdvancedPane.readOfflineNotify();"/>
>+	             <button id="offlineNotifyExceptions"
>+		                   label="&offlineNotifyExceptions.label;"
>+		                   accesskey="&offlineNotifyExceptions.accesskey;"
>+		                   oncommand="gAdvancedPane.showOfflineExceptions();"/>
>+	           </hbox>
>+	           <hbox>
>+	             <vbox flex="1">

Need to fix these whitespace issues too (unnecessary introduction of tabs/extra spaces).

I noticed a lot of "if(" and such in the rest of the patch, too - OCD in me can't help but comment :)

Comment 28

[Byron Milligan [:byronm]]

Attachment: smart_size_v6

Incorporates review comments and feedback.

Comment 29

[Byron Milligan [:byronm]]

Attachment: smart_size_v6

Now *actually* fixes whitespace/tab issues, I hope.

Comment 30

[Byron Milligan [:byronm]]

(In reply to comment #27)
> Comment on attachment 468733 [details] [diff] [review]
> smart_size_v5

> >+  writeSmartSizeEnabled: function ()
> >+  {
> >+    var prefs = Components.classes["@mozilla.org/preferences-service;1"]
> >+                          .getService(Components.interfaces.nsIPrefBranch);
> 
> this is unused.
> 

I actually removed this entire function, and removed the onsynctopreference attribute from the element in advanced.xul. I guess every time the preference is updated, onsyncfrompreference fires?
Because even without this function, the sub-option disables itself when the smart_size.enabled preference is written to.

> >diff --git a/browser/components/preferences/advanced.xul b/browser/components/preferences/advanced.xul
> 
> >+      <preference id="browser.cache.disk.smart_size"   name="browser.cache.disk.smart_size"   type="int"/>
> 
> This seems to be unused?
> 

You are right. I had meant to remove that; its gone now.

Thank you for the feedback; I hope I properly addressed it all. Please let me know if you still see anything you don't like.

Comment 31

[Byron Milligan [:byronm]]

Attachment: smart_size_v6

Included Limi's feedback on the text in the options window.

Comment 32

[Byron Milligan [:byronm]]

(In reply to comment #26)
> Comment on attachment 468733 [details] [diff] [review]
> smart_size_v5
> 
> Let's always have the cache size box show the actual max cache size that's
> being
> used, whether it's the smart size (greyed out), or the user-specified size.
> This should just be a matter of setting DISK_CACHE_CAPACITY_PREF to the
> smart-size if we use it.

Limi had suggested that we not overload the size box, and that it always retain the size explicitly set by the user. Thoughts? Note that if smart-sizing is turned on the first time the feature is introduced, cache.disk.capacity (and therefor this text box) are both set to the smart-size value.

Comment 33

[Byron Milligan [:byronm]]

Attachment: smart_size_v6

Changed "Clear Now" to "Clear Cache". I think this finally incorporates all of the feedback; sorry for all the spam.

Comment 34

[Byron Milligan [:byronm]]

Attachment: smart_size_v6

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 468983 [details] [diff] [review]
smart_size_v6

>-<!ENTITY clearCacheNow.label             "Clear Now">
>+<!ENTITY clearCacheNow.label             "Clear Cache">

You need to change the entity name for this. This needs ui-review too.

Comment 36

[Johnny Stenback (:jst)]

(In reply to comment #35)
> Comment on attachment 468983 [details] [diff] [review]
> smart_size_v6
> 
> >-<!ENTITY clearCacheNow.label             "Clear Now">
> >+<!ENTITY clearCacheNow.label             "Clear Cache">
> 
> You need to change the entity name for this. This needs ui-review too.

This might be a stupid question, but why would the entity name need to change? Isn't that just more work for our localizers?

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

It's the way we signal to localizers that the string has changed. Yay, hacky-tools!

Comment 38

[Jason Duell]

Comment on attachment 468983 [details] [diff] [review]
smart_size_v6

> Limi had suggested that we not overload the size box, and that it always retain the size explicitly set by the user. Thoughts?

My thought is that I disagree with him :)  Most users won't alternate between a custom setting and smart-sizing with any frequency.  (Unlike, say, proxy settings, where it makes a lot of sense to keep the user's previous settings, because if they re-enable proxying, they are likely to want exactly those settings again).  I think it's more intuitive and informative to set and keep the smart size in the box whenever smart-sizing is turned on.  That way if the user is running out of disk space or something, and get here to shrink their cache, they'll have some idea of how much is in use. (Admittedly we're only providing the max size, not the actual size: but they will usually be the same after enough browsing).

I also don't care enough about this UI issue (or the "Clear Cache" button wording) to block this patch from landing.  

I think this patch is ready as far as necko review goes:  other than the UI issues, I'd like to see gavin or someone else who understands prefs better than me sign off on those changes.

You've still got a bunch of // comments w/o leading space.

Comment 39

[Jason Duell]

cc-ing Limi to make sure he doesn't miss the discussion.

Comment 40

[Dão Gottwald [:dao]]

Comment on attachment 468983 [details] [diff] [review]
smart_size_v6

(In reply to comment #38)
> I also don't care enough about this UI issue (or the "Clear Cache" button
> wording) to block this patch from landing.  

There are two options, don't touch the string or change the entity.

Comment 41

[Alex Limi (:limi) — Firefox UX Team]

(In reply to comment #38)
> > Limi had suggested that we not overload the size box, and that it always retain the size explicitly set by the user. Thoughts?
> 
> My thought is that I disagree with him :)  Most users won't alternate between a
> custom setting and smart-sizing with any frequency.  (Unlike, say, proxy
> settings, where it makes a lot of sense to keep the user's previous settings,
> because if they re-enable proxying, they are likely to want exactly those
> settings again). 

Making something both an input and an indicator is usually something we want to avoid in cases like this. Especially since we gray out the box, it can't be read as indicating anything about something else in the UI.

I originally recommended a dedicated space for this — either an indicator that said how much space was in use, since this is different from the upper bound, or putting it in the button label so you'd have "Clear cache (83MB)". Repurposing an text input control to be an indicator when certain checkboxes are checked is not really an option. :)

Comment 42

[Jason Duell]

> Repurposing an text input control to be an indicator when certain checkboxes
are checked is not really an option.

nit:  the box would have always been an indicator, regardless of which boxes were checked (i.e. it would have always displayed the max cache size in use).

As my use of the past conditional suggests, I consider myself to have lost this debate.  Bryon, feel free to revert the "Clear Cache" wording (unless we want that: but it's also not my call), and ask Gavin to +r the prefs bits.

Getting close!

Comment 43

[Byron Milligan [:byronm]]

Attachment: smart_size_v7

Keeps "Clear Now" instead of the proposed "Clear Cache" change.
Fixes remaining style issues.
Keeps the UI as it was when Limi approved it. If we do want to still tweak the UI, let's do it in a separate bug.

Comment 44

[Michal Novotny [:michal]]

(In reply to comment #17)
>   I actually traced through the code paths in
> nsCacheProfilePrefObserver::ReadPrefs, and it looks like the profile *can*
> still be 
>   null by that point. Though semantically, I am still not sure what all of the
> checks for the profile in the function ReadPrefs are 
>   doing, so Michal, your input would still be valuable. I put an assertion with
> a comment in the patch for now.

The assertion shouldn't be there since mDiskCacheParentDirectory can still be null (as Jason already said in comment #26). Anyway I don't know exactly in what cases we don't have a profile directory. Btw I didn't find any provider for NS_APP_CACHE_PARENT_DIR. I guess that it is intended for embeddors that want to use the disk cache but don't want to define a profile directory???

Comment 45

[Johnny Stenback (:jst)]

We need this in for the feature freeze, which is now beta6.

Comment 46

[Jason Duell]

Comment on attachment 469214 [details] [diff] [review]
smart_size_v7

Moving review of prefs stuff to dwitte.

Comment 47

[Mike Beltzner [:beltzner, not reading bugmail]]

(But we would rather have it in earlier, or at least any string changes if some are required!)

Comment 48

[Byron Milligan [:byronm]]

(In reply to comment #47)
> (But we would rather have it in earlier, or at least any string changes if some
> are required!)

I think we can get it in soon. The only string change is 
<!ENTITY smartSizeCache.label "Let &brandShortName; manage the size of my cache.">

Comment 49

[[e-mail]]

Comment on attachment 469214 [details] [diff] [review]
smart_size_v7

>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js

>+// Dynamic cache size based on disk space available.
>+pref("browser.cache.disk.smart_size",       256000); // default is not used

I think you want 262,144 here.

>+bool
>+nsCacheProfilePrefObserver::PermittedToSmartSize(nsIPrefBranch* branch, PRBool
>+                                                 firstRun)
>+{
>+  // If user has explicitly set cache size, do not use smart-sizing by default.
>+  if (firstRun) {
>+    // check if user has set cache size in the past
>+    PRBool userSet;
>+    branch->PrefHasUserValue(DISK_CACHE_CAPACITY_PREF, &userSet);
>+    if (userSet) {
>+      branch->SetBoolPref(DISK_CACHE_SMART_SIZE_ENABLED_PREF, PR_FALSE);

I think you can return here, since there's no point in falling through. Also need to check 'rv' of these pref calls.

>+    }
>+  }
>+  PRBool smartSizeEnabled; 
>+  branch->GetBoolPref(DISK_CACHE_SMART_SIZE_ENABLED_PREF,
>+                      &smartSizeEnabled);
>+  return smartSizeEnabled;

This will warn on Windows, since you're forcing an integer value into a bool. 'return !!smartSizeEnabled' instead.

>@@ -451,16 +575,32 @@ nsCacheProfilePrefObserver::ReadPrefs(ns

>+    if (mDiskCacheParentDirectory) {
>+      PRBool firstSmartSizeRun;
>+      branch->GetBoolPref(DISK_CACHE_SMART_SIZE_FIRST_RUN_PREF, &firstSmartSizeRun); 

Need to check 'rv' of these calls (and below).

>+      if (PermittedToSmartSize(branch, firstSmartSizeRun)) {
>+        // This change will be reflected in UI and about:cache
>+        mDiskCacheCapacity = nsCacheProfilePrefObserver::GetSmartCacheSize() /
>+                             1024; // convert to KB.
>+        if (firstSmartSizeRun) {
>+          // It is no longer our first run
>+          branch->SetBoolPref(DISK_CACHE_SMART_SIZE_FIRST_RUN_PREF, PR_FALSE);
>+          // As a starting point, set disk.capacity to equal smart size
>+          branch->SetIntPref(DISK_CACHE_CAPACITY_PREF, mDiskCacheCapacity);
>+        }
>+        branch->SetIntPref(DISK_CACHE_SMART_SIZE_PREF, mDiskCacheCapacity); 
>+      }
>+    }

r=dwitte with fixes.

Comment 50

[[e-mail]]

(In reply to comment #49)
> >+// Dynamic cache size based on disk space available.
> >+pref("browser.cache.disk.smart_size",       256000); // default is not used
> 
> I think you want 262,144 here.

But wait -- if it's not used, why put it in all.js at all? Just handle the failure case of getting the pref (if indeed that's possible, i.e. you don't set it first) accordingly. Or if you do actually require it to be set here, then elaborate on your comment.

Comment 51

[Byron Milligan [:byronm]]

Attachment: smart_size_v8

Incorporates Dwitte's feedback from the previous two comments. 
*Removes unnecessary disk.smart_size preference
*Uses !! when returning PR_Bool where bool is expected
*Adds error checking to preference getters/setters.
*Returns early from PermittedToSmartSize where dwitte suggested

Additionally, I found two other bugs: 
*Integer overflow in the 800MB case of GetSmartCacheSize
*Not dividing the returned value of one of the GetSmartCacheSize() calls by 1024, which lead to an incorrect about:cache.

These are both fixed now. I think this code is bug free now. It looks like anode is ready to go with Bug 590804. Let's decide when we want to land this, so that we can give her a time frame. I think she will perhaps need to schedule Talos down time.

Thanks!

Comment 52

[Byron Milligan [:byronm]]

Attachment: smart_size_async_v1

Because we try to avoid doing any form of I/O on the main thread, I have rewritten this patch to check the user's disk space available on the cache I/O thread instead. The diskSpaceAvailable call seemed to be very quick, so I doubt that this will make a large difference, but better safe than sorry.

This is a bit of a pain, since this patch was already reviewed, but I think its important to make this change. Fortunately, none of the front-end XUL or JS had to be modified. The cache code has been changed, though it leverages pieces from the previously approved patch.

In order to avoid introducing a lot of synchronization, my update uses the strategy of dispatching a runnable to the cache I/O thread, which retrieves the disk space available, and then sends a runnable back to the main thread. This runnable that is sent back to the main thread does the proper setting of cache capacity variables. 

With regards to landing: I do not know if we prefer to land the already r+'d version, and land this later, or to just review and land this new version. It looks like the necessary Talos changes will be made Thursday morning (see Bug 591055), so we should be safe to land any time after that.

Also, there are likely style issues that I will fix shortly.

Comment 53

[[e-mail]]

Can you post this as a patch on top of your previous one, so we can just look at your changes without reading everything again? (mq is your friend here!)

Comment 54

[Byron Milligan [:byronm]]

Attachment: smart_size_async_v1

Refactored the patch so that it only contains changes made from the already approved, synchronous version.

Comment 55

[[e-mail]]

Comment on attachment 470943 [details] [diff] [review]
smart_size_async_v1

>diff --git a/netwerk/cache/nsCacheService.cpp b/netwerk/cache/nsCacheService.cpp

>+    static PRUint32         GetSmartCacheSize(void);

Reindent that to the above lines?

>+
> private:
>     bool                    PermittedToSmartSize(nsIPrefBranch*, PRBool firstRun);
>-    PRUint32                GetSmartCacheSize(void);
>-
> private:

Remove that second 'private' while you're in there.

>+class nsSetSmartSizeEvent: public nsRunnable 
>+{
>+public:
>+nsSetSmartSizeEvent(bool firstRun, PRInt32 smartSize, 
>+                         nsCOMPtr<nsIPrefBranch> branch,
>+                         nsRefPtr<nsCacheProfilePrefObserver> observer) 

nsIPrefBranch* branch
nsCacheProfilePrefObserver* observer

Otherwise you addref and release passing the arg in, which is unnecessary.

Also, indent everything inside the class? And unindent the second and third line of args a little?

>+NS_IMETHOD Run() 
>+{
>+    if (mFirstRun) { 
>+        mBranch->SetIntPref(DISK_CACHE_CAPACITY_PREF, mSmartSize);     
>+    }
>+    PRBool smartSizeEnabled;
>+    mBranch->GetBoolPref(DISK_CACHE_SMART_SIZE_ENABLED_PREF,
>+                         &smartSizeEnabled);

Failure case?

>+    // ensure smart sizing was not switched back off while event was pending.
>+    if (smartSizeEnabled) {
>+        // must set both fields to be safe
>+        nsCacheService::SetDiskCacheCapacity(PR_MAX(0, mSmartSize));
>+        mObserver->SetDiskCacheCapacity(PR_MAX(0, mSmartSize));

That function already does the PR_MAX, right?

>+// Runnable sent from main thread to cacheIO thread
>+class nsGetSmartSizeEvent: public nsRunnable
>+{
>+public:
>+    nsGetSmartSizeEvent(bool firstRun, nsCOMPtr<nsIPrefBranch> branch,
>+                        nsCacheProfilePrefObserver* observer) 

Same here.

>+    // responsible for computing the new smartsize and dispatching this value
>+    // (plus the code that sets it) back to the main thread.

Hmm, I'm having trouble parsing this. Reword?

>+    NS_IMETHOD Run()
>+    {
>+        mSmartSize = nsCacheProfilePrefObserver::GetSmartCacheSize() / 1024;
>+        nsCOMPtr<nsIRunnable> event = new nsSetSmartSizeEvent(mFirstRun,
>+                                                              mSmartSize,
>+                                                              mBranch, 
>+                                                              mObserver);
>+        NS_DispatchToMainThread(event);
>+        return NS_OK;
>+    }
>+private: 

Add a newline after the }.

>+    bool mFirstRun;
>+    PRInt32 mSmartSize;
>+    nsCOMPtr<nsIPrefBranch> mBranch;
>+    nsRefPtr<nsCacheProfilePrefObserver> mObserver;
>+};
>+
>+
>+NS_IMPL_THREADSAFE_ISUPPORTS1(nsCacheProfilePrefObserver, nsIObserver)
>+
>+

Too much newline. :)

Also, move this back up to where it was, before your new classes?

Also, I'd put the getter above the setter.

>+nsresult
>+nsCacheService::DispatchToCacheIOThread(nsCOMPtr<nsIRunnable> event)
>+{
>+    NS_ASSERTION(gService->mCacheIOThread, "Dispatching to CacheIOThread before it has been\
>+                                            created.");    

When is the cache IO thread created? Is it possible for this to happen after the cacheservice is inited?

>diff --git a/netwerk/cache/nsCacheService.h b/netwerk/cache/nsCacheService.h

>+    static nsresult DispatchToCacheIOThread(nsCOMPtr<nsIRunnable> event);

nsIRunnable* here.

r=dwitte with fixes.

Comment 56

[Jason Duell]

Comment on attachment 470943 [details] [diff] [review]
smart_size_async_v1

So the thing about this patch that I don't understand is whether we need any more synchronization for knowing when the cache size finally is set, or not.  I'm wondering particularly about startup time.  Previously, by the time nsCacheProfilePrefObserver::Install completed, we knew the cache size.  Now we don't necessarily.  From what I can tell, this means the cache's nsDiskCacheDevice has not had SetCapacity called on it yet, and so the cache's disk device will have the default size of 0.   I can't tell if this is a Bad Thing or not--it seems like it might result in any cache entries that are initially created getting doomed immediately in OnDataSizeChange, since there's no space for them?  I don't know the code here well enough to say for sure.  The worst case would be that EvictDiskCacheEntries(mCacheCapacity) gets called, and we wind up evicting the entire existing cache.  But the only way that currently looks possible is if we shutdown() before the smartSize events have run.

Would this all work more sensibly if at init time we started by setting mDiskCacheCapacity to the last known value for the cache size, and then fire off the Runnable events to update the smart size?   (I guess "last known size" would be the smart size pref, unless we're running for the first time, in which case it would be the regular cache size pref).


>diff --git a/netwerk/cache/nsCacheService.cpp b/netwerk/cache/nsCacheService.cpp

Add comment:

// This Runnable event is sent to the main thread after the IO thread calculates available disk space, so that there's no race setting mDiskCacheCapacity.

>+class nsSetSmartSizeEvent: public nsRunnable 
>+{
>+public:
>+nsSetSmartSizeEvent(bool firstRun, PRInt32 smartSize, 
>+                         nsCOMPtr<nsIPrefBranch> branch,
>+                         nsRefPtr<nsCacheProfilePrefObserver> observer) 
>+                   : mFirstRun(firstRun)
>+                   , mSmartSize(smartSize)
>+                   , mBranch(branch)
>+                   , mObserver(observer)
>+{
>+}
>+
>+NS_IMETHOD Run() 
>+{

I believe there's an assert you can add to ensure that we're running on the main thread.

>+NS_IMPL_THREADSAFE_ISUPPORTS1(nsCacheProfilePrefObserver, nsIObserver)

As dwitte mentioned, put the ISUPPORTS line right after the decl of nsCacheProfilePrefObserver (where it used to be, before you added the Runnable decls.

> 
>+// Necessary to ensure consistency, since smart size is calculated async

A little confusing.  The function SetDiskCacheCapacity is not per-se needed: rather the dispatch to the main thread via nsSetSmartSizeEvent is what's "needed" (or, precisely, the mechanism we've decided to use) to ensure consistency).  Ditch this comment.

>+void
>+nsCacheProfilePrefObserver::SetDiskCacheCapacity(PRInt32 capacity)
>+{
>+    mDiskCacheCapacity = PR_MAX(0, capacity);
>+}
>+
>+        rv = branch->GetBoolPref(DISK_CACHE_SMART_SIZE_FIRST_RUN_PREF, &firstSmartSizeRun); 

80 chars max, please.  This isn't the only line that's too big...

>+        if (NS_FAILED(rv)) firstSmartSizeRun = PR_FALSE;
>+        if (PermittedToSmartSize(branch, firstSmartSizeRun)) {
>+            nsCOMPtr<nsIRunnable> event = 
>+                new nsGetSmartSizeEvent(!!firstSmartSizeRun, branch, this);
>+            rv = nsCacheService::DispatchToCacheIOThread(event);
>+            if (NS_FAILED(rv)) mDiskCacheCapacity = BASE_LINE;
>+        }
>+
>         if (firstSmartSizeRun) {
>-          // It is no longer our first run
>-          rv = branch->SetBoolPref(DISK_CACHE_SMART_SIZE_FIRST_RUN_PREF, PR_FALSE);
>-          if (NS_FAILED(rv)) 
>-            NS_WARNING("Failed setting first_run pref in ReadPrefs.");
>+            // It is no longer our first run
>+            rv = branch->SetBoolPref(DISK_CACHE_SMART_SIZE_FIRST_RUN_PREF, PR_FALSE);

Also >80 chars

Comment 57

[Jason Duell]

Comment on attachment 469648 [details] [diff] [review]
smart_size_v8

User-interface re-open:

I'd like to summarize what this patch currently does, because I suspect it's not what any of us want:

1) For 1% of users (those who have previously manually set their cache size), the "Use up to XXXX MB of space for cache" field will be active, with the value they explicitly set.  Yay.

2) For the other 99% of users, "Allow Firefox to manage the size of my cache" will be checked, the cache size field will be greyed out, but will contain a "stale smart size" value.  I.e., if the user has never manually set the value, the first time this patch runs we replace the value with a newly calculated smart size, but then we never update the value again.

#2 doesn't really seem too great.  I doubt it's what limi had in mind. 

Here are some alternative options for the greyed-out box:

1) We leave it blank  (I'm not sure how tricky that is given the code below, which is designed to keep the box in sync with the pref value).

2) We keep the value updated to the current smart size.

3) We set the value to 0.

4) We don't update the value, and just keep the old default pref value (this means the greyed-out box will always say "50 MB" for users who haven't touched the pref.)

I've previously championed #2, and it's still my favorite.  Otherwise I suggest #1 if possible, followed by #3.   I don't think showing a random "50 MB" value is useful UI.

The "stale smart size" option seems particularly poor, given that if a user is running out of disk space and comes here to manually shrink the cache, they are likely to expect that the value in the box reflects the currect cache max (when the smart size algorithm will actually probably have kicked in again, and shrunk the cache to something much smaller).  I.e. while trying to shrink the cache, the user is quite likely to manually specify a size that's actually larger than the one in use.


>diff --git a/browser/components/preferences/advanced.xul b/browser/components/preferences/advanced.xul

>+              <label id="useCacheBefore" control="cacheSize"
>+                     accesskey="&useCacheBefore.accesskey;" value="&useCacheBefore.label;"/>
>+              <textbox id="cacheSize" type="number" size="2" 
>+                       preference="browser.cache.disk.capacity"
>+                       onsyncfrompreference="return gAdvancedPane.readCacheSize();"
>+                       onsynctopreference="return gAdvancedPane.writeCacheSize();"
>+                       aria-labelledby="useCacheBefore cacheSize useCacheAfter"/>
>+              <label id="useCacheAfter" flex="1" >&useCacheAfter.label;</label>

Comment 58

[[e-mail]]

(In reply to comment #56)
> Would this all work more sensibly if at init time we started by setting
> mDiskCacheCapacity to the last known value for the cache size, and then fire
> off the Runnable events to update the smart size?   (I guess "last known size"
> would be the smart size pref, unless we're running for the first time, in which
> case it would be the regular cache size pref).

Agree with Jason here. (I thought that's what you were doing, but if not, that sounds sane to me.)

Comment 59

[Byron Milligan [:byronm]]

(In reply to comment #55)
> Comment on attachment 470943 [details] [diff] [review]
> smart_size_async_v1
> >+nsresult
> >+nsCacheService::DispatchToCacheIOThread(nsCOMPtr<nsIRunnable> event)
> >+{
> >+    NS_ASSERTION(gService->mCacheIOThread, "Dispatching to CacheIOThread before it has been\
> >+                                            created.");    
> 
> When is the cache IO thread created? Is it possible for this to happen after
> the cacheservice is inited?

The cache IO thread is created when the cache service is initialized, which is done before anything in my patch would execute. But, if the creation of that thread fails, all we do is warn. So I wanted to check just in case we had failed to actually create the thread. But an assertion is probably inappropriate. I will just default to not smart-sizing if we do not have the background thread.

(In reply to comment #56)
> Comment on attachment 470943 [details] [diff] [review]
> smart_size_async_v1
> 
> So the thing about this patch that I don't understand is whether we need any
> more synchronization for knowing when the cache size finally is set, or not. 
> I'm wondering particularly about startup time.  Previously, by the time
> nsCacheProfilePrefObserver::Install completed, we knew the cache size.[...]

The problem would be if we called CreateDiskDevice() before we have heard back from the smart size event. This is plausible, since we do not have explicit synchronization. However, when this happens, we will set the cache size to be browser.cache.disk.capacity.  See http://mxr.mozilla.org/mozilla-central/source/netwerk/cache/nsCacheService.cpp#414 (through line #416). 

disk.capacity will be set to either the initial smart size value if the user *never* set the pref themself, or if they have, the size they explicitly set. Its therefor unlikely that we will empty the cache, as the user would have had to set their cache size to zero for that to happen. 

However, you are right in that the current implementation can lead to unnecessary eviction: suppose the user had set their cache to 25 MB, but has smart sizing on. Then we will size the disk cache to 25MB before we hear back from the smart size event, and this will lead to eviction. 

Its easy to fix by reading the previous smart size value, as you suggest, and therefor only shrinking the cache once we hear back from the event (or growing it, or keeping it the same, whatever the case may be). This requires reintroducing the browser.cache.disk.smart_size variable, which I had dropped in a previous version, since it was unneeded. Given the UI debate, and the need for it here, I'll bring it back in order to fix this problem.
 
> >diff --git a/netwerk/cache/nsCacheService.cpp b/netwerk/cache/nsCacheService.cpp
> > 
> >+// Necessary to ensure consistency, since smart size is calculated async
> 
> A little confusing.  The function SetDiskCacheCapacity is not per-se needed:
> rather the dispatch to the main thread via nsSetSmartSizeEvent is what's
> "needed" (or, precisely, the mechanism we've decided to use) to ensure
> consistency).  Ditch this comment.

I think the function is necessary to ensure correctness, but I agree the comment is misleading. The only comment that I actually need is the one I wrote here:

+    if (smartSizeEnabled) {
+        // must set both fields to be safe
+        nsCacheService::SetDiskCacheCapacity(mSmartSize);
+        mObserver->SetDiskCacheCapacity(mSmartSize);
+    }

Both these calls must be made. Like the previous issue, it has to do with us being weakened by relying on the event queues for serialization and not using explicit locking. There are two places we can dispatch a smart size event from. One is upon startup in nsCacheProfilePref::ReadPrefs, and the other is when the smart sizing pref is changed, via the call back nsCacheProfilePref::Observe. In either case (though most likely only in the first) it is possible that the event comes back before or after the disk cache has been created. If it comes back before the disk cache has been created, then calling nsCacheService::SetDiskCacheCapacity() will have no effect. The function will return immediately, since the device does not exist. By setting mObserver::mDiskCacheCapacity, we ensure that when the device is later initialized, we set it to the correct value. If it comes back after the device has been initialized, setting nsCacheProfilePrefObserver::mDiskCacheCapacity will have no effect, but calling nsCacheService::SetDiskCacheCapacity will. 

So I am basically covering my bases by setting both, even though I really need one, since I can't determine which one I need, and setting both is harmless.

But I agree: I will ditch the comment as you suggest, and only rely on the one that I pointed out here.

> Also >80 chars
I have added settings to my editor's config file so that it beeps, shocks me, and colors offending lines in red, so hopefully you will not see any more of this.

This thread is getting very long. Is this natural, or is the implementation too convoluted?

Comment 60

[[e-mail]]

(In reply to comment #59)
> The cache IO thread is created when the cache service is initialized, which is
> done before anything in my patch would execute. But, if the creation of that
> thread fails, all we do is warn.

Is the cache service useful without an IO thread? I'd imagine "not terribly". I'd just change init to fail hard if thread creation fails. (We've got big problems if it does!)

> The problem would be if we called CreateDiskDevice() before we have heard back
> from the smart size event. This is plausible, since we do not have explicit
> synchronization. However, when this happens, we will set the cache size to be
> browser.cache.disk.capacity.

Sounds OK to me.

> However, you are right in that the current implementation can lead to
> unnecessary eviction: suppose the user had set their cache to 25 MB, but has
> smart sizing on. Then we will size the disk cache to 25MB before we hear back
> from the smart size event, and this will lead to eviction. 

I'd fix this by suppressing eviction until we hear back from the thread. Doesn't require changing any prefs, and at worst will result in nothing going into the cache until we do. Correct?

(But if you need it for UI, then sure.)

Comment 61

[Byron Milligan [:byronm]]

(In reply to comment #60)
> (In reply to comment #59)
> > The cache IO thread is created when the cache service is initialized, which is
> > done before anything in my patch would execute. But, if the creation of that
> > thread fails, all we do is warn.
> 
> Is the cache service useful without an IO thread? I'd imagine "not terribly".
> I'd just change init to fail hard if thread creation fails. (We've got big
> problems if it does!)
> 

If we fail to create the IO thread, then we fall back to doing the disk access synchronously. But maybe that still counts as a big problem...

> > However, you are right in that the current implementation can lead to
> > unnecessary eviction: suppose the user had set their cache to 25 MB, but has
> > smart sizing on. Then we will size the disk cache to 25MB before we hear back
> > from the smart size event, and this will lead to eviction. 
> 
> I'd fix this by suppressing eviction until we hear back from the thread.
> Doesn't require changing any prefs, and at worst will result in nothing going
> into the cache until we do. Correct?
> 

Yes, this would work.

> (But if you need it for UI, then sure.)

Ok.

Comment 62

[Jason Duell]

> I'd just change init to fail hard if thread creation fails.

+1

> +        // must set both fields to be safe
> +        nsCacheService::SetDiskCacheCapacity(mSmartSize);
> +        mObserver->SetDiskCacheCapacity(mSmartSize);

OK, so how about

     // also set on observer, in case cache svc not init'd yet.

put it in between the two code lines (above the observer set)

> The problem would be if we called CreateDiskDevice() before we have heard back
> from the smart size event. This is plausible, since we do not have explicit
> synchronization. However, when this happens, we will set the cache size to be
> browser.cache.disk.capacity.  

OK, so I still think the logical thing to do is to use the previous cache size at startup, then wait for the smart logic to get updated.  So change the startup code that reads browser.cache.disk.capacity to instead read 

    (smart_size_enabled && !firstTime)  ? smart_size :
                                          browser.cache.disk.capacity

> I'd fix this by suppressing eviction until we hear back from the thread.

dwitte: that sounds like more work and complexity than simply reading in the previous size.  

> If we fail to create the IO thread, then we fall back to doing the disk access
synchronously. But maybe that still counts as a big problem...

Hmm, good question.  I'm not actually sure of the answer.  It's pretty unlikely to occur.  I'd guess we're probably not going to be functional in general if thread creation is failing on the OS.  But if we can actually proceed with sync I/O in that case, I suppose that's ok?   So warn, not fail.   (I'm very open to other opinions on this issue).

Comment 63

[[e-mail]]

(In reply to comment #62)
> OK, so I still think the logical thing to do is to use the previous cache size
> at startup, then wait for the smart logic to get updated.  So change the
> startup code that reads browser.cache.disk.capacity to instead read 
> 
>     (smart_size_enabled && !firstTime)  ? smart_size :
>                                           browser.cache.disk.capacity
> 
> > I'd fix this by suppressing eviction until we hear back from the thread.
> 
> dwitte: that sounds like more work and complexity than simply reading in the
> previous size.  

Isn't it as simple as an extra boolean and early return in the eviction code? Or is that code sprawled out in some other service? If so, then yeah, previous size sounds reasonable.

> I/O in that case, I suppose that's ok?   So warn, not fail.   (I'm very open to
> other opinions on this issue).

I don't think it's sensible to continue when such basic operations don't work. Just like infallible malloc, we should work toward reducing code complexity (especially in code which is extremely hard to test!) in cases where it makes sense. Our decade-old mantra of "work or fail gracefully in every possible circumstance" is actually detrimental to reliability because of the testing problem, and time spent maintaining code that's irrelevant. If this means we can ditch all the synchronous fallback code (I'm assuming there is some), we should file a followup for that. In the meantime I'd say just make it hard fail.

Comment 64

[Byron Milligan [:byronm]]

Attachment: new_ui

Try this on for (smart) size. 

It seems that the UI issues stem from the fact that the user cannot see how big their cache actually is through this options pane. Limi had suggested we display the actual size of the cache from the start of this bug, so here it finally is. 

This new line will always reflect how much space the cache currently occupies (with rounding, so the user does not get janky numbers). The sub-option will always be the ceiling the user has set for their cache, and we will maintain this value in the box across enabling/disabling of smart sizing. I'm not sure what the default should be here... If the user set their size before this patch, it will obviously be that value. If they have not, we can use a default value, or whatever we initially smart size the cache to, or 0. 

The only piece of info we do not show is the ceiling that smart-sizing has selected, but I think this is ok, because the user is trusting in Firefox to manage this for them anyways, and they can still see the number that really matters in real-time, which is how big their cache actually is.

Comment 65

[Alex Limi (:limi) — Firefox UX Team]

* When no value has been set before, and user opts into manually managing the cache, set the number to the theoretical ceiling (1000 MB, by default, I believe?) — the reason is that we want to reinforce that 1000MB is a reasonable cache value, as opposed to e.g. 50MB. They should know that they are *down-adjusting* their cache by manually setting it to e.g. 50MB.

* Since we're showing the current cache size at all times, it won't be hard for users to understand what the default ceiling is vs. how much is actually used anymore.

Upgrade behavior:

If a value has been set before the upgrade, we should do the following:

* If the user has *reduced* the cache size from the default, they are signaling that they are reducing the cache for disk space reasons, so we should respect their previous value.

* If the user has *increased* their cache size from the default, they are probably acting on outside advice (ie. classic Lifehacker article material) and trying to make their browser faster. In this case, we should set the Smart Size attribute, and let them go back and re-enable manual control if they have good reasons.

* If the user hasn't done any change, default is Smart Size (should be obvious, but including for completeness :)

Comment 66

[Byron Milligan [:byronm]]

Attachment: new_ui_frontend

This patch contains modifications to advanced.xul and advanced.js in order to implement the final UI design laid out by Limi in Comment 65.

Comment 67

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 471661 [details] [diff] [review]
new_ui_frontend

>diff --git a/browser/components/preferences/advanced.js b/browser/components/preferences/advanced.js

>+  updateActualCacheSize: function ()
>+  {
>+    var visitor = {
>+      visitDevice: function (deviceID, deviceInfo) 
>+      {
>+        if (deviceID == "disk") {
>+          var actualSizeLabel = document.getElementById("actualCacheSize");
>+          // Convert and round to nearest MB

You can use DownloadUtils.convertByteUnits for this (i.e. copy the code used for offlineAppUsage).

>+          actualSizeLabel.setAttribute("value", sizeStr);

use |.value =| rather than setAttribute (applies to the change in clearCache too)

>   clearCache: function ()
>   {
>+    // First set actual cache size to zero

Just call updateActualCacheSize after instead? More work, I guess. (Is it guaranteed to return 0?)

>diff --git a/browser/components/preferences/advanced.xul b/browser/components/preferences/advanced.xul

>+            <checkbox preference="browser.cache.disk.smart_size.enabled"
>+                      id="allowSmartSize" align="center" flex="1" 
>+                      onsyncfrompreference="return gAdvancedPane.readSmartSizeEnabled();"
>+                      label="&smartSizeCache.label;" checked="true"/>

Don't include |checked="true"| (onsyncfrompreference will override it anyways). Is |align="center"| really needed?

Looks good otherwise, but this seems to be on top of a previous patch which hasn't been reviewed yet. I'd kind of prefer to review the entirety of the frontend UI changes in one shot.

Comment 68

[Byron Milligan [:byronm]]

Attachment: all_frontend_code

This patch contains all of the modifications involved in implementing the modifications to the UI. Incorporates Gavin's feedback.

Comment 69

[Jason Duell]

Comment on attachment 469648 [details] [diff] [review]
smart_size_v8

I think our UI issues are all resolved for this bug by comment 65.

Comment 70

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 472044 [details] [diff] [review]
all_frontend_code

>diff --git a/browser/components/preferences/advanced.js b/browser/components/preferences/advanced.js

>+   * browser.cache.disk.smart_size
>+   * - the size of the cache dynamically determined base on disk space available

This doesn't really belong here, since it isn't relevant to the prefs UI at all (also should probably be renamed as discussed on IRC).

>+  updateActualCacheSize: function ()

>+          var actualSize = DownloadUtils.convertByteUnits(deviceInfo.totalSize);

nit: name this "sizeStrings" to make it clearer that it's an array of the two strings? Kind of threw me off a bit.

>+  letUserSetSize: function (smartSizeEnabled)

nit: rename this to updateCacheSizeUI?

>+  readSmartSizeEnabled: function ()
>+  {
>+    var enabled = document.getElementById("browser.cache.disk.smart_size.enabled").value;
>+    this.letUserSetSize(enabled);

I was surprised that onsyncfrompreference is called when the pref is set using the associated control - I would have expected you'd need to call letUserSetSize in an onchange handler.

>diff --git a/browser/components/preferences/advanced.xul b/browser/components/preferences/advanced.xul

The alignment in this file is all messed up - it would be nice to clean it up (replace tabs with spaces, etc.), or at least not make it any worse (I noticed some indentation issues).
>             <hbox align="center">
>+              <label id="actualCacheSize" value="" flex="1"/>

omit |value=""|

>-              <label id="useCacheAfter" flex="1">&useCacheAfter.label;</label>
>+              <label id="useCacheAfter" flex="1" >&useCacheAfter.label;</label>

don't make this change :)

>diff --git a/browser/locales/en-US/chrome/browser/preferences/preferences.properties b/browser/locales/en-US/chrome/browser/preferences/preferences.properties

>+#LOCALIZATION NOTE: The next string is for the disk usage of the http cache.
>+#   e.g., "Your cache is currently using 200 MB"
>+#   %1$S = size 
>+actualCacheSize=Your cache is currently using %1$S %2$S of disk space

mention %2$S = unit (MB, KB, etc.)

>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js

>+// Dynamic size based on user's disk space available
>+pref("browser.cache.disk.smart_size", 256000);

This doesn't need a default value, as we discussed on IRC.

r=me with those addressed.

Comment 71

[Byron Milligan [:byronm]]

Attachment: Part 1: Synchronous Smart Sizing

Carrying forward jduell's r+. Ready for checkin.

Comment 72

[Byron Milligan [:byronm]]

Attachment: Part 2: Asynchronous Smart Sizing

Carrying forward dwitte's r+. Ready for checkin.

Comment 73

[Byron Milligan [:byronm]]

Attachment: Part 3: Modifies cache code to support smart sizing UI.

All changes made in this patch were covered in dwitte's review for part 2. Ready for checkin.

Comment 74

[Byron Milligan [:byronm]]

Attachment: Part 4: Modifies preference code to implement smart sizing UI.

Incorporated Gavin's review comments, carrying forward the r+. Ready for checkin.

Comment 75

[:Gavin Sharp [email: [e-mail]]]

"Part 4" still has some trailing whitespace additions that would be good to clean up before check-in.

Comment 76

[Byron Milligan [:byronm]]

Attachment: Part 4: Modifies preference code to implement smart sizing UI.

White-space removal.

Comment 77

[[e-mail]]

(In reply to comment #0)
> IE is apparently using up to 1 GB of space for the cache now.
IE6 used to use a percentage of free space for the cache, so it could easily eat over 1 GB on a large drive. (Which caused bug 501605.)

IE8 defaults to a fixed size cache. They recommend 50-250MB.

Comment 78

[Byron Milligan [:byronm]]

http://hg.mozilla.org/mozilla-central/rev/9b0290865361
http://hg.mozilla.org/mozilla-central/rev/5aee5dd2674b
http://hg.mozilla.org/mozilla-central/rev/21b315161366
http://hg.mozilla.org/mozilla-central/rev/1b34b75baf26

Comment 79

[Vlado Valastiak [:wladow]]

Comment on attachment 473302 [details] [diff] [review]
Part 4: Modifies preference code to implement smart sizing UI.

>diff --git a/browser/locales/en-US/chrome/browser/preferences/advanced.dtd b/browser/locales/en-US/chrome/browser/preferences/advanced.dtd
>--- a/browser/locales/en-US/chrome/browser/preferences/advanced.dtd
>+++ b/browser/locales/en-US/chrome/browser/preferences/advanced.dtd
>@@ -46,16 +46,17 @@
> <!ENTITY useCacheBefore.label            "Use up to">
> <!ENTITY useCacheBefore.accesskey        "U">
> <!ENTITY useCacheAfter.label             "MB of space for the cache">
> <!ENTITY clearCacheNow.label             "Clear Now">
> <!ENTITY clearCacheNow.accesskey         "C">
>+<!ENTITY smartSizeCache.label            "Let &brandShortName; manage the size of my cache.">

This needs an accesskey.

Comment 80

[u88484]

Verified fixed using Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100909 Firefox/4.0b6pre

I seen the pref change values both up and down after adjusting the cache size to use.  Also seen the size of my cache decrease after down sizing how much cache to keep.  Selecting let Firefox control cache size reset the pref back to 256MB

Comment 81

[Mounir Lamouri (:mounir)]

Backed out because of a random crash, see bug 595413

Changesets:
http://hg.mozilla.org/mozilla-central/rev/b05c3fe70af8
http://hg.mozilla.org/mozilla-central/rev/9f11218d6644

And string have been pushed back:
http://hg.mozilla.org/mozilla-central/rev/4fb35dae1333

That should probably not block beta6 anymore...

Comment 82

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #81)
> And string have been pushed back:
> http://hg.mozilla.org/mozilla-central/rev/4fb35dae1333
> 
> That should probably not block beta6 anymore...

I think this is probably still a blocker - hopefully we can get the crash fixed before the new freeze date. I think the strings should be backed out until then, though - there's no need to rush them in now when they can't be tested, and it's easier to keep the patch landable as a single unit.

Comment 83

[Mounir Lamouri (:mounir)]

I backed out the strings following Gavin request:
http://hg.mozilla.org/mozilla-central/rev/5bf4e798d24f

Comment 84

[u60234]

+<!ENTITY smartSizeCache.label  "Let &brandShortName; manage the size of my cache.">

None of the other checkbox labels in the preferences window have a full stop. Should there really be one here?

Comment 85

[Alex Limi (:limi) — Firefox UX Team]

(In reply to comment #84)
> +<!ENTITY smartSizeCache.label  "Let &brandShortName; manage the size of my
> cache.">
> 
> None of the other checkbox labels in the preferences window have a full stop.
> Should there really be one here?

Yeah, good catch. There shouldn't be a period in this label.

Comment 86

[Jason Duell]

Re-landed with fix for crash (bug 595413).

  http://hg.mozilla.org/mozilla-central/rev/765e7c4a715a
  
> "Let &brandShortName; manage the size of my cache.">

I also took out the period from this string, per comments 84/85.

Accesskey and other followup UI stuff in 596714.  Various minor fixes to cache logic in bug 596476.

Thanks folks!

Comment 87

[u88484]

Verified fixed

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100916
Firefox/4.0b7pre