Last Comment Bug 795128 - add memory reporting for attribute nodes and attribute maps
: add memory reporting for attribute nodes and attribute maps
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
:
Mentors:
Depends on:
Blocks: DarkMatter 701443
  Show dependency treegraph
 
Reported: 2012-09-27 14:36 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2012-09-30 18:01 PDT (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (4.04 KB, patch)
2012-09-27 15:50 PDT, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
bugs: review+
n.nethercote: feedback+
continuation: feedback+
Details | Diff | Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-27 14:36:55 PDT
These aren't hooked up, and apparently some sites use them a lot.
Comment 1 Andrew McCreight [:mccr8] 2012-09-27 14:40:20 PDT
The logs in bug 701443 had 28,000 attribute maps in them.
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-09-27 15:50:46 PDT
Created attachment 665687 [details] [diff] [review]
Patch
Comment 3 Nicholas Nethercote [:njn] 2012-09-27 16:12:28 PDT
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.
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-09-27 16:16:27 PDT
I had good examples to steal from ;-)
Comment 5 Andrew McCreight [:mccr8] 2012-09-27 16:46:19 PDT
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".
Comment 6 Olli Pettay [:smaug] 2012-09-27 17:23:35 PDT
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;
>
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-09-30 09:46:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/96f3db193c75
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-09-30 18:01:58 PDT
https://hg.mozilla.org/mozilla-central/rev/96f3db193c75

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