Lazily allocate conditional processing data

REOPENED
Assigned to

Status

()

Core
SVG
REOPENED
5 years ago
5 years ago

People

(Reporter: Robert Longson, Assigned: Robert Longson)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
conditional processing attributes are rarely set and yet exist on most elements. We should allocate space for them lazily.
(Assignee)

Comment 1

5 years ago
Created attachment 623439 [details] [diff] [review]
patch
Assignee: nobody → longsonr
Attachment #623439 - Flags: review?(dholbert)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 4

5 years ago
Created attachment 625075 [details] [diff] [review]
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.
Attachment #623439 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Depends on: 757718
(Assignee)

Updated

5 years ago
Depends on: 761507
Depends on: 760996
Depends on: 786895
Backed out for the time being, for possibly causing bug 786895:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/f74d10518d51
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

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

Comment 9

5 years ago
(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 :)).
Backed out for reals (not empty this time):
 https://hg.mozilla.org/integration/mozilla-inbound/rev/f867845a9956
https://hg.mozilla.org/mozilla-central/rev/f867845a9956
Target Milestone: mozilla15 → ---
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
You need to log in before you can comment on or make changes to this bug.