669117 - Necko memory cache needs an about:memory reporter

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

If http://mxr.mozilla.org/mozilla-central/source/netwerk/cache/nsMemoryCacheDevice.h#123 is accurate this should be pretty easy.

Comment 1

[Nicholas Nethercote [inactive]]

If there's anything else in Necko that uses reasonable amount of memory (eg. more than 1MB) it would be good to report that too.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: hacky WIP patch

Here's a really hacky patch that I'm dumping here because it's 5pm and I'm stopping for the day.  I've never before set foot in netwerk/, I fully admit to having no idea what I'm doing.

How big do we expect the memory device to get?  It started off tiny, after loading a few pages it's up to ~500KB.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> How big do we expect the memory device to get?

It depends on user preferences.  By default, the size is computed by nsCacheProfilePrefObserver::MemoryCacheCapacity and maxes out at 30MB if the comments reflect reality.

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to comment #3)
> > How big do we expect the memory device to get?
> 
> It depends on user preferences.  By default, the size is computed by
> nsCacheProfilePrefObserver::MemoryCacheCapacity and maxes out at 30MB if the
> comments reflect reality.

Ok, 30MB is definitely worth tracking.  Are there any other big memory users under network?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Not that I can think of offhand...

There's a network-related one that's really DOM: XHR.  The data retrieved by XHR is stored in memory in many cases; we should add an XHR memory reporter.

Comment 6

[Jason Duell]

Michal/Bjarne: can one of you take a look at the patch here?

Comment 7

[Nicholas Nethercote [inactive]]

And please note that the patch is obviously crap and needs to be cleaned up, but confirmation that I'm measuring the right thing would be welcome :)

Comment 8

[Bjarne (:bjarne)]

(In reply to comment #6)
> Michal/Bjarne: can one of you take a look at the patch here?

|mTotalSize| is probably the correct value to use, yes. It is also the value reported in about:cache as "Storage in Use". The "Inactive storage" is storage used by entries currently not used on any page (iirc).

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: patch, v2

A cleaner patch.  Details:

- I'm reporting "active" and "inactive" memory in the memory device.  The 
  former is mTotalSize - mInactivesize;  the latter is mInactiveSize.

  The weird thing is that "active" was always zero on the small amount of 
  testing I did.  And that matched about:cache, which said:

  - Storage in use: 411 KiB
  - Inactive storage: 411 KiB

  So I don't understand what "in use" and "inactive" mean here.

- The descriptions of the reporters should be more specific, but I don't 
  really understand enough to make them so, due to the above.

Comment 10

[Honza Bambas (:mayhemer)]

mInactiveSize is the sum of bytes allocated by entries that are currently not held by any http channel.

An "active" entry means an entry being currently worked with which is very short, usually just the time from the load start to the load end, while a channel pushes or reads data to or from the cache entry, sets/reads headers.

So, mTotalSize - mInactiveSize will mostly give you 0.  IMO there is no reason to report bytes allocated by only the active entries, its not interesting.

Comment 11

[Bjarne (:bjarne)]

Comment on attachment 544399 [details] [diff] [review]
patch, v2

(Very nicely formulated, Honza.)

I'm not really the right person to review this since the changes related to the cache are trivial. Comment #10 confirms that you seem to be reporting relevant things. I'd suggest that someone familiar with the memory-reporting mechanism does the review.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: patch, v3

Just the one reporter now:  network-memory-cache-device.  It's a mouthful but about:cache uses "memory cache device" so it seems reasonable.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 545595 [details] [diff] [review]
patch, v3

I'm going to rope in a Necko peer to take a look at this too.  In particular:

@@ -1100,16 +1110,18 @@ nsCacheService::Shutdown()
 
         // Make sure to wait for any pending cache-operations before
         // proceeding with destructive actions (bug #620660)
         (void) SyncWithCacheIOThread();
         
         // deallocate memory and disk caches
         delete mMemoryDevice;
         mMemoryDevice = nsnull;
+        NS_UnregisterMemoryReporter(MemoryDeviceReporter);
+        MemoryDeviceReporter = nsnull;
 
 #ifdef NECKO_DISK_CACHE
         delete mDiskDevice;
         mDiskDevice = nsnull;
 #endif // !NECKO_DISK_CACHE
 
 #ifdef NECKO_OFFLINE_CACHE
         NS_IF_RELEASE(mOfflineDevice);

makes me a bit uncomfortable (we should probably unregister the reporter before deleting the device.)

Also, perhaps we should report as network/memory-cache-device in case we add other Necko reporters in the future?

Comment 14

[Nicholas Nethercote [inactive]]

> (we should probably unregister the reporter
> before deleting the device.)

Good idea, I'll change.

> Also, perhaps we should report as network/memory-cache-device in case we add
> other Necko reporters in the future?

I had that originally, but it looks silly and is unnecessarily verbose having an entry like this:

├───11,270,674 B (03.61%) -- network
│   └──11,270,674 B (03.61%) -- memory-cache-device

If we ever add any other network things, we can tree-ify it easily.  (I've contemplated collapsing storage/sqlite/* into storage-sqlite/* for this reason, but never got around to it.)

Comment 15

[Jason Duell]

Comment on attachment 545595 [details] [diff] [review]
patch, v3

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

Looks good.

::: netwerk/cache/nsCacheService.cpp
@@ +1010,5 @@
> +    KIND_HEAP,
> +    UNITS_BYTES,
> +    nsCacheService::MemoryDeviceSize,
> +    "Memory used by the network memory cache device.")
> +

Any reason we can't just refer to it as "network memory cache", i.e. does "device" add anything, really?

@@ +1120,1 @@
>  

+1 on khuey's suggestion to move above deletion of mMemoryDevice.

Comment 16

[Jason Duell]

To be clear, I'd prefer that the listing show up in about:memory as "network-memory-cache" rather than "network-memory-cache-device"

Comment 17

[Nicholas Nethercote [inactive]]

I was just trying to match the terminology in about:cache, but "network-memory-cache" is fine.  And I'll remove "device" from the description, too.  Thanks for the quick review!

Comment 18

[Nicholas Nethercote [inactive]]

I'm just glad no-one suggested a name beginning with "netwerk" :P

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: patch, v4

I got lots of xpcshell failures: "MemoryReporter_NetworkMemoryCache not thread-safe".  This patch makes it thread-safe.  This required some minor infrastructure changes in nsIMemoryReporter.idl.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 546069 [details] [diff] [review]
patch, v4

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

::: netwerk/cache/nsCacheService.cpp
@@ +1935,5 @@
>  
> +PRInt64
> +nsCacheService::MemoryDeviceSize()
> +{
> +    return GlobalInstance()->mMemoryDevice->TotalSize();

Can mMemoryDevice be null?  Do other callsites nullcheck?

Comment 21

[Nicholas Nethercote [inactive]]

(In reply to comment #20)
> 
> Can mMemoryDevice be null?

It can -- allocation can fail in CreateMemoryDevice().  Good catch, I'll add a null check.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 546069 [details] [diff] [review]
patch, v4

r=me with the null guard if necessary

Comment 23

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b31c7eeec41b

Comment 24

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/b31c7eeec41b

Comment 25

[Jason Duell]

Nicolas,

Is there some reason why I don't see network-memory-cache in my nightly's about:memory?  The memory cache is not empty (about:cache shows that), though it's small, but I thought we were reporting the total RAM reserved (mTotalSize), not the amount actually being used.

Comment 26

[Nicholas Nethercote [inactive]]

(In reply to Jason Duell (:jduell) from comment #25)
> Nicolas,
> 
> Is there some reason why I don't see network-memory-cache in my nightly's
> about:memory?  The memory cache is not empty (about:cache shows that),
> though it's small, but I thought we were reporting the total RAM reserved
> (mTotalSize), not the amount actually being used.

The heart of the reporter is this:

size_t
nsMemoryCacheDevice::TotalSize()
{
    return mTotalSize;
}

so it should be working.  I'll take a look on Monday.

Comment 27

[Nicholas Nethercote [inactive]]

Oh wait, if you're viewing about:memory the network-memory-cache might be aggregated in one of the "N omitted" branches.  Try about:memory?verbose which turns off the aggregation of small reports.

Comment 28

[Jason Duell]

Ah, right--I see it with ?verbose.   Thanks!