The default bug view has changed. See this FAQ.

Need memory reporter for History::mObservers

RESOLVED FIXED in mozilla10

Status

()

Toolkit
Places
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bz, Assigned: Justin Lebar (not reading bugmail))

Tracking

Trunk
mozilla10
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2][inbound])

Attachments

(1 attachment, 2 obsolete attachments)

This hashtable has an entry per link, typically, so can be pretty big.
Blocks: 678376
Blocks: 126212
Assignee: nobody → justin.lebar+bug
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 1

6 years ago
Created attachment 559909 [details] [diff] [review]
Patch v1
Attachment #559909 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

6 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.
> 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?
Attachment #559909 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 5

6 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.
> 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

6 years ago
Created attachment 560037 [details] [diff] [review]
Patch v2
Attachment #560037 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #559909 - Attachment is obsolete: true
(Assignee)

Comment 8

6 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.
Blocks: 688125
Blocks: 671297
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

6 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

6 years ago
...I guess counting sizeof(*this) makes sense if inheritance is involved.

I still would prefer two functions, though.
(Assignee)

Comment 12

6 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...
I don't.

Comment 14

6 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

6 years ago
Created attachment 563539 [details] [diff] [review]
Patch v3

With changes to jsdhash removed.
(Assignee)

Updated

6 years ago
Attachment #560037 - Attachment is obsolete: true
Attachment #560037 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #563539 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Blocks: 693898
Comment on attachment 563539 [details] [diff] [review]
Patch v3

r=me.  Sorry for the lag....
Attachment #563539 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 17

6 years ago
Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/535331914716
Whiteboard: [MemShrink:P2] → [MemShrink:P2][inbound]
https://hg.mozilla.org/mozilla-central/rev/535331914716
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
How big can this get?  I'm only seeing 10KB right now...
(Assignee)

Comment 20

6 years ago
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!
You need to log in before you can comment on or make changes to this bug.