1335863 - stylo: Optimize some FFI calls

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

I stress-profiled selector matching, and some FFI calls showed up that we can inline. These are in the range of 0.7%-1.4% of selector matching overhead each, which isn't a game changer, but nothing to sneeze at either.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Inline Gecko_IsHTMLElementInHTMLDocument. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Inline common case parent access. v1

Comment 3

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e4b5e2f9eefc1eca53138724a74d279bee1bdb6

Comment 4

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=add54df4624b8e7d4d543a8853f6ce39b3085f53

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Inline common case parent access. v2

Now with runtime verification in debug builds.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8832628 [details] [diff] [review]
Part 1 - Inline Gecko_IsHTMLElementInHTMLDocument. v1

Review of attachment 8832628 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoBindings.cpp
@@ -169,5 @@
>    return aElement->StyleState().ServoValue();
>  }
>  
>  bool
> -Gecko_IsHTMLElementInHTMLDocument(RawGeckoElementBorrowed aElement)

We could keep this in debug builds and compare the result, though maybe it's not worth the churn? What do you think?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8832724 [details] [diff] [review]
Part 2 - Inline common case parent access. v2

Review of attachment 8832724 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsIContentInlines.h
@@ +53,5 @@
> +  // Check if we want the flattened parent for style, and the node is the root
> +  // of a native anonymous content subtree parented to the document's root element.
> +  if (Type == nsIContent::eForStyle && aNode->IsContent() &&
> +      aNode->AsContent()->IsRootOfNativeAnonymousSubtree() &&
> +      aNode->OwnerDoc()->GetRootElement() == aNode->GetParentNode())

nit: I think you can use GetParent() here (moving the parent declaration above this condition). The root element is always content.

::: layout/style/ServoBindings.cpp
@@ +74,5 @@
>  
> +#ifdef DEBUG
> +bool
> +Gecko_FlattenedTreeParentIsParent(RawGeckoNodeBorrowed aNode)
> +{

heh, so you did it here :)

Comment 8

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> >  bool
> > -Gecko_IsHTMLElementInHTMLDocument(RawGeckoElementBorrowed aElement)
> 
> We could keep this in debug builds and compare the result, though maybe it's
> not worth the churn? What do you think?

I think this one is straightforward enough that it doesn't need runtime verification.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> nit: I think you can use GetParent() here (moving the parent declaration
> above this condition). The root element is always content.

I ended up rearranging it in a slightly different way to avoid both content-checking the parent as well as checking the bit (since the bit implies that the node is an element).

Comment 9

[Bobby Holley (:bholley)]

Decided to give this a regular gecko try push too: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60d6954898c84f08826feec35e52458688deabbf

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d8ebcae5098
Inline Gecko_IsHTMLElementInHTMLDocument. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcfaa9b629bb
Inline common case parent access. r=emilio

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2d8ebcae5098
https://hg.mozilla.org/mozilla-central/rev/dcfaa9b629bb