682437 - Need memory reporter for History::mObservers

Status: RESOLVED FIXED

Description

[Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-)]

This hashtable has an entry per link, typically, so can be pretty big.

Comment 1

[Justin Lebar (not reading bugmail)]

Created attachment 559909 [details] [diff] [review]
Patch v1

Comment 2

[Justin Lebar (not reading bugmail)]

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] (if a patch has no decent message, automatic r-)]

> 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] (if a patch has no decent message, automatic r-)]

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)]

> 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] (if a patch has no decent message, automatic r-)]

> 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)]

Created attachment 560037 [details] [diff] [review]
Patch v2

Comment 8

[Justin Lebar (not reading bugmail)]

>> 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]]

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)]

> 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)]

...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)]

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] (if a patch has no decent message, automatic r-)]

I don't.

Comment 14

[Luke Wagner [:luke]]

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)]

Created attachment 563539 [details] [diff] [review]
Patch v3

With changes to jsdhash removed.

Comment 16

[Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-)]

Comment on attachment 563539 [details] [diff] [review]
Patch v3

r=me.  Sorry for the lag....

Comment 17

[Justin Lebar (not reading bugmail)]

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/535331914716

Comment 18

[Marco Bonardo [::mak]]

https://hg.mozilla.org/mozilla-central/rev/535331914716

Comment 19

[Nicholas Nethercote [:njn]]

How big can this get?  I'm only seeing 10KB right now...

Comment 20

[Justin Lebar (not reading bugmail)]

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] (if a patch has no decent message, automatic r-)]

> 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]]

(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] (if a patch has no decent message, automatic r-)]

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]]

(In reply to Boris Zbarsky (:bz) from comment #23)
> We did.  This was the #10 item in bug 678376 comment 13.

Excellent!