1059654 - properly report memory for images that don't happen to have an entry in the image cache

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Timothy Nikkel (:tnikkel)]

The image cache is just a map from URI to imgRequest. The most common case for an image not to be in the cache is if someone requests a URI that's already in the cache, and we have to download a fresh copy of the image (say because there is a newer version on the server, or the server tells us not to cache it). The fresh copy will now occupy the spot in the cache for that URI, but the old users still need the old image (loading a new image somewhere else shouldn't change what's displayed in an already loaded image) so the old image sticks around.

To track them I just added a hash set of imgRequests to imgLoader called the "not cache" (I'm not clever at names). When we remove an imgRequest from the cache it goes in the not cache, and similarly vice versa. It doesn't hold a ref so we clear the entry from the imgRequest destructor. I ran into a snag with this were during an xpcshell test imgRequest can hold stale imgLoader pointers. I think this can easily happen if we shutdown the imgLoader, it drops its ref to the imgRequest, but the imgRequest is still in a cycle so it stays alive. After the imgLoader destruction is complete we cycle collect and the imgRequest as a stale pointer. So I added a clear of imgLoader pointers for any imgRequests that might be alive after we've dropped all our refs to imgRequests.

I decided to call the newly reported images "stale" in about:memory. So we now have a similar used/unused raster/vector tree under images/stale as we did under images.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Nick, if you have any thoughts about how to report these images let me know. Also I think you've touched this code recently so you can look it over if you like.

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 8480380 [details] [diff] [review]
patch

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

My main comment is that I find "not cache" a very strange name. Is "not" an acronym? Or does it just mean "uncached"? In the memory report paths you use "stale" instead... maybe that would be a better name to use throughout?

Otherwise, it looks good. How much memory is typically used by stale images as opposed to non-stale images?
And is it correct to assume that a stale image is likely to be freed soon?

Thanks.

::: image/src/imgLoader.h
@@ +379,5 @@
>  
>    imgCacheTable mChromeCache;
>    imgCacheQueue mChromeCacheQueue;
>  
> +  // hash set of every imgRequest for this loader that isn't in mCache or

s/hash/Hash/

@@ +382,5 @@
>  
> +  // hash set of every imgRequest for this loader that isn't in mCache or
> +  // mChromeCache. The union over all imgLoader's of mCache, mChromeCache,
> +  // and mNotCache should be every imgRequest that is alive. These are weak
> +  // pointers so we relie on the imgRequest destructor to remove itself.

s/relie/rely/

Comment 3

[Timothy Nikkel (:tnikkel)]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> My main comment is that I find "not cache" a very strange name. Is "not" an
> acronym? Or does it just mean "uncached"? In the memory report paths you use
> "stale" instead... maybe that would be a better name to use throughout?

It's just a container to hold a list of all images that aren't in the cache so that we have a complete list (together with the cache) of all images. I thought that stale would be a reasonable way to report the information in a semi-user facing way. However calling it stale in the code might be confusing because we already have separate concepts of expired images, and images that must be validated. I could change it to uncached throughout (or any better suggestion).

> Otherwise, it looks good. How much memory is typically used by stale images
> as opposed to non-stale images?

I think typically they will be very small, but in some situations they might balloon up (like bug 1033679). But we won't know for sure until we measure them.

> And is it correct to assume that a stale image is likely to be freed soon?

Not necessarily. If you load the same page in two tabs and the server (for whatever reason) required us to fetch the image again we'd keep the old version of image for the old tab alive outside of the cache until that tab was closed.

Comment 4

[Nicholas Nethercote [inactive]]

"uncached" sounds good, thanks.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8480380 [details] [diff] [review]
patch

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

Overall looks good. I agree with Nick that "uncached" sounds better.

I also need to take a shower after looking at the memory reporting code. The combination of template specializations and preprocessor macros... wow!

::: image/src/imgLoader.cpp
@@ +319,5 @@
>      return PL_DHASH_NEXT;
>    }
>  
> +  static PLDHashOperator EntryNotCacheImageSizes(nsPtrHashKey<imgRequest>* aEntry,
> +                                                 void *aUserArg)

Nit: per our style guide, the '*' should go with the type here and throughout the code in this file.

@@ +1794,5 @@
>  
>    return NS_OK;
>  }
>  
> +void imgLoader::AddToNotCache(imgRequest *request)

Nit: aRequest. Same goes for RemoveFromNotCache.

::: image/src/imgLoader.h
@@ +216,5 @@
>  
>  public:
>    typedef mozilla::image::ImageURL ImageURL;
>    typedef nsRefPtrHashtable<nsCStringHashKey, imgCacheEntry> imgCacheTable;
> +  typedef nsTHashtable< nsPtrHashKey<imgRequest> > imgNotCacheSet;

FYI, "> >" isn't needed anymore on any compiler we support.

@@ +288,5 @@
>  
>    bool PutIntoCache(nsIURI *key, imgCacheEntry *entry);
>  
> +  void AddToNotCache(imgRequest *request);
> +  void RemoveFromNotCache(imgRequest *request);

Again our style guide would recommend "imgRequest*" here, though I realize you're probably just following local style.

@@ +386,5 @@
> +  // pointers so we relie on the imgRequest destructor to remove itself.
> +  imgNotCacheSet mNotCache;
> +  // The imgRequest can have refs to them held on non-main thread, so we need
> +  // a mutex because we modify the not cache from the imgRequest destructor.
> +  typedef mozilla::Mutex Mutex;

I suggest grouping this with the other typedefs.

@@ +387,5 @@
> +  imgNotCacheSet mNotCache;
> +  // The imgRequest can have refs to them held on non-main thread, so we need
> +  // a mutex because we modify the not cache from the imgRequest destructor.
> +  typedef mozilla::Mutex Mutex;
> +  Mutex* mNotCacheMutex;

This should be a Mutex instead of a pointer to a Mutex.

::: image/src/imgRequest.cpp
@@ +403,5 @@
> +  nsRefPtr<imgStatusTracker> statusTracker = GetStatusTracker();
> +  if (!statusTracker || statusTracker->ConsumerCount() == 0) {
> +    return false;
> +  }
> +  return true;

This is just "return statusTracker && statusTracker->ConsumerCount() > 0", right?

@@ +433,5 @@
>    if (p)
>      p->AdjustPriority(delta);
>  }
>  
> +  bool HasConsumers();

This is a forward declaration for a function that never gets defined...

Comment 6

[Timothy Nikkel (:tnikkel)]

Made those changes, pushed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6414bb45f5d

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b6414bb45f5d