Closed Bug 669117 Opened 13 years ago Closed 13 years ago

Necko memory cache needs an about:memory reporter

Categories

(Core :: Networking: Cache, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: khuey, Assigned: n.nethercote)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2][inbound])

Attachments

(1 file, 3 obsolete files)

Whiteboard: [MemShrink]
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.
Attached patch hacky WIP patch (obsolete) — Splinter Review
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.
(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.
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee: nobody → nnethercote
Michal/Bjarne: can one of you take a look at the patch here?
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 :)
(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).
Attached patch patch, v2 (obsolete) — Splinter Review
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 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)
Attached patch patch, v3 (obsolete) — Splinter Review
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+
> (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"
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!
I'm just glad no-one suggested a name beginning with "netwerk" :P
Attached patch patch, v4Splinter Review
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?
(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+
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
Closed: 13 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.
(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.
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.