754592 - Lazily allocate conditional processing data

Status: REOPENED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

conditional processing attributes are rarely set and yet exist on most elements. We should allocate space for them lazily.

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

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 3

[Jonathan Watt [:jwatt]]

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

Comment 4

[Robert Longson [:longsonr]]

Attachment: address review comments

I've added comments and reverted the const changes but I can't change things to nsRefPtr as it doesn't compile.

Comment 5

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92f2cf2f42b2

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/92f2cf2f42b2

Comment 7

[Daniel Holbert [:dholbert]]

Backed out for the time being, for possibly causing bug 786895:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/f74d10518d51

Comment 8

[Robert Longson [:longsonr]]

The backout changeset seems empty. I don't think it worked.

Comment 9

[Ed Morley [:emorley]]

(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

Comment 10

[Daniel Holbert [:dholbert]]

(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 :)).

Comment 11

[Daniel Holbert [:dholbert]]

Backed out for reals (not empty this time):
 https://hg.mozilla.org/integration/mozilla-inbound/rev/f867845a9956

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f867845a9956

Comment 13

[Daniel Holbert [:dholbert]]

Backed out on Aurora / Beta, too (with a=akeybl granted over on bug 786895):
 https://hg.mozilla.org/releases/mozilla-aurora/rev/e8503a9a261c
 https://hg.mozilla.org/releases/mozilla-beta/rev/b8cc5e2e8ed5

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 15

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.