Closed Bug 1438974 Opened 2 years ago Closed 2 years ago

Always parse linked stylesheets asynchronously

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(9 files, 1 obsolete file)

33.47 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.84 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.06 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
15.56 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.65 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
10.71 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.42 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.00 KB, patch
bholley
: review+
Details | Diff | Splinter Review
3.02 KB, patch
smaug
: review+
Details | Diff | Splinter Review
I'm splitting these patches out of bug 1346988 to get them landed first, both to sniff out any issues and avoid bitrot.
MozReview-Commit-ID: 7XNu42NtITm
Attachment #8951747 - Flags: review?(bzbarsky)
MozReview-Commit-ID: 69fK5VAWrbj
Attachment #8951748 - Flags: review?(bzbarsky)
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
Attachment #8951749 - Flags: review?(bzbarsky)
MozReview-Commit-ID: A2BxoZjFOU1
Attachment #8951750 - Flags: review?(bzbarsky)
This will allow us to make the regular path fully asynchronous.

MozReview-Commit-ID: 6ZurtiNQPZK
Attachment #8951751 - Flags: review?(bzbarsky)
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
Attachment #8951752 - Flags: review?(bzbarsky)
MozReview-Commit-ID: GXLT5NakIop
Attachment #8951753 - Flags: review?(bzbarsky)
[ Triage 2017/02/20: P3 ]
Priority: -- → P3
My apologies for the lag here.  I was traveling too much of this week.  :(

I will get to this on Monday.
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.
Attachment #8951747 - Flags: review?(bzbarsky) → review+
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.
Attachment #8951748 - Flags: review?(bzbarsky) → review-
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.
Attachment #8951749 - Flags: review?(bzbarsky) → review+
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?
Attachment #8951750 - Flags: review?(bzbarsky) → review+
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.
Attachment #8951751 - Flags: review?(bzbarsky) → review+
Attachment #8951748 - Attachment is obsolete: true
Comment on attachment 8951752 [details] [diff] [review]
Part 6 - Make XBL stylesheet loading block onload on the bound doc. v1

r=me
Attachment #8951752 - Flags: review?(bzbarsky) → review+
Comment on attachment 8954497 [details] [diff] [review]
Part 2 - Add an explicit "being parsed" flag to SheetLoadData and remove mParsingDatas. v2

r=me
Attachment #8954497 - Flags: review+
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?
Attachment #8951753 - Flags: review?(bzbarsky) → review+
(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!
> 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.
> 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?
This fixes a failure in test_ext_contentScripts_register.js introduced
by async parsing.

MozReview-Commit-ID: JViFhpDX2k1
Attachment #8954602 - Flags: review+
MozReview-Commit-ID: 6mCk1PjStND
Attachment #8954621 - Flags: review?(bugs)
Attachment #8954621 - Flags: review?(bugs) → review+
Depends on: 1441896
Pushed by bholley@mozilla.com:
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
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
That seems pretty surprising to me, fwiw.  I would not expect any performance improvements from the changes thus far.
(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.
Could be, I guess.  Suggests that the test is broken in terms of what it's measuring...
NI snorp, who is presumably interested in making sure the mobile perf tests are actually measuring the right thing. Feel free to delegate.
Flags: needinfo?(snorp)
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.
Flags: needinfo?(snorp)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (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?
Flags: needinfo?(snorp)
(In reply to Bobby Holley (:bholley) from comment #34)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (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.
Flags: needinfo?(snorp) → needinfo?(bob)
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.
Flags: needinfo?(bob)
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.
Depends on: 1465388
Depends on: 1471508
You need to log in before you can comment on or make changes to this bug.