Closed
Bug 682437
Opened 13 years ago
Closed 13 years ago
Need memory reporter for History::mObservers
Categories
(Toolkit :: Places, defect)
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: bzbarsky, Assigned: justin.lebar+bug)
References
Details
(Whiteboard: [MemShrink:P2][inbound])
Attachments
(1 file, 2 obsolete files)
9.62 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
This hashtable has an entry per link, typically, so can be pretty big.
Updated•13 years ago
|
Assignee: nobody → justin.lebar+bug
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #559909 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 2•13 years ago
|
||
I couldn't figure out how to generate pldhash from jsdhash, so I tried to make the same changes to both files. Hopefully I did it right.
Reporter | ||
Comment 3•13 years ago
|
||
> I couldn't figure out how to generate pldhash from jsdhash,
I'm not sure it's possible anymore; the jsdhash impl has been hacked enough that the sed script probably no longer works right. :(
Reporter | ||
Comment 4•13 years ago
|
||
Comment on attachment 559909 [details] [diff] [review] Patch v1 >+ return PL_DHashTableSizeOf(&mTable); Doing that if !IsInitialized() will return bogus numbers because entrySize hasn't been set yet, right? I think the rest of this looks ok, but we need a check for this somewhere. Maybe just check for ops in the hashtable code itself and return 0 if that's null?
Attachment #559909 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 5•13 years ago
|
||
> I'm not sure it's possible anymore; the jsdhash impl has been hacked enough that the > sed script probably no longer works right. :( Ah. So can I use moz_malloc_usable_size in the pldhash implementation? (In reply to Boris Zbarsky (:bz) from comment #4) > Doing that if !IsInitialized() will return bogus numbers because entrySize > hasn't been set yet, right? Actually -- and this wasn't intentional, so it at least deserves a comment -- nsTHashtable::IsInitialized is: PRBool IsInitialized() const { return !!mTable.entrySize; } so entrySize will be 0 when we're not initialized, which means SizeOf will be 0, just like we want.
Reporter | ||
Comment 6•13 years ago
|
||
> Ah. So can I use moz_malloc_usable_size in the pldhash implementation? I don't know.... > so entrySize will be 0 when we're not initialized Ah, ok. Good. And yes, that needs a comment. But other users of those pldhash/jsdhash functions should check for initialization first, right? That should get documented, if so.
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #560037 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
Attachment #559909 -
Attachment is obsolete: true
Assignee | ||
Comment 8•13 years ago
|
||
>> Ah. So can I use moz_malloc_usable_size in the pldhash implementation? > > I don't know.... I'm now checking for mozalloc in pldhash and using moz_malloc_usable_size if we have mozalloc. I didn't make the change in jsdhash, since mozalloc isn't there.
Comment 9•13 years ago
|
||
Comment on attachment 560037 [details] [diff] [review] Patch v2 Review of attachment 560037 [details] [diff] [review]: ----------------------------------------------------------------- Do you have any numbers for how big this table gets? ::: js/src/jsdhash.cpp @@ +787,5 @@ > +JS_PUBLIC_API(uint64) > +JS_DHashTableSizeOf(JSDHashTable *table) > +{ > + // XXX malloc_usable_size would be better. > + return JS_DHASH_TABLE_SIZE(table) * table->entrySize; In cases like this I've been passing in a pointer to a malloc_usable_size-like function. See JSUsableSizeFun in jsutil.h. ::: js/src/jsdhash.h @@ +579,5 @@ > +/** > + * Get the hashtable's "shallow heap size" in bytes. The size returned here > + * includes all the heap memory allocated by the hashtable itself. It does not > + * include sizeof(*this) or heap memory allocated by the objects in the > + * hash table. I've been using a bool param to indicate if sizeof(*this) should be counted, see HashTable::sizeOf() in jshashtable.h.
Assignee | ||
Comment 10•13 years ago
|
||
> Do you have any numbers for how big this table gets? Not over 1MB, even when I have a document with thousands of links. > I've been using a bool param to indicate if sizeof(*this) should be counted, see > HashTable::sizeOf() in jshashtable.h. We should probably have a convention for these functions, but in general, I really don't like public functions which take bool parameters. (Quick, does addObserver(foo, false) add a strong or a weak reference to the observer?)
Assignee | ||
Comment 11•13 years ago
|
||
...I guess counting sizeof(*this) makes sense if inheritance is involved. I still would prefer two functions, though.
Assignee | ||
Comment 12•13 years ago
|
||
bz/luke, do you have a preference wrt making JS_DHashTableSizeOf take a JSUsableSizeFun? It doesn't matter much now since nobody in JS is using this...
Reporter | ||
Comment 13•13 years ago
|
||
I don't.
Comment 14•13 years ago
|
||
Perhaps you should only add it to the PL_ version. I see only 2 uses of JS_DHashTable remaining; maybe I'll remove them today.
Assignee | ||
Comment 15•13 years ago
|
||
With changes to jsdhash removed.
Assignee | ||
Updated•13 years ago
|
Attachment #560037 -
Attachment is obsolete: true
Attachment #560037 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
Attachment #563539 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 16•13 years ago
|
||
Comment on attachment 563539 [details] [diff] [review] Patch v3 r=me. Sorry for the lag....
Attachment #563539 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 17•13 years ago
|
||
Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/535331914716
Whiteboard: [MemShrink:P2] → [MemShrink:P2][inbound]
Comment 18•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/535331914716
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Comment 19•13 years ago
|
||
How big can this get? I'm only seeing 10KB right now...
Assignee | ||
Comment 20•13 years ago
|
||
On the testcase in bug 693898, it gets up to 1.8mb. But that testcase eats up 500mb of memory or so...
Reporter | ||
Comment 21•13 years ago
|
||
> How big can this get?
Depends on how many links the pages you have loaded have... but until we landed this reporter we just had no idea.
Comment 22•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #21) > > Depends on how many links the pages you have loaded have... but until we > landed this reporter we just had no idea. Ok. But in general I think using data from DMD to drive the implementation of memory reporters is a good idea!
Reporter | ||
Comment 23•13 years ago
|
||
We did. This was the #10 item in bug 678376 comment 13. In that DMD log, if I read it correctly, this hashtable was using 21MB out of total browser memory usage of 622MB. In fact, looking at the about:memory for that testcase right now, I see: 1,920.75 MB (100.0%) -- explicit ... ├─────48.18 MB (02.51%) -- history-links-hashtable (I'm on a 64-bit build, which might explain the somewhat higher numbers.)
Comment 24•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #23) > We did. This was the #10 item in bug 678376 comment 13. Excellent!
You need to log in
before you can comment on or make changes to this bug.
Description
•