Open Bug 754592 Opened 9 years ago Updated 9 years ago

Lazily allocate conditional processing data

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

REOPENED

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

Attachments

(1 file, 1 obsolete file)

conditional processing attributes are rarely set and yet exist on most elements. We should allocate space for them lazily.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #623439 - Flags: review?(dholbert)
Attachment #623439 - Attachment is patch: true
Robert and I just took a look at this, and SVGStringList takes 16 bytes on 64-bit Mac. So we get a 48 byte saving with his patch for almost every SVG element. There is a slight overhead of the hash lookup for GetProperty(), but that's probably negligible compared to the other overhead during painting.
Comment on attachment 623439 [details] [diff] [review]
patch

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

::: content/svg/content/src/DOMSVGTests.cpp
@@ +251,5 @@
>  {
>    for (PRUint32 i = 0; i < ArrayLength(sStringListNames); i++) {
>      if (aAttribute == *sStringListNames[i]) {
> +      SVGStringList *stringList = GetStringListAttribute(i);
> +      if (stringList) {

I think you need a comment here noting why you don't remove the property. I.e. the DOMSVGStringList DOM wrappers depend on there always being a property object as long as they exist.

@@ +260,5 @@
>      }
>    }
>  }
>  
> +// Callback function, for freeing PRUint64 values stored in property table

Append "when the element goes away".

@@ +276,5 @@
> +SVGStringList*
> +DOMSVGTests::GetStringListAttribute(PRUint8 aAttrEnum)
> +{
> +  nsIAtom *attrName = GetAttrName(aAttrEnum);
> +  nsCOMPtr<nsSVGElement> element = do_QueryInterface(this);

I think you should const_cast |this| so you can avoid removing the |const| from all the methods.

Also, shouldn't this be an nsRefPtr?

@@ +289,5 @@
> +  if (stringListPtr) {
> +    return stringListPtr;
> +  }
> +  nsIAtom *attrName = GetAttrName(aAttrEnum);
> +  nsCOMPtr<nsSVGElement> element = do_QueryInterface(this);

nsRefPtr
Attachment #623439 - Flags: review?(dholbert) → review+
I've added comments and reverted the const changes but I can't change things to nsRefPtr as it doesn't compile.
Attachment #623439 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/92f2cf2f42b2
Flags: in-testsuite-
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/92f2cf2f42b2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 757718
Depends on: 761507
Depends on: CVE-2012-3970
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The backout changeset seems empty. I don't think it worked.
(In reply to Daniel Holbert [:dholbert] from comment #7)
> Backed out for the time being, for possibly causing bug 786895:
>   https://hg.mozilla.org/integration/mozilla-inbound/rev/f74d10518d51

(In reply to Robert Longson from comment #8)
> The backout changeset seems empty. I don't think it worked.

(Coming here from marking the merge) 

It is unfortunately empty.

On a related note:
Both Mak's backout script and sfink's port of it to a mercurial extension protect against this + do all the hard work for you (including getting rid of the unnecessary merge changesets and adding both cset and bug # to the commit message, giving a more sheriff-friendly format).
https://wiki.mozilla.org/User:Mak77
https://bitbucket.org/sfink/qbackout
(In reply to Ed Morley [:edmorley] from comment #9)
> (In reply to Robert Longson from comment #8)
> > The backout changeset seems empty. I don't think it worked.
> 
> (Coming here from marking the merge) 
> 
> It is unfortunately empty.

oops, thanks! Not sure what went wrong.

> On a related note:
> Both Mak's backout script and sfink's port of it to a mercurial extension
> protect against this + do all the hard work for you

Thanks -- good to know. Unfortunately, they can't do all the hard work in this case -- this isn't a straight "hg backout", because the code has changed quite a lot since this landed, so there's a good deal of manual merging required.  So, I imported (or rather, intended to import) a cleanly-applying backout patch that longsonr posted on bug 786895.

Anyway, I'll redo the backout later today (with more sanity checks :)).
Target Milestone: mozilla15 → ---
You need to log in before you can comment on or make changes to this bug.