Closed Bug 915296 Opened 9 years ago Closed 9 years ago

HTTP cache v2: Show cache consumption correctly in UI

Categories

(Core :: Networking: Cache, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: mayhemer, Assigned: mayhemer)

References

(Depends on 1 open bug)

Details

(Whiteboard: [cache2])

Attachments

(1 file, 8 obsolete files)

New cache visit and consumption API just provides info on stuff that is currently loaded as entries class representation in memory.  We don't walk the disk at all.

We don't necessarily need to make the entry walking work, just make consumption counting work in some simple way for the Options dialog.
Blocks: 916052
Adding dep on the index, it can make fixing this bug much simpler.
Depends on: 923016
No longer blocks: 916052
Summary: HTTP cache v2: Make visiting the new cache fully work, or at least to get consumption → HTTP cache v2: Show cache consumption correctly in UI
Attached patch v1 (obsolete) — Splinter Review
Ehsan, please review the UI part changes.  I've changed how we show the cache consumption in the preferences/advanced/network pane a bit.  The new cache may (re)build it's index in background and it can be potentially slow (like seconds) to be done.  So, when the consumption process is still in a state of determination I refresh the value every half a second.  My first tests were with some 4500 files in the cache, a debug build and a relatively slow laptop.  It took just something around 4 seconds to complete!  So I don't think there is a need for some progress indicator, the 'in-progress' state will well be visible as numbers will change.  But not up to me to decide.

The patch:
- adds a new method on nsICacheStorageService to get the consumption
- it forwards to the index + returns a bool whether the index is in a fully up-to-date state
- both browser/components/preferences/advanced.js and it's in-content version updated (simplified actually)
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #8390408 - Flags: review?(michal.novotny)
Attachment #8390408 - Flags: review?(ehsan)
Comment on attachment 8390408 [details] [diff] [review]
v1

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

::: netwerk/cache2/nsICacheStorageService.idl
@@ +88,5 @@
> +   *    the disk cache takes.
> +   * @return
> +   *    Number of bytes the disk cache consumes.
> +   */
> +  int64_t diskConsumption([optional] out boolean aCertain);

I find the word "certain" a bit weird in this context. Could you please use e.g. "reliable" instead?
Attachment #8390408 - Flags: review?(michal.novotny) → review+
Comment on attachment 8390408 [details] [diff] [review]
v1

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

Gavin is probably a better reviewer here.
Attachment #8390408 - Flags: review?(ehsan) → review?(gavin.sharp)
I think a number that updates itself "magically" in the UI is not ideal.

I would prefer a temporary "Calculating..." state that is then updated with the final value if obtained. That suggests the API should probably just take a callback that is invoked when "certainty" is true, rather than the polling model you created here.
Attachment #8390408 - Flags: review?(gavin.sharp) → feedback-
Attached patch v2 (obsolete) — Splinter Review
Attachment #8390408 - Attachment is obsolete: true
Attachment #8391589 - Flags: review?(gavin.sharp)
Attachment #8391589 - Flags: review?(michal.novotny)
Comment on attachment 8391589 [details] [diff] [review]
v2

Why is onNetworkCacheDiskConsumptionCalculating needed? Seems like the UI can just assume that it's unknown to start, and then fill it in when onNetworkCacheDiskConsumption fires. If that happens quickly enough to be unnoticeable, that's fine.

It would also allow simplifying the interface to take a single callback rather than an observer object with two functions.
OK Gavin, I'll rework it for you ones more.
BTW, in 99.99% cases the cache size will be known.  So drawing first the Calculating string and then redraw it immediately with a known value is IMO a waste.  That is why I introduced the "calculating" callback.  But if you think it's better to have a simple IDL I'll do it.  Or maybe tell me what all exactly you want so I don't have to retest this every time.  Thanks.
I'm happy to discuss this any time, you can find me on IRC if you prefer that to Bugzilla.

Comment 7 describes what I want, is there any part that isn't clear?
Attachment #8391589 - Flags: review?(gavin.sharp)
I thought I gave you a rational for why there should be two callbacks in comment 9.  I don't care if you want to have worse performance of the UI deliberately and want to output a "calculating" string that will in 99.99% of cases overwritten immediately with "size is XXX bytes".  I took care of performance, but if you don't like it, I will gladly remove it :D
Flags: needinfo?(gavin.sharp)
The performance of switching strings in the UI is negligible, a simpler callback interface (and thus simpler UI code) makes more sense to me.
Flags: needinfo?(gavin.sharp)
(If my assumption is correct, the delay shouldn't really be user-perceptible in the common case anyways.)
Comment on attachment 8391589 [details] [diff] [review]
v2

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

::: netwerk/cache2/CacheIndex.h
@@ +933,5 @@
> +      if (observer) {
> +        if (mSize < 0)
> +          observer->OnNetworkCacheDiskConsumptionCalculating();
> +        else
> +          observer->OnNetworkCacheDiskConsumption(mSize);

IDL says that OnNetworkCacheDiskConsumption() will be called only once, but it can be called twice when the disk size is not known when AsyncGetDiskConsumption() is called and the index switches to READY state and DiskConsumptionObserver::OnDiskConsumption() is called for the second time before DiskConsumptionObserver::Run() is called.
Attachment #8391589 - Flags: review?(michal.novotny) → feedback+
(In reply to Michal Novotny (:michal) from comment #14)
> IDL says that OnNetworkCacheDiskConsumption() will be called only once, but

I have to rework this anyway, still this may need to be addressed (probably in the IDL).  Thanks.
Attached patch v2 -> v3 (obsolete) — Splinter Review
Attachment #8391589 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
Attachment #8397367 - Flags: review?(michal.novotny)
Attachment #8397367 - Flags: review?(gavin.sharp)
- callback simplified: only called once when the size is known
- UI shows the 'calculating' string when the callback had not been notified
Attached patch v2 -> v3.1 (obsolete) — Splinter Review
I realized that the callback is always re-posted to the main thread, so the 'notified' flag doesn't have meaning.
Attachment #8397363 - Attachment is obsolete: true
Attached patch v3.1 (obsolete) — Splinter Review
Attachment #8397367 - Attachment is obsolete: true
Attachment #8397367 - Flags: review?(michal.novotny)
Attachment #8397367 - Flags: review?(gavin.sharp)
Attachment #8397389 - Flags: review?(michal.novotny)
Attachment #8397389 - Flags: review?(gavin.sharp)
Comment on attachment 8397389 [details] [diff] [review]
v3.1

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

::: netwerk/cache2/CacheIndex.cpp
@@ +1281,5 @@
> +  }
> +
> +  LOG(("CacheIndex::AsyncGetCacheSize - remembering callback"));
> +  // Indicate calculation is in progress, callback will be called
> +  // a second time when index update/build is done.

Comments are useless and confusing when you don't keep them up to date.
Attachment #8397389 - Flags: review?(michal.novotny)
(In reply to Michal Novotny (:michal) from comment #21)
> Comment on attachment 8397389 [details] [diff] [review]
> v3.1
> 
> Review of attachment 8397389 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/cache2/CacheIndex.cpp
> @@ +1281,5 @@
> > +  }
> > +
> > +  LOG(("CacheIndex::AsyncGetCacheSize - remembering callback"));
> > +  // Indicate calculation is in progress, callback will be called
> > +  // a second time when index update/build is done.
> 
> Comments are useless and confusing when you don't keep them up to date.

Michal, is this the only concern?
Comment on attachment 8397389 [details] [diff] [review]
v3.1

Also fix this LOG:

+CacheIndex::AsyncGetDiskConsumption(nsICacheStorageConsumptionObserver* aObserver)
+{
+  LOG(("CacheIndex::AsyncGetCacheSize()"));
Attachment #8397389 - Flags: review+
I'll double check the patch for both logs and comments.  Thanks.
Gavin, ping on review.
Comment on attachment 8397389 [details] [diff] [review]
v3.1

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

>+    this.observer = {
>+      onNetworkCacheDiskConsumption: function(consumption) {
...
>+      },
>+      QueryInterface: XPCOMUtils.generateQI([
>+        Components.interfaces.nsICacheStorageConsumptionObserver,
>+        Components.interfaces.nsISupportsWeakReference,
>+        Components.interfaces.nsISupports
>+      ])
>     };

this can all just be:

this.observer = (consumption) => {
...
};

and you can get rid of the XPCOMUtils import.

>+    var actualSizeLabel = document.getElementById("actualDiskCacheSize");

>+    var sizeStr = prefStrBundle.getString("actualDiskCacheSizeCalculated");

You should declare these all at the top of the function and then your callback can refer to them via closure, rather than re-declaring them. I would also eliminate the two "sizeStr" variables in favor of just assigning to actualSizeLabel.value directly, to avoid shadowing the variable.

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

>+actualDiskCacheSizeCalculated=Your web content cache disk size is being calculated

You can ask a UI person, but I would just use "Calculating web content cache size…"

>diff --git a/netwerk/cache2/nsICacheStorageService.idl b/netwerk/cache2/nsICacheStorageService.idl

>+[scriptable, uuid(7728ab5b-4c01-4483-a606-32bf5b8136cb)]
>+interface nsICacheStorageConsumptionObserver : nsISupports

Mark this interface "function" to make my previous suggestion work.
Attachment #8397389 - Flags: review?(gavin.sharp) → feedback+
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #26)
> Comment on attachment 8397389 [details] [diff] [review]
> this can all just be:
> 
> this.observer = (consumption) => {
> ...
> };
> 
> and you can get rid of the XPCOMUtils import.

Doesn't work.  I need nsISupportsWeakReference that your suggestion doesn't give me.  So I will omit this requirement.
Attached patch v4 (obsolete) — Splinter Review
- added code to show also the old cache consumption (actually c++ reimpl of the js code from browser) hence asking re-r from michal
- except the 'function' and () => {} opt, gavin's suggestions
Attachment #8397388 - Attachment is obsolete: true
Attachment #8397389 - Attachment is obsolete: true
Attachment #8400931 - Flags: review?(michal.novotny)
Attachment #8400931 - Flags: review?(gavin.sharp)
Attached patch v4(.1) (obsolete) — Splinter Review
And updated comments/logs as suggested.
Attachment #8400931 - Attachment is obsolete: true
Attachment #8400931 - Flags: review?(michal.novotny)
Attachment #8400931 - Flags: review?(gavin.sharp)
Attachment #8400933 - Flags: review?(michal.novotny)
Attachment #8400933 - Flags: review?(gavin.sharp)
Comment on attachment 8400933 [details] [diff] [review]
v4(.1)

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

::: netwerk/cache2/CacheStorageService.cpp
@@ +599,5 @@
> +
> +NS_IMETHODIMP
> +AsyncGetDiskConsumptionWrapper::OnCacheEntryInfo(nsICacheEntry *aEntry)
> +{
> +  return NS_OK;

Maybe add MOZ_ASSERT(false, "Unexpected") here.

@@ +630,5 @@
> +  nsCOMPtr<nsICacheStorage> defaultStorage;
> +  rv = DiskCacheStorage(def, false, getter_AddRefs(defaultStorage));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  nsCOMPtr<nsICacheStorage> anonymousStorage;
> +  rv = DiskCacheStorage(def, false, getter_AddRefs(anonymousStorage));

anon instead of def
Attachment #8400933 - Flags: review?(michal.novotny) → review+
Gavin, ping on review, parts of this patch are blocking ongoing development.

Michal, thanks for a quick review.
These past two weeks I've been behind on bugmail, and I've not been getting firebot notifications for these review requests, so apologies for the long review cycles. If you email me directly or ping me on IRC I'm much more responsive.
(In reply to Honza Bambas (:mayhemer) from comment #27)
> Doesn't work.  I need nsISupportsWeakReference that your suggestion doesn't
> give me.  So I will omit this requirement.

JS functions objects should support weak references. Did you see otherwise in testing?
Comment on attachment 8400933 [details] [diff] [review]
v4(.1)

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

>+      QueryInterface: XPCOMUtils.generateQI([
>+        Components.interfaces.nsICacheStorageConsumptionObserver,
>+        Components.interfaces.nsISupportsWeakReference,
>+        Components.interfaces.nsISupports

Don't need nsISupports here (generateQI adds it for you). Applies to both files.

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

>+actualDiskCacheSizeCalculated=Calculating web content cache size

Add the ellipsis? (use unicode character "…")
Attachment #8400933 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #33)
> (In reply to Honza Bambas (:mayhemer) from comment #27)
> > Doesn't work.  I need nsISupportsWeakReference that your suggestion doesn't
> > give me.  So I will omit this requirement.
> 
> JS functions objects should support weak references. Did you see otherwise
> in testing?

I did exactly what you've suggested in comment 26 and do_QueryWeakReference didn't work for me.
Attached patch v4.2 [to land]Splinter Review
Attachment #8400933 - Attachment is obsolete: true
Attachment #8402727 - Flags: review+
Comment on attachment 8402727 [details] [diff] [review]
v4.2 [to land]

>-    storage1.asyncVisitStorage(visitor, false /* Do not walk entries */);
>-    storage2.asyncVisitStorage(visitor, false /* Do not walk entries */);
>+    cacheService.asyncGetDiskConsumption(this.observer);
So, what does this new API do that the old one didn't?
Flags: needinfo?(honzab.moz)
https://hg.mozilla.org/mozilla-central/rev/d327810140cd
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
(In reply to neil@parkwaycc.co.uk from comment #37)
> >-    storage1.asyncVisitStorage(visitor, false /* Do not walk entries */);
> >-    storage2.asyncVisitStorage(visitor, false /* Do not walk entries */);
> >+    cacheService.asyncGetDiskConsumption(this.observer);
> So, what does this new API do that the old one didn't?

Shows the consumption of the new cache correctly.  The visitor API was used just as an intermediate step and was pretty overcomplicated for getting just a disk consumption number and also till today is not correctly implemented for the new cache (bug 916052).  That bug was not planned for cache2 first release at the time I started to work on this - and still might not make it for 100%, hence this new simple API.
Flags: needinfo?(honzab.moz)
Depends on: 998445
Depends on: 998693
(In reply to Honza Bambas from comment #40)
> (In reply to comment #37)
> > So, what does this new API do that the old one didn't?
> 
> Shows the consumption of the new cache correctly.  The visitor API was used
> just as an intermediate step and was pretty overcomplicated for getting just
> a disk consumption number and also till today is not correctly implemented
> for the new cache (bug 916052).  That bug was not planned for cache2 first
> release at the time I started to work on this - and still might not make it
> for 100%, hence this new simple API.

Right now we're still using the visitor API for offline apps?
Flags: needinfo?(honzab.moz)
(In reply to neil@parkwaycc.co.uk from comment #41)
> Right now we're still using the visitor API for offline apps?

Yes.
Flags: needinfo?(honzab.moz)
You need to log in before you can comment on or make changes to this bug.