Closed
Bug 682437
Opened 12 years ago
Closed 12 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•12 years ago
|
Assignee: nobody → justin.lebar+bug
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #559909 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 2•12 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•12 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•12 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•12 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•12 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•12 years ago
|
||
Attachment #560037 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•12 years ago
|
Attachment #559909 -
Attachment is obsolete: true
Assignee | ||
Comment 8•12 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•12 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•12 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•12 years ago
|
||
...I guess counting sizeof(*this) makes sense if inheritance is involved. I still would prefer two functions, though.
Assignee | ||
Comment 12•12 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•12 years ago
|
||
I don't.
![]() |
||
Comment 14•12 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•12 years ago
|
||
With changes to jsdhash removed.
Assignee | ||
Updated•12 years ago
|
Attachment #560037 -
Attachment is obsolete: true
Attachment #560037 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•12 years ago
|
Attachment #563539 -
Flags: review?(bzbarsky)
![]() |
Reporter | |
Comment 16•12 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•12 years ago
|
||
Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/535331914716
Whiteboard: [MemShrink:P2] → [MemShrink:P2][inbound]
Comment 18•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/535331914716
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
![]() |
||
Comment 19•12 years ago
|
||
How big can this get? I'm only seeing 10KB right now...
Assignee | ||
Comment 20•12 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•12 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•12 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•12 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•12 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
•