1438974 - Always parse linked stylesheets asynchronously

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Bobby Holley (:bholley)]

I'm splitting these patches out of bug 1346988 to get them landed first, both to sniff out any issues and avoid bitrot.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Pass an explicit parent SheetLoadData for child stylesheet loads. v1

MozReview-Commit-ID: 7XNu42NtITm

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add an explicit "being parsed" flag to SheetLoadData and remove mParsingDatas. v1

MozReview-Commit-ID: 69fK5VAWrbj

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Separate the Gecko and Servo parsing paths in the Loader. v1

This will allow us to avoid touching the old style system when making
the Servo parses asynchronous, and make it easier to drop the old code
when the time comes.

MozReview-Commit-ID: 5em0PMnb5Nw

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Shuffle some stuff around in ServoStyleSheet::ParseSheet. v1

MozReview-Commit-ID: A2BxoZjFOU1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 5 - Separate synchronous stylesheet parsing into a separate path. v1

This will allow us to make the regular path fully asynchronous.

MozReview-Commit-ID: 6ZurtiNQPZK

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 6 - Make XBL stylesheet loading block onload on the bound doc. v1

This doesn't affect most XBL stylesheets since they load from chrome://
URIs which get loaded synchronously, but it does affect
test_media_queries_dynamic_xbl.html, which would otherwise fail when we
make stylesheet loading asynchronous.

MozReview-Commit-ID: 31y47Z0Oxak

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 7 - Make Servo stylesheet parsing async by default. v1

MozReview-Commit-ID: GXLT5NakIop

Comment 8

[Jet Villegas (inactive)]

[ Triage 2017/02/20: P3 ]

Comment 9

[Boris Zbarsky [:bzbarsky]]

My apologies for the lag here.  I was traveling too much of this week.  :(

I will get to this on Monday.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8951747 [details] [diff] [review]
Part 1 - Pass an explicit parent SheetLoadData for child stylesheet loads. v1

>+++ b/layout/style/Loader.h

Please document the new arg to LoadChildSheet where the other @param bits are.

>+++ b/layout/style/ServoStyleSheet.h
>+             css::SheetLoadData* aLoadData,

Document.

>+  // The data describing the parent load, if applicable.
>+  RefPtr<SheetLoadData> mParentLoadData;

This is a bit confusing.  Is this describing the load of the sheet being parsed (yes, it is!) or the load of its parent?

This should probably just be called mLoadData and documented as describing the load of the sheet currently being parsed....

>+++ b/layout/style/nsCSSParser.h
>+                      mozilla::css::SheetLoadData* aParentLoadData,

Document.  And is that named right, both here and in the .cpp?  It's _this_ sheet's load data, right?

>+++ b/servo/ports/geckolib/glue.rs
>+    load_data: *mut SheetLoadData,

Should be documented somewhere.

r=me with the above fixed.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8951748 [details] [diff] [review]
Part 2 - Add an explicit "being parsed" flag to SheetLoadData and remove mParsingDatas. v1

You need to initialize mIsBeingParsed in the various SheetLoadData constructors.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8951749 [details] [diff] [review]
Part 3 - Separate the Gecko and Servo parsing paths in the Loader. v1

>+                                  aLoadData->mSheet->GetSheetURI(),
>+                                  aLoadData->mSheet->GetBaseURI(),
>+                                  aLoadData->mSheet->Principal(),

Can use aSheet for those.

>+    aLoadData->mSheet->GetSheetURI(),
>+    aLoadData->mSheet->GetBaseURI(),
>+    aLoadData->mSheet->Principal(),

And here.

r=me, though it might be nice to also document the two new functions in the header.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8951750 [details] [diff] [review]
Part 4 - Shuffle some stuff around in ServoStyleSheet::ParseSheet. v1

r=me if you document the new function (e.g. why it exists).  Presumably for main-thread tasks that need to happen at end of parse?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8951751 [details] [diff] [review]
Part 5 - Separate synchronous stylesheet parsing into a separate path. v1

>+      aLoadData->mSheet->GetSheetURI(),
>+      aLoadData->mSheet->GetBaseURI(),
>+      aLoadData->mSheet->Principal(),

Again, aSheet.

Might be good to document how ServoStyleSheet::ParseSheetSync and ServoStyleSheet::ParseSheet differ.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add an explicit "being parsed" flag to SheetLoadData and remove mParsingDatas. v2

MozReview-Commit-ID: 69fK5VAWrbj

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8951752 [details] [diff] [review]
Part 6 - Make XBL stylesheet loading block onload on the bound doc. v1

r=me

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8954497 [details] [diff] [review]
Part 2 - Add an explicit "being parsed" flag to SheetLoadData and remove mParsingDatas. v2

r=me

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8951753 [details] [diff] [review]
Part 7 - Make Servo stylesheet parsing async by default. v1

I think it would be clearer for the commit message to say that this makes stylesheet parse _completion_ async by default.  

Why do we need the nsresult bits for the MozPromise if it's always resolved with NS_OK?  Is it just that we can't have a void-typed MozPromise?

We should probably have some assertions about how ParseSheeet() is not called more than once on a given sheet, because the promise machinery won't work correctly in that case, right?

Comment 19

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #18)
> Comment on attachment 8951753 [details] [diff] [review]
> Part 7 - Make Servo stylesheet parsing async by default. v1
> 
> I think it would be clearer for the commit message to say that this makes
> stylesheet parse _completion_ async by default.  

Done.
 
> Why do we need the nsresult bits for the MozPromise if it's always resolved
> with NS_OK?  Is it just that we can't have a void-typed MozPromise?

Yeah. The convention is to use /* Dummy = */ bool, so I'll switch to that.

> We should probably have some assertions about how ParseSheeet() is not
> called more than once on a given sheet, because the promise machinery won't
> work correctly in that case, right?

Strictly speaking, the promise machinery handles this case fine, and it's the other stylesheet logic that would likely not work right for multiple Parse calls (but that's nothing new). I'll assert mParsePromise.IsEmpty() to be safe.

Thanks for the reviews!

Comment 20

[Bobby Holley (:bholley)]

Try push, since it's been a while: https://treeherder.mozilla.org/#/jobs?repo=try&revision=50c8bd6dd2541eb8152d6ad7d24412410be28d60

Comment 21

[Boris Zbarsky [:bzbarsky]]

> the promise machinery handles this case fine

Well, it'll no-op the Resolve() call and never call the Then() callback.

People can in fact do multiple ParseSheet calls right now, via ServoStyleSheet::ReparseSheet.  And it works, I assume.  So this is a sort-of-new restriction on the non-sync parse which happens to be OK because we never do it more than once anyway.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> and never call the Then() callback.

Er, nevermind, I guess it will call the callback fine.  And given that so far the actual parse is sync that will even be ok, kinda.

I did have one other thought: should we be using some event target associated with the loader's doc, when we have a doc?

Comment 23

[Bobby Holley (:bholley)]

Attachment: Part 6.1 - Fix WebExtensions to block layout for pending sheets.r=kmag

This fixes a failure in test_ext_contentScripts_register.js introduced
by async parsing.

MozReview-Commit-ID: JViFhpDX2k1

Comment 24

[Bobby Holley (:bholley)]

Attachment: Part 8 - Dispatch to the appropriate event target. v1

MozReview-Commit-ID: 6mCk1PjStND

Comment 25

[Bobby Holley (:bholley)]

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51fe30371a87
Add an explicit "being parsed" flag to SheetLoadData and remove mParsingDatas. r=bz
https://hg.mozilla.org/integration/autoland/rev/40c130e79ebb
Separate the Gecko and Servo parsing paths in the Loader. r=bz
https://hg.mozilla.org/integration/autoland/rev/4c1996495064
Shuffle some stuff around in ServoStyleSheet::ParseSheet. r=bz
https://hg.mozilla.org/integration/autoland/rev/76745afd3072
Separate synchronous stylesheet parsing into a separate path. r=bz
https://hg.mozilla.org/integration/autoland/rev/84a95007749b
Make XBL stylesheet loading block onload on the bound doc. r=bz
https://hg.mozilla.org/integration/autoland/rev/a100ab83e7a9
Fix WebExtensions to block layout for pending sheets. r=kmag
https://hg.mozilla.org/integration/autoland/rev/dd55c414f2ad
Make Servo stylesheet parsing completion async by default. r=bz
https://hg.mozilla.org/integration/autoland/rev/023a3a83d667
Dispatch to the appropriate event target. r=smaug

Comment 27

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/51fe30371a87
https://hg.mozilla.org/mozilla-central/rev/40c130e79ebb
https://hg.mozilla.org/mozilla-central/rev/4c1996495064
https://hg.mozilla.org/mozilla-central/rev/76745afd3072
https://hg.mozilla.org/mozilla-central/rev/84a95007749b
https://hg.mozilla.org/mozilla-central/rev/a100ab83e7a9
https://hg.mozilla.org/mozilla-central/rev/dd55c414f2ad
https://hg.mozilla.org/mozilla-central/rev/023a3a83d667

Comment 28

[Ionuț Goldan [:igoldan]]

This brought some performance improvements on Android!

== Change summary for alert #11822 (as of Wed, 28 Feb 2018 16:43:33 GMT) ==

Improvements:

  9%  remote-nytimes android-4-4-armv7-api16 opt      2,018.34 -> 1,830.98
  3%  remote-nytimes android-4-2-armv7-api16 opt      3,349.14 -> 3,248.38

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11822

Comment 29

[Boris Zbarsky [:bzbarsky]]

That seems pretty surprising to me, fwiw.  I would not expect any performance improvements from the changes thus far.

Comment 30

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #29)
> That seems pretty surprising to me, fwiw.  I would not expect any
> performance improvements from the changes thus far.

Perhaps the testcase has a complicated stylesheet in the <body> that doesn't block layout, but generally arrives early enough in our testing environment? By making it available later, we may end up applying the stylesheet in a later flush outside the measurement interval.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Could be, I guess.  Suggests that the test is broken in terms of what it's measuring...

Comment 32

[Bobby Holley (:bholley)]

NI snorp, who is presumably interested in making sure the mobile perf tests are actually measuring the right thing. Feel free to delegate.

Comment 33

[James Willcox (:snorp) ([e-mail]) (he/him)]

The test measures the throbber time, which although imperfect, is at least an easily measurable thing that relates to user perception of a completed page load. This may change soon, though, as we are considering changes to measure first paint or "time to stable frame" instead.

Comment 34

[Bobby Holley (:bholley)]

(In reply to James Willcox (:snorp) ([e-mail]) (PTO until April 9) from comment #33)
> The test measures the throbber time, which although imperfect, is at least
> an easily measurable thing that relates to user perception of a completed
> page load. This may change soon, though, as we are considering changes to
> measure first paint or "time to stable frame" instead.

My point here is that the testcase is racey, because it presumably has a big stylesheet in the <body> somewhere. The testcase would be more useful and reliable if we moved the stylesheet to <head>. Where does the testcase live?

Comment 35

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Bobby Holley (:bholley) from comment #34)
> (In reply to James Willcox (:snorp) ([e-mail]) (PTO until April
> 9) from comment #33)
> > The test measures the throbber time, which although imperfect, is at least
> > an easily measurable thing that relates to user perception of a completed
> > page load. This may change soon, though, as we are considering changes to
> > measure first paint or "time to stable frame" instead.
> 
> My point here is that the testcase is racey, because it presumably has a big
> stylesheet in the <body> somewhere. The testcase would be more useful and
> reliable if we moved the stylesheet to <head>. Where does the testcase live?

Ah, that's possible. The test is an old copy of nytimes.com, but I don't know where it lives. Adding Bob Clary who manages this.

Comment 36

[Bob Clary [:bc] (inactive)]

It used to live in a special git repo until IT got rid of it. It currently is just passed around via zip file. I can make the change if you like and if autophone's results still matter.

Comment 37

[Bobby Holley (:bholley)]

I had Bob send me the file and took a look. Nothing obvious unfortunately. There's one big stylesheet and it appears in index.html before the <body>, so I'd think it should properly block load. There may be some JS fiddling around with things, but I'm not going to dive into that unless somebody asks me to. Realistically we should update the pageset anyway.