Make "network-memory-cache" memory reporter traversal-based

RESOLVED WONTFIX

Status

()

Core
Networking: Cache
RESOLVED WONTFIX
6 years ago
2 years ago

People

(Reporter: njn, Unassigned)

Tracking

12 Branch
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Created attachment 607047 [details] [diff] [review]
patch, v0

+++ This bug was initially created as a clone of Bug #713203 +++

The attached patch attempts to convert the "network-memory-cache" memory reporter traversal-based instead of counter-based, to avoid problems such as the one in bug 713203, and to integrate it with DMD.

Unfortunately, I think the lion's share of the memory is stored within nsCacheEntry::mData.  jduell, is that right?  And that has type |nsISupports*|, which makes it hard to query for the size... I can probably use |nsISizeOf| somehow to measure that memory.
Attachment #607047 - Flags: feedback?(jduell.mcbugs)
Comment on attachment 607047 [details] [diff] [review]
patch, v0

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

Michal or Nick are better reviewers than I for this.
Attachment #607047 - Flags: feedback?(jduell.mcbugs) → feedback?(michal.novotny)
Comment on attachment 607047 [details] [diff] [review]
patch, v0

(In reply to Nicholas Nethercote [:njn] from comment #0)
> Unfortunately, I think the lion's share of the memory is stored within
> nsCacheEntry::mData.

You're right, this patch won't count allocated segments of nsStorageStream::SegmentedBuffer.


> And that has type |nsISupports*|, which makes it hard to query for the size..
> I can probably use |nsISizeOf| somehow to measure that memory.

nsCacheEntry::mData can be anything when the session is NOT_STREAM_BASED, but I think that there is no code that uses our cache in non-stream mode. For a stream based session mData can be:

- nsDiskCacheBinding in case of disk cache
- nsStorageStream in case of memory cache
- nsOfflineCacheBinding in case of offline cache


BTW there is missing a cache lock in the current code in nsCacheService::MemoryDeviceSize(). This bug isn't probably critical now since we just return an integer member. But it will become critical with your patch since you're enumerating eviction lists in nsMemoryCacheDevice::SizeOfIncludingThis() while they might change on another thread.
Attachment #607047 - Flags: feedback?(michal.novotny) → feedback-

Updated

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

Updated

6 years ago
Assignee: nobody → n.nethercote
(Reporter)

Updated

4 years ago
Assignee: n.nethercote → nobody
patches code that is deprecated..
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.