Last Comment Bug 669904 - Count size of nsAttrValue
: Count size of nsAttrValue
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on: 677493 667883 677487 678820
Blocks: 663271
  Show dependency treegraph
 
Reported: 2011-07-07 07:59 PDT by Mounir Lamouri (:mounir)
Modified: 2012-06-12 19:12 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP Patch (4.66 KB, patch)
2011-07-07 09:55 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
WIP Patch (4.66 KB, patch)
2011-07-07 10:30 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1 (5.96 KB, patch)
2011-07-08 06:23 PDT, Mounir Lamouri (:mounir)
jst: review+
bzbarsky: feedback+
Details | Diff | Splinter Review
Patch v2 (6.20 KB, patch)
2011-08-09 05:25 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Inter diff between v1 and v2 (3.67 KB, patch)
2011-08-09 05:25 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2 (5.66 KB, patch)
2011-08-09 05:26 PDT, Mounir Lamouri (:mounir)
jst: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-07-07 07:59:45 PDT

    
Comment 1 Mounir Lamouri (:mounir) 2011-07-07 09:55:02 PDT
Created attachment 544523 [details] [diff] [review]
WIP Patch

WIP patch which crashes because of non-allowed memory access during nsAttrValue::SizeOf(). I will investigate that.
Comment 2 Mounir Lamouri (:mounir) 2011-07-07 10:30:33 PDT
Created attachment 544538 [details] [diff] [review]
WIP Patch

Crash free ;)
Comment 3 Mounir Lamouri (:mounir) 2011-07-08 06:23:09 PDT
Created attachment 544796 [details] [diff] [review]
Patch v1

I will try to open bugs for most of the TODO's after the review.
Comment 4 Boris Zbarsky [:bz] 2011-07-12 13:15:46 PDT
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, jst@mozilla.com) 2011-07-13 18:45:23 PDT
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) 2011-08-09 04:21:54 PDT
(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) 2011-08-09 05:25:15 PDT
Created attachment 551727 [details] [diff] [review]
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) 2011-08-09 05:25:53 PDT
Created attachment 551729 [details] [diff] [review]
Inter diff between v1 and v2
Comment 9 Mounir Lamouri (:mounir) 2011-08-09 05:26:39 PDT
Created attachment 551730 [details] [diff] [review]
Patch v2
Comment 10 Boris Zbarsky [:bz] 2011-08-09 06:12:28 PDT
> 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) 2011-08-11 04:23:27 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/96995a72c29d

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