Last Comment Bug 682437 - Need memory reporter for History::mObservers
: Need memory reporter for History::mObservers
Status: RESOLVED FIXED
[MemShrink:P2][inbound]
:
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on:
Blocks: 126212 671297 678376 688125 693898
  Show dependency treegraph
 
Reported: 2011-08-26 15:22 PDT by Boris Zbarsky [:bz]
Modified: 2011-10-17 18:01 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (9.14 KB, patch)
2011-09-12 16:19 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review-
Details | Diff | Splinter Review
Patch v2 (11.28 KB, patch)
2011-09-13 14:25 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v3 (9.62 KB, patch)
2011-09-29 13:12 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2011-08-26 15:22:46 PDT
This hashtable has an entry per link, typically, so can be pretty big.
Comment 1 Justin Lebar (not reading bugmail) 2011-09-12 16:19:08 PDT
Created attachment 559909 [details] [diff] [review]
Patch v1
Comment 2 Justin Lebar (not reading bugmail) 2011-09-12 16:22:34 PDT
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.
Comment 3 Boris Zbarsky [:bz] 2011-09-12 21:40:54 PDT
> 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 4 Boris Zbarsky [:bz] 2011-09-12 21:43:59 PDT
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?
Comment 5 Justin Lebar (not reading bugmail) 2011-09-12 21:52:28 PDT
> 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.
Comment 6 Boris Zbarsky [:bz] 2011-09-12 22:12:56 PDT
> 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.
Comment 7 Justin Lebar (not reading bugmail) 2011-09-13 14:25:29 PDT
Created attachment 560037 [details] [diff] [review]
Patch v2
Comment 8 Justin Lebar (not reading bugmail) 2011-09-13 15:14:04 PDT
>> 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 Nicholas Nethercote [:njn] 2011-09-27 08:41:37 PDT
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.
Comment 10 Justin Lebar (not reading bugmail) 2011-09-27 08:49:40 PDT
> 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?)
Comment 11 Justin Lebar (not reading bugmail) 2011-09-27 08:51:13 PDT
...I guess counting sizeof(*this) makes sense if inheritance is involved.

I still would prefer two functions, though.
Comment 12 Justin Lebar (not reading bugmail) 2011-09-27 08:53:05 PDT
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...
Comment 13 Boris Zbarsky [:bz] 2011-09-27 08:54:48 PDT
I don't.
Comment 14 Luke Wagner [:luke] 2011-09-27 09:33:43 PDT
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.
Comment 15 Justin Lebar (not reading bugmail) 2011-09-29 13:12:15 PDT
Created attachment 563539 [details] [diff] [review]
Patch v3

With changes to jsdhash removed.
Comment 16 Boris Zbarsky [:bz] 2011-10-12 11:58:19 PDT
Comment on attachment 563539 [details] [diff] [review]
Patch v3

r=me.  Sorry for the lag....
Comment 17 Justin Lebar (not reading bugmail) 2011-10-12 17:16:27 PDT
Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/535331914716
Comment 18 Marco Bonardo [::mak] 2011-10-13 07:33:02 PDT
https://hg.mozilla.org/mozilla-central/rev/535331914716
Comment 19 Nicholas Nethercote [:njn] 2011-10-16 20:30:02 PDT
How big can this get?  I'm only seeing 10KB right now...
Comment 20 Justin Lebar (not reading bugmail) 2011-10-16 21:15:20 PDT
On the testcase in bug 693898, it gets up to 1.8mb.  But that testcase eats up 500mb of memory or so...
Comment 21 Boris Zbarsky [:bz] 2011-10-16 22:05:33 PDT
> 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 Nicholas Nethercote [:njn] 2011-10-16 23:10:28 PDT
(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!
Comment 23 Boris Zbarsky [:bz] 2011-10-17 07:18:41 PDT
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 Nicholas Nethercote [:njn] 2011-10-17 18:01:02 PDT
(In reply to Boris Zbarsky (:bz) from comment #23)
> We did.  This was the #10 item in bug 678376 comment 13.

Excellent!

Note You need to log in before you can comment on or make changes to this bug.