Last Comment Bug 584293 - Speed up .style on HTML and XUL elements
: Speed up .style on HTML and XUL elements
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: P1 normal (vote)
: mozilla2.0b4
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 569719 585745 628794
Blocks: domperf
  Show dependency treegraph
 
Reported: 2010-08-03 23:18 PDT by Boris Zbarsky [:bz]
Modified: 2016-07-14 19:16 PDT (History)
3 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta4+


Attachments
Fix (13.03 KB, patch)
2010-08-03 23:38 PDT, Boris Zbarsky [:bz]
peterv: review+
Details | Diff | Splinter Review
Updated to comments (15.51 KB, patch)
2010-08-04 10:32 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2010-08-03 23:18:22 PDT
As the summary says: make .style faster.  Measurements suggest the upcoming patch makes it 3.5x faster.

The change to nsXULElement::GetStyle overlaps with a similar change in bug 569719; hence the dependency.  Zack should land his stuff first.
Comment 1 Boris Zbarsky [:bz] 2010-08-03 23:38:57 PDT
Created attachment 462694 [details] [diff] [review]
Fix

Two notes:

1)  This doesn't handle SVG elements.  That's a different interface, but would be easy to do if we have a lot of free bits in the typemapping.  Do we?

2)  This is still slower than webkit.  Remaining time spent is 55% under xpc_qsXPCOMObjectToJsval (self time, time in NativeInterface2JSObject, time in FindInJSObjectScope, QI on the decl).  22% is spent in the GetStyle quickstub (a lot of this setting up the ToJsval function call with its many arguments).  5% is spent calling GetStyle on the element.  9% is spent in trace-generated code, and 6% in GetPropertyWithNativeGetter.
Comment 2 Peter Van der Beken [:peterv] 2010-08-04 07:30:32 PDT
Comment on attachment 462694 [details] [diff] [review]
Fix

>+nsIDOMCSSStyleDeclaration*
>+nsStyledElement::GetStyle(nsresult* retval)
> {
>+  nsXULElement* xulElement = nsXULElement::FromContent(this);
>+  if (xulElement) {
>+    nsresult rv = xulElement->EnsureLocalStyle();
>+    if (NS_FAILED(rv)) {

It would be nice if EnsureLocalStyle returned void (all it throws is OUT_OF_MEMORY?). Oh well.

>   nsGenericElement::nsDOMSlots *slots = GetDOMSlots();
>-  NS_ENSURE_TRUE(slots, NS_ERROR_OUT_OF_MEMORY);
>+  if (!slots) {

I don't think this can happen anymore?

>diff --git a/content/html/content/src/nsGenericHTMLElement.cpp b/content/html/content/src/nsGenericHTMLElement.cpp

>+  NS_IMETHOD GetStyle(nsIDOMCSSStyleDeclaration** aStyle) {

Brace on its own line.

>diff --git a/content/xul/content/src/nsXULElement.cpp b/content/xul/content/src/nsXULElement.cpp

>+  NS_IMETHOD GetStyle(nsIDOMCSSStyleDeclaration** aStyle) {

Same here.

The bitmap has 32 bits. Adding nsSVGStylableElement looks fine to me.
Comment 3 Boris Zbarsky [:bz] 2010-08-04 10:31:36 PDT
> all it throws is OUT_OF_MEMORY?

I think so, but I don't think it's following infallible-only codepaths yet.  Once it is, I'd hope static analysis would win for us.

> I don't think this can happen anymore?

Indeed.  Took that null-check out.

> Brace on its own line.

Done, both places.

I added nsSVGStylableElement.
Comment 4 Boris Zbarsky [:bz] 2010-08-04 10:32:23 PDT
Created attachment 462819 [details] [diff] [review]
Updated to comments
Comment 5 Boris Zbarsky [:bz] 2010-08-05 19:58:15 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/e7b3d37b2bbd
Comment 6 Boris Zbarsky [:bz] 2010-08-06 06:14:04 PDT
Some combination of this and bug 584287 gave us a 5% win on Dromaeo (DOM) on tinderbox on Linux and Mac.

Note You need to log in before you can comment on or make changes to this bug.