The default bug view has changed. See this FAQ.

Count size of nsAttrValue

RESOLVED FIXED in mozilla8

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
5 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)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
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.
Assignee: nobody → mounir
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
Created attachment 544538 [details] [diff] [review]
WIP Patch

Crash free ;)
Attachment #544523 - Attachment is obsolete: true
(Assignee)

Comment 3

6 years ago
Created attachment 544796 [details] [diff] [review]
Patch v1

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+
(Assignee)

Updated

6 years ago
Depends on: 677487
(Assignee)

Comment 6

6 years ago
(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).
(Assignee)

Updated

6 years ago
Depends on: 677493
(Assignee)

Comment 7

6 years ago
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.
Attachment #551727 - Flags: review?(jst)
(Assignee)

Comment 8

6 years ago
Created attachment 551729 [details] [diff] [review]
Inter diff between v1 and v2
(Assignee)

Comment 9

6 years ago
Created attachment 551730 [details] [diff] [review]
Patch v2
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.

Updated

6 years ago
Attachment #551730 - Flags: review?(jst) → review+
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
(Assignee)

Comment 11

6 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/96995a72c29d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8

Updated

6 years ago
Depends on: 678820
You need to log in before you can comment on or make changes to this bug.