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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Cameron McCormack (:heycam)]

Yeah, we're not use the TreeMatchContext for anything there.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Bobby Holley (:bholley)]

I'm seeing a fair amount of overhead related to this. Taking.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Stop accepting aTreeMatchContext in the ServoStyleSet methods. v1

This makes it clear it's unused.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Don't maintain a TreeMatchContext for Servo. v1

Comment 6

[Bobby Holley (:bholley)]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fcf1321d8d67
https://hg.mozilla.org/mozilla-central/rev/7e2357039f4a

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 12

[Bobby Holley (:bholley)]

(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?

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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.)

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 15

[Bobby Holley (:bholley)]

(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.