This hashtable has an entry per link, typically, so can be pretty big.
Created attachment 559909 [details] [diff] [review] Patch v1
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.
> 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. :(
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?
> 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.
> 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.
Created attachment 560037 [details] [diff] [review] Patch v2
>> 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 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.
> 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?)
...I guess counting sizeof(*this) makes sense if inheritance is involved. I still would prefer two functions, though.
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...
I don't.
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.
Created attachment 563539 [details] [diff] [review] Patch v3 With changes to jsdhash removed.
Comment on attachment 563539 [details] [diff] [review] Patch v3 r=me. Sorry for the lag....
Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/535331914716
https://hg.mozilla.org/mozilla-central/rev/535331914716
How big can this get? I'm only seeing 10KB right now...
On the testcase in bug 693898, it gets up to 1.8mb. But that testcase eats up 500mb of memory or so...
> 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.
(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!
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.)
(In reply to Boris Zbarsky (:bz) from comment #23) > We did. This was the #10 item in bug 678376 comment 13. Excellent!
or
