XUL shouldn't always traverse all the prototypes

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Noticed this while doing something else...
I think we could optimize prototype traversing, if prototype is owned
by some not-to-be-traversed content object.

Adding the following to NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsXULElement,
                                                  nsGenericElement)
doesn't work because then something gets released too early:
nsIDocument* currentDoc = tmp->GetCurrentDoc();
if (currentDoc && nsCCUncollectableMarker::InGeneration(
                        currentDoc->GetMarkedCCGeneration())) {
  return NS_OK;
}
Posted patch like thisSplinter Review
This makes ownership of prototypes easier to understand, and
makes code less error-prone, because refcounting is handled using
nsRefPtr, nor explicit AddRef/Release
This reduces calls to nsXULPrototypeNode's Traverse. In a simple case when
just one window is open, number of calls drop from ~900 to ~300.

Mochitest or chrome don't show any leaks.
Assignee: jag → Olli.Pettay
Attachment #341098 - Flags: review?(peterv)
Comment on attachment 341098 [details] [diff] [review]
like this

>diff --git a/content/xul/content/src/nsXULElement.cpp b/content/xul/content/src/nsXULElement.cpp

>@@ -2713,28 +2719,28 @@ nsXULPrototypeElement::Serialize(nsIObje

>     // Now write children
>-    rv |= aStream->Write32(PRUint32(mNumChildren));
>-    for (i = 0; i < mNumChildren; i++) {
>-        nsXULPrototypeNode* child = mChildren[i];
>+    rv |= aStream->Write32(PRUint32(mChildren.Length()));
>+    for (i = 0; i < mChildren.Length(); i++) {
>+        nsRefPtr<nsXULPrototypeNode> child = mChildren[i];

I'd do
        nsXULPrototypeNode* child = mChildren[i].get();

>@@ -2798,30 +2804,27 @@ nsXULPrototypeElement::Deserialize(nsIOb

>+    PRUint32 numChildren = PRInt32(number);

Why do you need that cast? number is PRUint32.

>diff --git a/content/xul/content/src/nsXULElement.h b/content/xul/content/src/nsXULElement.h

> class nsXULPrototypeElement : public nsXULPrototypeNode

>+          mChildren(0),

No need for this.

> #ifdef NS_BUILD_REFCNT_LOGGING
>     virtual const char* ClassName() { return "nsXULPrototypeElement"; }
>     virtual PRUint32 ClassSize() { return sizeof(*this); }
> #endif

Please remove these too.

> 
>     virtual void ReleaseSubtree()
>     {
>-      if (mChildren) {
>-        for (PRInt32 i = mNumChildren-1; i >= 0; i--) {
>-          if (mChildren[i])
>+        for (PRInt32 i = mChildren.Length() - 1; i >= 0; i--) {
>+          if (mChildren[i].get())
>             mChildren[i]->ReleaseSubtree();
>         }

Looks like this is now overindented.
Attachment #341098 - Flags: review?(peterv) → review+
> >+    PRUint32 numChildren = PRInt32(number);
> 
> Why do you need that cast? number is PRUint32.
That cast is actually needed, because of what and how is stored in
XULcache.
(In reply to comment #2)
> > #ifdef NS_BUILD_REFCNT_LOGGING
> >     virtual const char* ClassName() { return "nsXULPrototypeElement"; }
> >     virtual PRUint32 ClassSize() { return sizeof(*this); }
> > #endif
Addref/release use those.
(In reply to comment #2)
> Looks like this is now overindented.
Actually, not-enough-indented, because xul uses 4 spaces.
Posted patch +commentsSplinter Review
I could have asked sr
Attachment #342049 - Flags: superreview?(peterv)
Attachment #342049 - Flags: superreview?(peterv) → superreview+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.