stylo: Don't maintain TreeMatchContext when using the Servo style system

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
7 months ago
2 months ago

People

(Reporter: bz, Assigned: bholley)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox53 wontfix, firefox54 wontfix, firefox55 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

7 months ago
Right now it's initialized unconditionally even when using the servo style system, afaict.  Is it actually _used_ for anything then?  If not, can we ifdef some of it out or something?
Yeah, we're not use the TreeMatchContext for anything there.
(Reporter)

Comment 2

7 months ago
OK.  I guess we can't entirely ifdef it out, because we need to support the Gecko mode too.  But maybe we can skip all its maintenance when in servo mode or something.
Priority: -- → P4
I'm seeing a fair amount of overhead related to this. Taking.
Assignee: nobody → bobbyholley
Priority: P4 → P1
Summary: stylo: Figure out what should happen with the TreeMatchContext in the nsFrameConstructorState → stylo: Don't maintain TreeMatchContext when using the Servo style system
Created attachment 8868966 [details] [diff] [review]
Part 1 - Stop accepting aTreeMatchContext in the ServoStyleSet methods. v1

This makes it clear it's unused.
Attachment #8868966 - Flags: review?(bzbarsky)
Created attachment 8868967 [details] [diff] [review]
Part 2 - Don't maintain a TreeMatchContext for Servo. v1
Attachment #8868967 - Flags: review?(bzbarsky)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43cfbb51f9d78e526a81c5fc7ec7db69e4155c6a
(Reporter)

Comment 7

2 months ago
Comment on attachment 8868966 [details] [diff] [review]
Part 1 - Stop accepting aTreeMatchContext in the ServoStyleSet methods. v1

r=me
Attachment #8868966 - Flags: review?(bzbarsky) → review+
(Reporter)

Comment 8

2 months ago
Comment on attachment 8868967 [details] [diff] [review]
Part 2 - Don't maintain a TreeMatchContext for Servo. v1

>+  TreeMatchContextHolder(nsIDocument* aDocument)

"explicit".  Might as well keep the static analysis CI happy.  ;)

>+  TreeMatchContext*         mTreeMatchContext;

This is in the frame constructor state.  Please document whether this is allowed to be null (I assume it can be).

>+    TreeMatchContext* aTreeMatchContext,

(In the CSSFC state ctor) Again, please document it can be null.

>+++ b/layout/style/StyleSetHandleInlines.h
>+    return AsGecko()->ResolveStyleFor(aElement, aParentContext, aMayCompute, *aTreeMatchContext);

Please assert aTreeMatchContext.

r=me
Attachment #8868967 - Flags: review?(bzbarsky) → review+

Comment 9

2 months ago
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fcf1321d8d67
Stop accepting aTreeMatchContext in the ServoStyleSet methods. r=bz
https://hg.mozilla.org/integration/autoland/rev/7e2357039f4a
Don't maintain a TreeMatchContext for Servo. r=bz
https://hg.mozilla.org/mozilla-central/rev/fcf1321d8d67
https://hg.mozilla.org/mozilla-central/rev/7e2357039f4a
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
I don't think we need to uplift this to 54, but I'd like to double check with you that's what you intend.
status-firefox53: affected → wontfix
status-firefox54: --- → fix-optional
Flags: needinfo?(bobbyholley)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11)
> I don't think we need to uplift this to 54, but I'd like to double check
> with you that's what you intend.

Er, no. What about this bug suggests that we would?
Flags: needinfo?(bobbyholley)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #12)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11)
> > I don't think we need to uplift this to 54, but I'd like to double check
> > with you that's what you intend.
> 
> Er, no. What about this bug suggests that we would?

The "status-firefox53: affected" field was set.  I believe Bugzilla sets it automatically under some conditions...?  (I can't seem to trigger it anymore, maybe something was changed.)
Relman triages through anything that was affected in current but earlier versions (that we know of) and fixed in nightly, as potential uplifts. I am double checking with bug assignees as I either wontfix or mark "fix optional" for 53 and 54. Some people have been happy for the reminder and have strongly wished to get in a last minute uplift. We catch a lot of issues this way.
status-firefox54: fix-optional → wontfix
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #14)
> Relman triages through anything that was affected in current but earlier
> versions (that we know of) and fixed in nightly, as potential uplifts. I am
> double checking with bug assignees as I either wontfix or mark "fix
> optional" for 53 and 54. Some people have been happy for the reminder and
> have strongly wished to get in a last minute uplift. We catch a lot of
> issues this way.

That makes sense, but I think we probably want a better filter for "affected in current but earlier versions". If we're basing that only on when a given bug was filed, we're going to have tons of false positives. This bug in particular is a performance improvement for stylo, which is still several cycles away from actually shipping.
You need to log in before you can comment on or make changes to this bug.