270251 - nsSVGElement::UpdateContentStyleRule issues

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Boris Zbarsky [:bzbarsky]]

There are several issues I see with this method:

1) It should just bail early if mAttrsAndChildren.AttrCount() is 0
   (which will speed up creation of SVG elements once we do XBL for nodes not in
   documents).
2) Memory leaks.  |declaration| is leaked if getting a parser fails, eg
3) It should get parsers off the document's CSSLoader (and unset SVGMode before
   recycling them).  That would reduce the memory-thrashing significantly...

Comment 1

[Jonathan Watt [:jwatt]]

I could have a go at addressing these issues, although I don't know anything
about item 3.

Comment 2

[Boris Zbarsky [:bzbarsky]]

For item 3, compare code at
http://lxr.mozilla.org/seamonkey/source/content/html/style/src/nsDOMCSSAttrDeclaration.cpp#167
for example.  In the profile for bug 261974, about 2/3 of the time in
UpdateContentStyleRule (which is about 10% of total time on the testcase)
happens in the allocation and deallocation of parser objects...

Comment 3

[jpl24]

Attachment: update_patch_v1

-Adds the attribute count check.
-Fixes leaking declaration.
-Gets the CSS Parser from the document.

Comment 4

[jpl24]

bz could you take a quick look and see if this is what you wanted?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Sort of... The decl still leaks if InitializeEmpty() returns false.  There's no
reason to hold a strong ref to the CSSLoader.  And when you're done with the
parser, if you have a CSSLoader you should call RecycleParser on it.

Lastly, the code that does:

   NS_NewCSSStyleRule(getter_AddRefs(mContentStyleRule), nsnull, declaration);
   NS_ASSERTION(mContentStyleRule, "could not create contentstylerule");

is wrong.  That _can_ fail on OOM, for example.  See what other callers of
NS_NewCSSStyleRule in the tree do.  The memory management at
http://lxr.mozilla.org/seamonkey/source/content/html/style/src/nsDOMCSSAttrDeclaration.cpp#107
is correct, say...

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm not sure that the bail-if-no-attrs thing is usefull. This function should
only be called when we are resolving style on the element, which I didn't think
we did unless we were in the tree. So it won't happen until all attributes are
set at which point we need to do it anyway.

Isn't there a risk that we end up bailing (due to error or otherwise) before we
reach the last |parser->SetSVGMode(PR_FALSE)|. Wouldn't that be better to do in
the code that recycles the parser. So that as soon as the parser is recycled it
gets reset. Alternativly we could do it to all parser before returning them in
the  nsICSSLoader::GetParserFor function.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, i missed that there's a RecycleParser call. As long as you reset the svgmode
in relation to that you should be fine.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. i guess the 0-attrs-check does save us an nsCSSDeclaration allocation and
some other work for the elements that doesn't have any attributes. I don't
understand how it's related to xbl-binding though, feel free to enlighten me.

So if you want to save this work more aggressivly you could move (almost) all
the code that's before the for-loop into an |if (!parser)| block inside the
for-loop. That way we won't do all that work unless it's really needed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> which I didn't think we did unless we were in the tree.

That won't last.  I was profiling this in a build that does XBL binding
attachment when we create JS wrappers even if the node is not in a tree.  Doing
XBL bindings means resolving style for the element and looking at the computed
-moz-binding.  So we were hitting this code, with all the expensive CSS parser
creation, etc, from inside createElement() itself (from the classinfo code that
creates the wrapper, in fact).

I'm not sure whether it's cheaper to create an empty CSSDeclaration the first
time we need it or to check whether we need it every time (in the case when we
have a bunch of attributes, none mapped into style).

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Setting a bunch of non-stylerelated attributes shouldn't cause us to get into
neither WalkContentStyleRules nor UpdateContentStyleRule, so I don't think
leaving mContentStyleRule null will affect performance. 

Comment 11

[jpl24]

Sorry I've been slow on this one, I've got a patch ready to go but I had to
update and rebuild which slowed me down a bit. I'll upload it tonight after I
finish testing it.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Jonas, my point was the following:

Say we have no style-related attributes.  If we leave mContentStyleRule null,
then any time we do a style reresolve (which can happen for reasons _other_ than
setting style attributes, eg if we have :hover rules or other style in the tree
changing or whatever) we will have to walk all our attributes only to discover
that none of them matter.  So it may be faster to just create the empty decl
once in that case.  I don't know how SVG is typically used, so I don't know
whether this is an issue or not....

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

ah, i'm with you. Dunno which is best. If we leave a comment refering back to
this bug we can always change it in the future once we have some numbers.

Comment 14

[jpl24]

Attachment: update_patch_v2

Okay I think this fixes all of the issues Boris pointed out.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 168173 [details] [diff] [review]
update_patch_v2

>Index: src/nsSVGElement.cpp
>+  // Bail early if there are no child attributes.

Add something like: "If we moved the code below into the for-loop we could
avoid creating an mDeclaration even when there are non-style related
attributes. See bug 270251."

>+  nsIDocument* doc = GetOwnerDoc();
>+  if (!doc) {
>+    NS_ERROR("SVG element without owner document");
>+    return;
>+  }
>+  
>+  nsCOMPtr<nsIURI> baseURI = GetBaseURI();
>+  nsIURI *docURI = doc->GetDocumentURI();
>+  
>+  // Create the nsCSSDeclaration.
>   nsCSSDeclaration* declaration = new nsCSSDeclaration();

Make this an nsRefPtr<nsCSSDeclaration> instead. That way you won't manually
have to delete the declaration thus reducing risks of accidental leaks. That
way you can collapse the two if-statments below like before.

>+  // Try to fetch the CSS Parser from the document.
>+  nsICSSLoader* cssLoader(doc->GetCSSLoader());
>+  NS_ASSERTION(cssLoader, "Document with no CSS loader!");

Hmm.. is this really worth an assertion? As a general rule of thumb i don't
like asserting and handling as error. If something is known to happen in cases
valid enough that we want to handle it it doesn't deserve an asserion IMHO.

>+  nsresult rv = NS_OK; 
>+  if (cssLoader) {
>+    rv = cssLoader->GetParserFor(nsnull, getter_AddRefs(parser));
>+  } else {
>+      NS_NewCSSParser(getter_AddRefs(parser));

You need |rv = NS_New...| here

>+  if (NS_FAILED(rv)) {
>+    NS_WARNING("failed to get or create a css parser");
>+    declaration->RuleAbort();
>     return;
>+  }

Just replace this with NS_ENSURE_SUCCESS(rv, rv) since you don't need the
RuleAbort call.

>+  if (NS_FAILED(rv)) {
>+    NS_WARNING("could not create contentstylerule");
>+    declaration->RuleAbort();
>+  }

Use NS_WARN_IF_FALSE (or NS_ENSURE_SUCCESS, if we run out of memory we have
bigger problems then an unrecycled parser)

r=me with that

Comment 16

[jpl24]

I've fixed all comments except these two:

> Make this an nsRefPtr<nsCSSDeclaration> instead. That way you won't manually
> have to delete the declaration thus reducing risks of accidental leaks. That
> way you can collapse the two if-statments below like before.

I had wanted to do it this way but I think this won't work because
nsCSSDeclaration has a private destructor. When I tried it I got a compiler error: 

../../../../dist\include\xpcom\nsCOMPtr.h(200) : error C2876: 'nsCSSDeclaration'
: not all overloads are accessible

If there is a way to do this it'd be much better and less leak prone.

> Just replace this with NS_ENSURE_SUCCESS(rv, rv) since you don't need the
> RuleAbort call.
I couldn't figure out how to get this to work since updateContentStyleRule
returns void. Should this function return nsresult so that the caller, which
does return nsresult, can actually check if the function was successful? If this
is the right approach I can do that quickly.

Thanks for the quick review on the patch.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ugh, major bummer, seems like nsCSSDeclaration is special when it comes to
refcounting.

r=me stand with those to left as before.

Comment 18

[jpl24]

Attachment: update_patch_v3

-Adds comment about avoiding creating declaration.
-Add missing rv
-Remove incorrect assertion

r should carry over

Comment 19

[jpl24]

does SVG need sr? I couldn't find anything about it in the mozilla hacking
document. If not, if someone could check this in when they get the chance that'd
be great(assuming sicking doesn't object).

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

i'd prefer it if bz looked at the way the nsCSSDeclaration is handeled since
I've never seen it before.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 168581 [details] [diff] [review]
update_patch_v3

>Index: src/nsSVGElement.cpp
>+  if(mAttrsAndChildren.AttrCount() == 0)

Space before '(', please.

>   nsCSSDeclaration* declaration = new nsCSSDeclaration();

This is the way to go.	"refcounting" of CSSDeclaration objects is an
implementation detail of the style system that we are working on eliminating or
hiding.  They're not COM objects and don't do COM refcounting.

>+  nsICSSLoader* cssLoader(doc->GetCSSLoader());

I'd use '=' here...

sr=bzbarsky with these nits fixed.

Comment 22

[jpl24]

Sure, would whoever can check this in like a new patch or prefer to fix these
before committing?(Whatever is easier for you guys)

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

as a general rule of thumb always create a complete patch when someone is
checking in for you, that way a lot more people will be able to do it. This is
much less of an issue once you get checkin permissions.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, and thanks for doing this!! Fixing cleanup bugs like this is always appriciated!

Comment 25

[jpl24]

Attachment: update_patch_final

Sure, here's a final patch for check-in. I'm off to find a new bug.

Comment 26

[tor]

Checked in - thanks.

Checking in nsSVGElement.cpp;
/cvsroot/mozilla/content/svg/content/src/nsSVGElement.cpp,v  <--  nsSVGElement.cpp
new revision: 1.69; previous revision: 1.68
done