Necko memory cache needs an about:memory reporter

RESOLVED FIXED in mozilla8

Status

()

Core
Networking: Cache
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: khuey, Assigned: njn)

Tracking

(Blocks: 1 bug)

unspecified
mozilla8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

([MemShrink:P2][inbound])

Attachments

(1 attachment, 3 obsolete attachments)

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

Updated

6 years ago
Blocks: 563700, 640791
(Assignee)

Updated

6 years ago
Whiteboard: [MemShrink]
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
Created attachment 543722 [details] [diff] [review]
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.
> 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.
(Assignee)

Comment 4

6 years ago
(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?
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.
(Assignee)

Updated

6 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Updated

6 years ago
Assignee: nobody → nnethercote
Michal/Bjarne: can one of you take a look at the patch here?
(Assignee)

Comment 7

6 years ago
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

6 years ago
(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).
(Assignee)

Comment 9

6 years ago
Created attachment 544399 [details] [diff] [review]
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.
Attachment #543722 - Attachment is obsolete: true
Attachment #544399 - Flags: review?(bjarne)
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

6 years ago
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.
Attachment #544399 - Flags: review?(bjarne)
(Assignee)

Comment 12

6 years ago
Created attachment 545595 [details] [diff] [review]
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.
Attachment #544399 - Attachment is obsolete: true
Attachment #545595 - Flags: review?(khuey)
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?
Attachment #545595 - Flags: review?(khuey)
Attachment #545595 - Flags: review?(jduell.mcbugs)
Attachment #545595 - Flags: review+
(Assignee)

Comment 14

6 years ago
> (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 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.
Attachment #545595 - Flags: review?(jduell.mcbugs) → review+
To be clear, I'd prefer that the listing show up in about:memory as "network-memory-cache" rather than "network-memory-cache-device"
(Assignee)

Comment 17

6 years ago
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!
(Assignee)

Comment 18

6 years ago
I'm just glad no-one suggested a name beginning with "netwerk" :P
(Assignee)

Comment 19

6 years ago
Created attachment 546069 [details] [diff] [review]
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.
Attachment #545595 - Attachment is obsolete: true
Attachment #546069 - Flags: review?(khuey)
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?
(Assignee)

Comment 21

6 years ago
(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 on attachment 546069 [details] [diff] [review]
patch, v4

r=me with the null guard if necessary
Attachment #546069 - Flags: review?(khuey) → review+
(Assignee)

Comment 23

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/b31c7eeec41b
Whiteboard: [MemShrink:P2] → [MemShrink:P2][inbound]
http://hg.mozilla.org/mozilla-central/rev/b31c7eeec41b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
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.
(Assignee)

Comment 26

5 years ago
(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.
(Assignee)

Comment 27

5 years ago
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.
Ah, right--I see it with ?verbose.   Thanks!
You need to log in before you can comment on or make changes to this bug.