Closed Bug 795128 Opened 12 years ago Closed 12 years ago

add memory reporting for attribute nodes and attribute maps

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Gavin, Assigned: khuey)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink])

Attachments

(1 file)

These aren't hooked up, and apparently some sites use them a lot.
The logs in bug 701443 had 28,000 attribute maps in them.
Blocks: 701443
Whiteboard: [MemShrink]
Assignee: nobody → khuey
Attached patch PatchSplinter Review
Attachment #665687 - Flags: review?(continuation)
Comment on attachment 665687 [details] [diff] [review]
Patch

Review of attachment 665687 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for following the existing style, e.g. documenting fields that you're not measuring.
Attachment #665687 - Flags: feedback+
I had good examples to steal from ;-)
Comment on attachment 665687 [details] [diff] [review]
Patch

Review of attachment 665687 [details] [diff] [review]:
-----------------------------------------------------------------

Looks okay to me, but somebody else should review it, given that I know nothing about what an attribute or an attribute map is. Sorry.

::: content/base/src/FragmentOrElement.cpp
@@ +617,5 @@
> +  // - mChildrenList
> +  // - mClassList
> +
> +  // The following members are not measured:
> +  // - mBindingParent / mControllers: because they're   non-owning

Extra spaces between "they're" and "non-owning".
Attachment #665687 - Flags: review?(continuation) → feedback+
Comment on attachment 665687 [details] [diff] [review]
Patch

>diff --git a/content/base/public/FragmentOrElement.h b/content/base/public/FragmentOrElement.h
>--- a/content/base/public/FragmentOrElement.h
>+++ b/content/base/public/FragmentOrElement.h
>@@ -343,16 +343,18 @@ public:
>   {
>   public:
>     nsDOMSlots();
>     virtual ~nsDOMSlots();
> 
>     void Traverse(nsCycleCollectionTraversalCallback &cb, bool aIsXUL);
>     void Unlink(bool aIsXUL);
> 
>+    size_t SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf) const;
>+
>     /**
>      * The .style attribute (an interface that forwards to the actual
>      * style rules)
>      * @see nsGenericHTMLElement::GetStyle
>      */
>     nsCOMPtr<nsICSSDeclaration> mStyle;
> 
>     /**
>diff --git a/content/base/src/FragmentOrElement.cpp b/content/base/src/FragmentOrElement.cpp
>--- a/content/base/src/FragmentOrElement.cpp
>+++ b/content/base/src/FragmentOrElement.cpp
>@@ -593,16 +593,40 @@ FragmentOrElement::nsDOMSlots::Unlink(bo
>     NS_IF_RELEASE(mControllers);
>   mChildrenList = nullptr;
>   if (mClassList) {
>     mClassList->DropReference();
>     mClassList = nullptr;
>   }
> }
> 
>+size_t
>+FragmentOrElement::nsDOMSlots::SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf) const
>+{
>+  size_t n = aMallocSizeOf(this);
>+
>+  if (mAttributeMap) {
>+    n += mAttributeMap->SizeOfIncludingThis(aMallocSizeOf);
>+  }
>+
>+  // Measurement of the following members may be added later if DMD finds it is
>+  // worthwhile:
>+  // - Superclass members (nsINode::nsSlots)
>+  // - mStyle
>+  // - mDataSet
>+  // - mSMILOverrideStyle
>+  // - mSMILOverrideStyleRule
>+  // - mChildrenList
>+  // - mClassList
>+
>+  // The following members are not measured:
>+  // - mBindingParent / mControllers: because they're   non-owning
>+  return n;
>+}
>+
> FragmentOrElement::FragmentOrElement(already_AddRefed<nsINodeInfo> aNodeInfo)
>   : nsIContent(aNodeInfo)
> {
> }
> 
> FragmentOrElement::~FragmentOrElement()
> {
>   NS_PRECONDITION(!IsInDoc(),
>@@ -1933,11 +1957,19 @@ FragmentOrElement::FireNodeRemovedForChi
>        child = child->GetNextSibling()) {
>     nsContentUtils::MaybeFireNodeRemoved(child, this, doc);
>   }
> }
> 
> size_t
> FragmentOrElement::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const
> {
>-  return nsIContent::SizeOfExcludingThis(aMallocSizeOf) +
>-         mAttrsAndChildren.SizeOfExcludingThis(aMallocSizeOf);
>+  size_t n = 0;
>+  n += nsIContent::SizeOfExcludingThis(aMallocSizeOf);
>+  n += mAttrsAndChildren.SizeOfExcludingThis(aMallocSizeOf);
>+
>+  nsDOMSlots* slots = GetExistingDOMSlots();
>+  if (slots) {
>+    n += slots->SizeOfIncludingThis(aMallocSizeOf);
>+  }
>+
>+  return n;
> }
>diff --git a/content/base/src/nsDOMAttributeMap.cpp b/content/base/src/nsDOMAttributeMap.cpp
>--- a/content/base/src/nsDOMAttributeMap.cpp
>+++ b/content/base/src/nsDOMAttributeMap.cpp
>@@ -496,8 +496,28 @@ nsDOMAttributeMap::Count() const
> }
> 
> uint32_t
> nsDOMAttributeMap::Enumerate(AttrCache::EnumReadFunction aFunc,
>                              void *aUserArg) const
> {
>   return mAttributeCache.EnumerateRead(aFunc, aUserArg);
> }
>+
>+size_t
>+AttrCacheSizeEnumerator(const nsAttrKey& aKey,
>+                        const nsRefPtr<nsDOMAttribute>& aValue,
>+                        nsMallocSizeOfFun aMallocSizeOf,
>+                        void* aUserArg)
>+{
>+  return aMallocSizeOf(aValue.get());
>+}
>+
>+size_t
>+nsDOMAttributeMap::SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf) const
>+{
>+  size_t n = aMallocSizeOf(this);
>+  n += mAttributeCache.SizeOfExcludingThis(AttrCacheSizeEnumerator,
>+                                           aMallocSizeOf);
>+
>+  // NB: mContent is non-owning and thus not counted.
>+  return n;
>+}
>diff --git a/content/base/src/nsDOMAttributeMap.h b/content/base/src/nsDOMAttributeMap.h
>--- a/content/base/src/nsDOMAttributeMap.h
>+++ b/content/base/src/nsDOMAttributeMap.h
>@@ -154,16 +154,18 @@ public:
>     }
> #endif
> 
>     return static_cast<nsDOMAttributeMap*>(aSupports);
>   }
> 
>   NS_DECL_CYCLE_COLLECTION_CLASS(nsDOMAttributeMap)
> 
>+  size_t SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf) const;
>+
> private:
>   Element *mContent; // Weak reference
> 
>   /**
>    * Cache of nsDOMAttributes.
>    */
>   AttrCache mAttributeCache;
>
Attachment #665687 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/96f3db193c75
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.