669904 - Count size of nsAttrValue

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: WIP Patch

WIP patch which crashes because of non-allowed memory access during nsAttrValue::SizeOf(). I will investigate that.

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: WIP Patch

Crash free ;)

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

I will try to open bugs for most of the TODO's after the review.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Some things that worry me a bit:

1)  For eStringBase (both places) this can be an overestimate because the buffer
    can be shared.  Is that OK?  Would it make sense to report the string
    storage used divided by the refcount?  ;)
2)  eAtomBase is _definitely_ overcounting unless we're holding the only ref to
    the atom.  It might be better to add a memory reporter for the atom table
    instead.  This applies to the atom array case too.

The rest of my worries are covered by your todos.

And yes, I think we should count inline style as part of the DOM.

And probably file a followup for memory reporting for style system.... (some is covered by the presshell arenas, but some is not).

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 544796 [details] [diff] [review]
Patch v1

r=jst, but I agree with what bz said. We should do something separate for the atom table and only count the size of the pointer here, and deal with the string sharing issue as best as we can.

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> Some things that worry me a bit:
> 
> 1)  For eStringBase (both places) this can be an overestimate because the
> buffer
>     can be shared.  Is that OK?  Would it make sense to report the string
>     storage used divided by the refcount?  ;)

I suppose ::StorageSize() shouldn't divide by mRefCount. Though, we could have a getter for mRefCount or make nsDomMemoryReporter a friend of nsStringBuffer.
Anyway, this will be fixed in bug 677487, taking the risk of multiple counting for the moment.

> 2)  eAtomBase is _definitely_ overcounting unless we're holding the only ref
> to
>     the atom.  It might be better to add a memory reporter for the atom table
>     instead.  This applies to the atom array case too.

bug 677491

> And probably file a followup for memory reporting for style system.... (some
> is covered by the presshell arenas, but some is not).

Looks like you opened this bug already (bug 671299).

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Re-asking for a review to make sure the changes where correct.
I'm going to attach an inter-diff to make the review easier.

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Inter diff between v1 and v2

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I suppose ::StorageSize() shouldn't divide by mRefCount.

No, it should absolutely not.  ;)

> Anyway, this will be fixed in bug 677487

Sounds good.

Comment 11

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/96995a72c29d