99797 - style.color parses too much (DHTML optimisation)

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Hixie (not reading bugmail)]

To set a property at the moment, we parse way more than we need to (just follow
the code path from |style.border| -- we first create a full declaration, then parse
it, looking for which property it is, whether it is !important, etc). We should
just expose a wrapper for CSSParserImpl::ParseProperty, and call that directly.

Comment 1

[Johnny Stenback (:jst)]

Reassigning to pierre to work on an API to the style system that lets us do
this. Pierre, if this is not your problem, please reassign.

Comment 2

[Pierre Saslawsky]

Reassigned to Boris who volunteered to fix it

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase to test this on

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Another testcase to test on

Comment 6

[Boris Zbarsky [:bzbarsky]]

On that first testcase (setting style.top 5000 times) my opt build went from

Avg: 3.268
Stddev: 0.040

to
Avg: 3.080
Stddev: 0.042

an speedup of 5.8% or so.

Reviews?

Comment 7

[Johnny Stenback (:jst)]

Holy &^*!, CSSLoaderImpl::RecycleParser() screws up the refcount on the parsers
that it recycles, wanna fix that while you're at it, if you do you could make
your nsICSSParser*'s nsCOMPtr<nsICSSParser>'s?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Sure thing.  That behavior _did_ strike me as very odd... :)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch fixing RecycleParser() wackiness and spreading nsCOMPtr goodness everywhere

Comment 10

[Johnny Stenback (:jst)]

I would suggest loosing the redundancy in code that does things like this:

+    result = mContent->GetDocument(*getter_AddRefs(doc));
     if (NS_SUCCEEDED(result) && (nsnull != doc)) {
-      doc->GetBaseURL(baseURI);
+      doc->GetBaseURL(*getter_AddRefs(baseURI));

There's no need to check for "NS_SUCCEEDED(result)" and "nsnull != doc" (which
should sumply be "doc", simply checking for "doc" is all you need. There's a
bunch of raw QueryyInterface() calls that were converted into
do_QueryInterface(ptr, &result), loose the result where it's not needed,
checking for success by checking that the nsCOMPtr is non-null is enough.

- In CSSParserImpl::ParseProperty():

+  nsString* str = new nsString(aPropValue);
+  if (nsnull == str) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }

why not simply if (!str)?

- In nsDOMCSSDeclaration.cpp:

  return aDecl->RemoveProperty(aPropName,
                               nsDependentString(&nullChar, PRUint32(0)));

is faster and less code than using a nsAutoString for the empty string.

- In nsDOMCSSDeclaration.h, use const nsAReadableString& and not const
nsAString& for consistency, they are the same thing.

Other than that the changes look good, but please attach a new patch and I'll
have one more look.

Comment 11

[Boris Zbarsky [:bzbarsky]]

>  return aDecl->RemoveProperty(aPropName,
                               nsDependentString(&nullChar, PRUint32(0)));
>
> is faster and less code than using a nsAutoString for the empty string.

Unfortunately, RemoveProperty wants a writable string that it then proceeds to
write to.... so it seems to make sense to pass in an nsAutoString...

Attaching patch that addresses the other comments

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: updated patch. Get rid of redundant error-checking

Comment 13

[Brendan Eich [:brendan]]

+  if (!aValue.Length()) {
+    // If the new value of the property is an empty string we remove the

Please use IsEmpty() instead of fetching the length, which could (if we do lazy
length computation) become costly.  It's also more expressive, as jag points out
(cc'ing him).

/be

Comment 14

[Boris Zbarsky [:bzbarsky]]

Oops.  That's what I get for copying and pasting code.  Length() usage fixed,
attaching new patch.  Filed bug 100649 on similar usages in other parts of
related code.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch without offending Length() call

Comment 16

[Johnny Stenback (:jst)]

- In CSSParserImpl::ParseProperty(), no need for " = null" for nsCOMPtr's:

+  nsCOMPtr<nsIUnicharInputStream> input = nsnull;

- In DOMCSSDeclarationImpl::ParsePropertyValue():

+          result = htmlContainer->GetCSSLoader(*getter_AddRefs(cssLoader));
+        }
+      }
+    }
+  }
+  if (cssLoader) {
+    result = cssLoader->GetParserFor(nsnull, getter_AddRefs(cssParser));
+  }
+  else {
+    result = NS_NewCSSParser(getter_AddRefs(cssParser));
+  }

'result' from htmlContainer->GetCSSLoader() is never used, either check it and
return it, or remove it (I vote for remove it), same goes for the same (or very
similar) code in DOMCSSDeclarationImpl::ParseDeclaration().

With that, sr=jst

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK.  Attaching patch with that change.

An interesting question...  The testcase that modifies the style rule (instead
of modifying the style attribute) runs about twice as fast as the other one.  I
wonder what the reasons for that are...

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch addressing last set of jst's comments

Comment 19

[Boris Zbarsky [:bzbarsky]]

Interesting... I did two profiles (mozilla on each of the testcases).  Neither
has an obvious processor hogging function in it.  The only major difference that
I can see that would account for the factor of 2 speed difference is:

Total hit count: 3104

Total hit count: 1597

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Profile on testcase 1

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Profile on testcase 2

Comment 22

[Marc Attinasi]

Comment on attachment 50037 [details] [diff] [review]
Patch addressing last set of jst's comments

r=attinasi - but, I think you should get review from the style module owner or a peer: dbaron or pierre or glazman I think would be good.

Comment 23

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

attachment 50037 [details] [diff] [review] seems fine to me, although I have a few general concerns
(perhaps they could be addressed in a later patch, or maybe you prefer to
address them now):
 * nsICSSParser::ParseProperty is intended to be used externally -- so does the
aHint parameter make sense?  It looks like it's never used.  Would it ever be? 
(Could it replace CaptureChange?  Is CaptureChange used here?)
 * You've now duplicated code so that code that used to be almost identical in
two places is now duplicated in 4 places.  Any chance of condensing this?

Comment 24

[Boris Zbarsky [:bzbarsky]]

> so does the aHint parameter make sense?  It looks like it's never used.

Sure it is.  It's passed to the document in the StyleRuleChanged() or
AttributeChanged() calls

> You've now duplicated code so that code that used to be almost identical in
> two places is now duplicated in 4 places.  Any chance of condensing this?

Hmm.  I suppose I could add some helper functions to get the CSS parser to those
classes (collapsing the number of copies of code back to 2).  I think I'd prefer
to do it as part of my patch to bug 95336 which will slightly rework the
parser-getting mechanism....

Comment 25

[Pierre Saslawsky]

Agreed: bug 95336 is the place to do the cleanup and make sure we are not missing 
anything before re-using the parser.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Marc says he's happy with David and Pierre's comments.  Checked in on trunk. 
Marking fixed.