Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Depends on 1 bug)

Trunk
mozilla8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

No description provided.
Posted patch WIP Patch (obsolete) — Splinter Review
WIP patch which crashes because of non-allowed memory access during nsAttrValue::SizeOf(). I will investigate that.
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Posted patch WIP Patch (obsolete) — Splinter Review
Crash free ;)
Attachment #544523 - Attachment is obsolete: true
Posted patch Patch v1Splinter Review
I will try to open bugs for most of the TODO's after the review.
Attachment #544538 - Attachment is obsolete: true
Attachment #544796 - Flags: review?(jst)
Attachment #544796 - Flags: feedback?(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).
Attachment #544796 - Flags: feedback?(bzbarsky) → feedback+
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.
Attachment #544796 - Flags: review?(jst) → review+
Depends on: 677487
(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).
Depends on: 677493
Posted patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #551727 - Flags: review?(jst)
Posted patch Patch v2Splinter Review
Attachment #551727 - Attachment is obsolete: true
Attachment #551730 - Flags: review?(jst)
Attachment #551727 - Flags: review?(jst)
> I suppose ::StorageSize() shouldn't divide by mRefCount.

No, it should absolutely not.  ;)

> Anyway, this will be fixed in bug 677487

Sounds good.
Attachment #551730 - Flags: review?(jst) → review+
Whiteboard: [inbound]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/96995a72c29d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Depends on: 678820
You need to log in before you can comment on or make changes to this bug.