stylo: Optimize some FFI calls

RESOLVED FIXED in Firefox 54

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8832628 [details] [diff] [review]
Part 1 - Inline Gecko_IsHTMLElementInHTMLDocument. v1
Attachment #8832628 - Flags: review?(emilio+bugs)
(Assignee)

Comment 2

a year ago
Created attachment 8832629 [details] [diff] [review]
Part 2 - Inline common case parent access. v1
Attachment #8832629 - Flags: review?(emilio+bugs)
(Assignee)

Comment 5

a year ago
Created attachment 8832724 [details] [diff] [review]
Part 2 - Inline common case parent access. v2

Now with runtime verification in debug builds.
Attachment #8832724 - Flags: review?(emilio+bugs)
(Assignee)

Updated

a year ago
Attachment #8832629 - Attachment is obsolete: true
Attachment #8832629 - Flags: review?(emilio+bugs)
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?
Attachment #8832628 - Flags: review?(emilio+bugs) → review+
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 :)
Attachment #8832724 - Flags: review?(emilio+bugs) → review+
(Assignee)

Comment 8

a year ago
(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 10

a year ago
Pushed by bholley@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2d8ebcae5098
https://hg.mozilla.org/mozilla-central/rev/dcfaa9b629bb
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.