stylo: Consider parallelizing CSS parsing

RESOLVED FIXED in Firefox 61

Status

()

P4
normal
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla61
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 61+, firefox57 wontfix, firefox61 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
Johnny suggested this a few weeks ago. Given that CSS parsing is turning out to hot, we should investigate whether there are any bits of work we do that can be trivially parallelized on top of rayon.

Not urgent to look into this right now, just getting it on file as a potential perf win so that we don't forget about it.
It'd be quite hard taking @import and the loader into account, but the good part is that @import is only taken into account at the beginning of the stylesheet, so we could start parallelizing once we've passed the import section I believe.
Why is CSS parsing with stylo so much hotter than with Gecko's style system?
(Assignee)

Comment 3

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #2)
> Why is CSS parsing with stylo so much hotter than with Gecko's style system?

Well, in part because of dumb stuff, see bug 1331843. A lot of that has been fixed, and I haven't had time to remeasure yet.

However, the other piece here is that parallelizing stuff like parsing is a heck of a lot easier to do safely in Rust than in C++. So even once we get to perf parity with Gecko, we can potentially parallelize stuff that would have been too daunting to consider parallelizing in Gecko.
Multiple stylesheets can be parsed in parallel of each other trivially. Within one stylesheet it’s less easy. Several ideas come to mind, but I don’t know which of them if any would be an overall improvement. Before we try any of them, though, I think there’s still room to improve single-threaded parsing performance. I’ve written https://bugzilla.mozilla.org/show_bug.cgi?id=1331843#c16 about this.

https://github.com/haxney/speculate is a research project for a speculative tokenizer based on rust-cssparser. It makes multiple threads start tokenizing from arbitrary points in the Unicode input assuming that that point is not inside a comment or quoted string. If that assumption turns out to be wrong, these results are thrown away and that part of the input is tokenized again. Results seemed promising, but this was never integrated in a parser that does more than tokenizing. Also this is written in 2013 Rust and would need to be ported.

We could have a pipeline with a tokenizer thread sending tokens over a channel to another thread for the rest of parsing. (With crossbeam scoped threads to deal with lifetimes.)

Finally, we could have one thread find the start and end of each top-level rule in a stylesheet, and other threads tokenize/parse them in parallel. This top-level thing still needs to look for comments, quoted strings, and backslash escapes. But maybe that’s doable with less work (and tighter inner loops) than full tokenization. This would only be effective if we can find the end of a rule several times faster than we can fully parse it, which remains to be seen. Doing this more fine-grained than top-level rules (up to single declarations) is also possible, but more/smaller work items for a thread pool might mean more synchronization cost.
(Assignee)

Comment 5

2 years ago
Thanks for the detailed analysis Simon.

We should definitely look into parallelizing multiple stylesheets, which would be the lowest-hanging fruit if we could teach the Gecko machinery to handle async-parsed stylesheets.

My intuition tells me that we could probably identify top-level rules quite a bit faster than a full parse. The fast tokenizer could maintain pretty minimal state and would need to maintain it only when one of a very small number of characters is found. Would take some time to prove out and implement though.
So a few things:

1)  The "find the rule boundaries" idea was my first thought.  It's definitely worth testing.

2)  Gecko already has a concept of "async parsing" to some extent: obviously @import can happen async and a sheet is not considered fully ready until all of those happen.  Rejiggering this a bit to allow servo to return a "sheet is not done parsing" state would not be too hard. We'd need to support cloning of still-being-parsed sheets, but as long as the cloning is just a COW addref of the actual data structure still being parsed into, that should be ok.  We explicitly disallow access to rules of sheets which are still "being parsed" (including having the imports still loaded), so that wouldn't be an issue.  So if the servo side is ready, making the gecko side work should be pretty straightforward.  Maybe half a day's work for me.
(Assignee)

Comment 7

2 years ago
We just discussed this on IRC. I think we should do it - it would be a huge win in some cases for not very much work.
Priority: P4 → P1
(Assignee)

Updated

2 years ago
Assignee: nobody → bobbyholley
(Assignee)

Comment 8

2 years ago
Created attachment 8865063 [details] [diff] [review]
Always load stylesheets asynchronously when possible. WIP

MozReview-Commit-ID: LGWWTaMtUAW
(Assignee)

Comment 9

2 years ago
So, I wrote up a patch (attached) to see what happens when I try to always parse stylesheets async in Gecko (ignoring the servo bits). The idea would be to get that into the tree, and then use the newfound flexibility to parse things asynchronously for Servo.

However, it turned try orange [1]. I haven't had time to dig in yet, but before I do I thought it was worth asking bz whether he thinks that doing this ought to work, or whether I should change my approach.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe71b2b46abdafd8b61d0c2180ffe54260fea5e2
Flags: needinfo?(bzbarsky)
Doing this ought to work, subject, like I said, to fixing the cycle-detection in LoadChildSheet.

There's one obvious conceptual bug in the attached patch: it does nothing to keep onload from firing while the runnable is live.  Spot-checking a few tests that failed, they all failed because of that: the test's onload fired before the sheet was actually parsed and applied.  Adding BlockOnload()/UnblockOnload() calls as needed should help there.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 11

2 years ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #10)
> Doing this ought to work, subject, like I said, to fixing the
> cycle-detection in LoadChildSheet.

Is there actually anything that needs fixing? It seems to me that the important part is just that the data for the stylesheet currently being parsed is top of the stack (so that dependent loads can find it). I think my patch preserves that, no?

> 
> There's one obvious conceptual bug in the attached patch: it does nothing to
> keep onload from firing while the runnable is live.  Spot-checking a few
> tests that failed, they all failed because of that: the test's onload fired
> before the sheet was actually parsed and applied.  Adding
> BlockOnload()/UnblockOnload() calls as needed should help there.


Ah, good catch. Trying with that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=762052e789892fc91988ee4f1d8b761e73d2411d
> Is there actually anything that needs fixing? 

Hmm.  For purposes of this patch, probably not, since the parse is still completed sync while mParsingDatas is modified.  For actual parallelized parsing, more work will be needed.
(Assignee)

Comment 13

2 years ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #12)
> > Is there actually anything that needs fixing? 
> 
> Hmm.  For purposes of this patch, probably not, since the parse is still
> completed sync while mParsingDatas is modified.  For actual parallelized
> parsing, more work will be needed.

And to confirm, that work is just passing a ref to the parent stylesheet when loading the child, so that any child sheet loads _it_ triggers can find the appropriate chain to walk?
We pass the parent sheet to LoadChildSheet already.  But that's not enough to do the check we're doing via HaveAncestorDataWithURI right now.  Specifically, due to sheet sharing, our parent sheet may be effectively being loaded from multiple parents, and we want to check all of them (and all their ancestors) for the recursion.

That is, because of sharing, sheets do not form a forest when @import is used; instead we have a DAG in which a single node might have multiple "parents".  And we are trying to enforce the "A" part of "DAG", which means we need to check all things that are our "ancestors" in the DAG.
(Assignee)

Updated

2 years ago
Priority: P1 → P4
status-firefox57=wontfix unless someone thinks this bug should block 57
status-firefox57: --- → wontfix
(Assignee)

Comment 16

a year ago
In Austin, bz said he could take a look at this after the holidays. Boris, do you still have cycles? I think it should give us a noticeable win on Tp6.
Flags: needinfo?(bzbarsky)
I still plan to give the "parse separate sheets in parallel" thing a shot.  

I don't currently have plans to parallelize within-sheet.
(Assignee)

Comment 18

a year ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #17)
> I still plan to give the "parse separate sheets in parallel" thing a shot.  

Great!

> I don't currently have plans to parallelize within-sheet.

Yeah, I don't think that's really worth pursuing for now. Getting parallelism plus OMT is probably 80% of the benefit in practice.
(Assignee)

Comment 19

11 months ago
Boris and I discussed, and I'm going to take this. Starting with some basic refactoring:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9c52e44e6dc1c29a13d3db2b6c656098395a813
Flags: needinfo?(bzbarsky)
(Assignee)

Updated

11 months ago
Attachment #8865063 - Attachment is obsolete: true
(Assignee)

Comment 22

11 months ago
Green try run with the async pieces: https://treeherder.mozilla.org/#/jobs?repo=try&revision=30dfa3efc753b9272e331ddce241721f78ee5661

Next step is to teach servo to send the parse the rayon pool.
(Assignee)

Updated

11 months ago
Depends on: 1438974
(Assignee)

Comment 23

11 months ago
I've gotten reasonably far along with this. I still need to handle font-face/counter-style, namespace resolution, and error reporting. Current work at [1].

It's far enough along now that I was able to profile tp6-facebook [2]. The profile shows us getting totally slammed in atomization contention, which is tricky. I think I can probably fix the atom table machinery if I try hard enough, but it'll take some work.

This is all going to be too risky for 60, so I'm going to pause this for a little bit to help out with stylo-chrome, which is nearing the deadline.

[1] https://github.com/bholley/gecko/commits/station_q
[2] https://perfht.ml/2GDA4Ei
(Assignee)

Updated

11 months ago
Depends on: 1440824
(Assignee)

Updated

10 months ago
Depends on: 1450734
(Assignee)

Updated

10 months ago
No longer depends on: 1450734
(Assignee)

Updated

10 months ago
Depends on: 1449087
(Assignee)

Updated

10 months ago
Depends on: 1449068
(Assignee)

Updated

10 months ago
Depends on: 1451421
See Also: → bug 1452143
(Assignee)

Updated

10 months ago
Depends on: 1452143
We should probably mention this in the rel notes.
Keywords: dev-doc-needed
(Assignee)

Updated

9 months ago
Depends on: 1454021
(Assignee)

Updated

9 months ago
Depends on: 1454030
(Assignee)

Updated

9 months ago
Depends on: 1454460
(Assignee)

Updated

9 months ago
Depends on: 1454511
(Assignee)

Comment 26

9 months ago
Created attachment 8968594 [details] [diff] [review]
Parse sheets on the thread pool. v1

MozReview-Commit-ID: KddpGFdaqEe
Attachment #8968594 - Flags: review?(emilio)
Attachment #8968594 - Flags: review?(bzbarsky)
Comment on attachment 8968594 [details] [diff] [review]
Parse sheets on the thread pool. v1

Review of attachment 8968594 [details] [diff] [review]:
-----------------------------------------------------------------

Need to take a closer look tomorrow, but here are some questions :)

::: layout/style/ServoStyleSheet.cpp
@@ -201,5 @@
>                              nsIPrincipal* aSheetPrincipal,
>                              css::SheetLoadData* aLoadData,
>                              uint32_t aLineNumber,
> -                            nsCompatibility aCompatMode,
> -                            css::LoaderReusableStyleSheets* aReusableSheets)

Where did the LoaderReusableSheets go, why is it not needed?

@@ +214,5 @@
> +  //
> +  // (1) The pref is off.
> +  // (2) The browser is recording CSS errors (which parallel parsing can't handle).
> +  // (3) The stylesheet is a chrome stylesheet, since those can use -moz-bool-pref,
> +  //     which needs to access the pref service, which is not threadsafe.

Can we ensure that @-moz-bool-pref doesn't parse in normal UA sheets?

@@ +216,5 @@
> +  // (2) The browser is recording CSS errors (which parallel parsing can't handle).
> +  // (3) The stylesheet is a chrome stylesheet, since those can use -moz-bool-pref,
> +  //     which needs to access the pref service, which is not threadsafe.
> +
> +  bool allowParallel = StaticPrefs::layout_css_parsing_parallel();

This could benefit being on its own function, early returning instead of doing if (allowParallel).

@@ +217,5 @@
> +  // (3) The stylesheet is a chrome stylesheet, since those can use -moz-bool-pref,
> +  //     which needs to access the pref service, which is not threadsafe.
> +
> +  bool allowParallel = StaticPrefs::layout_css_parsing_parallel();
> +  if (allowParallel && aLoader->GetDocument() &&

I don't think this is enough to catch error reporting in shadow dom, since:

  https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/layout/style/ServoStyleSheet.cpp#280

Mind double-checking that? The fix may be just looking at GetContainingShadow() instead of just mDocument.

@@ +226,5 @@
> +
> +  if (allowParallel) {
> +    bool isChrome = false;
> +    aSheetURI->SchemeIs("chrome", &isChrome);
> +    allowParallel = !isChrome;

Can we ensure that UA sheets don't parse @-moz-bool-pref? Seems like a footgun.

::: servo/ports/geckolib/stylesheet_loader.rs
@@ +93,5 @@
> +            extra_data: extra_data,
> +            bytes: bytes,
> +            origin: origin,
> +            quirks_mode: quirks_mode,
> +            line_number_offset: line_number_offset,

nit: Use the shorthand syntax?

@@ +125,5 @@
> +        _context: &ParserContext,
> +        lock: &SharedRwLock,
> +        media: Arc<Locked<MediaList>>,
> +    ) -> Arc<Locked<ImportRule>> {
> +

nit: stray newline?

@@ +131,5 @@
> +        // string in the CssUrl, we just need to sort out the impedence mismatch
> +        // between Arc<String> and nsCString. This is low-priority though, because the
> +        // refcounting only really matters for data URIs, and those are unlikely to be
> +        // used for @import.
> +        let spec_str: nsCString = url.as_str().into();

Gecko understands these strings fwiw, see the IntoCssUrl stuff and such.
(Assignee)

Comment 28

9 months ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #27)
> Need to take a closer look tomorrow, but here are some questions :)

Early feedback here is definitely helpful, thanks.

> 
> ::: layout/style/ServoStyleSheet.cpp
> @@ -201,5 @@
> >                              nsIPrincipal* aSheetPrincipal,
> >                              css::SheetLoadData* aLoadData,
> >                              uint32_t aLineNumber,
> > -                            nsCompatibility aCompatMode,
> > -                            css::LoaderReusableStyleSheets* aReusableSheets)
> 
> Where did the LoaderReusableSheets go, why is it not needed?

It's an optional param, defaulting to null, and the only caller of this function doesn't pass it:

https://searchfox.org/mozilla-central/rev/bd326a2a6b729dc62a5aee57354a97ceac4d1dc0/layout/style/Loader.cpp#1628

I believe it's only used for places where we already take the sync path.

> @@ +214,5 @@
> > +  //
> > +  // (1) The pref is off.
> > +  // (2) The browser is recording CSS errors (which parallel parsing can't handle).
> > +  // (3) The stylesheet is a chrome stylesheet, since those can use -moz-bool-pref,
> > +  //     which needs to access the pref service, which is not threadsafe.
> 
> Can we ensure that @-moz-bool-pref doesn't parse in normal UA sheets?

UA stylesheets already need to load synchronously, so there's no parallelism there. This is just to handle document stylesheets in the XUL documents in the parent process. If anybody changes that somehow, they'll hit the main-thread assertion in Gecko_GetBoolPrefValue, so this isn't really a footgun.
> 
> @@ +216,5 @@
> > +  // (2) The browser is recording CSS errors (which parallel parsing can't handle).
> > +  // (3) The stylesheet is a chrome stylesheet, since those can use -moz-bool-pref,
> > +  //     which needs to access the pref service, which is not threadsafe.
> > +
> > +  bool allowParallel = StaticPrefs::layout_css_parsing_parallel();
> 
> This could benefit being on its own function, early returning instead of
> doing if (allowParallel).

Sure.

> 
> @@ +217,5 @@
> > +  // (3) The stylesheet is a chrome stylesheet, since those can use -moz-bool-pref,
> > +  //     which needs to access the pref service, which is not threadsafe.
> > +
> > +  bool allowParallel = StaticPrefs::layout_css_parsing_parallel();
> > +  if (allowParallel && aLoader->GetDocument() &&
> 
> I don't think this is enough to catch error reporting in shadow dom, since:
> 
>  
> https://searchfox.org/mozilla-central/rev/
> a30a60eadcff99d2a043241955d215dbeccad876/layout/style/ServoStyleSheet.cpp#280
> 
> Mind double-checking that? The fix may be just looking at
> GetContainingShadow() instead of just mDocument.

Hm, I'm not convinced. The _stylesheet_ may have a null mDocument, but I think the loader always has one, unless it's being torn down. In particular, the constructor requires it, and the only caller of the DocGroup constructor is LoadAdditionalSheet, which likely doesn't concern us here.

> Can we ensure that UA sheets don't parse @-moz-bool-pref? Seems like a
> footgun.

See above.

> 
> ::: servo/ports/geckolib/stylesheet_loader.rs
> @@ +93,5 @@
> > +            extra_data: extra_data,
> > +            bytes: bytes,
> > +            origin: origin,
> > +            quirks_mode: quirks_mode,
> > +            line_number_offset: line_number_offset,
> 
> nit: Use the shorthand syntax?

Done.

> 
> @@ +125,5 @@
> > +        _context: &ParserContext,
> > +        lock: &SharedRwLock,
> > +        media: Arc<Locked<MediaList>>,
> > +    ) -> Arc<Locked<ImportRule>> {
> > +
> 
> nit: stray newline?

Fixed.

> 
> @@ +131,5 @@
> > +        // string in the CssUrl, we just need to sort out the impedence mismatch
> > +        // between Arc<String> and nsCString. This is low-priority though, because the
> > +        // refcounting only really matters for data URIs, and those are unlikely to be
> > +        // used for @import.
> > +        let spec_str: nsCString = url.as_str().into();
> 
> Gecko understands these strings fwiw, see the IntoCssUrl stuff and such.

Ah, that's a nice bit of simplification. I'll bundle it into the patch.
(Assignee)

Comment 29

9 months ago
Created attachment 8968724 [details] [diff] [review]
Parse sheets on the thread pool. v2

MozReview-Commit-ID: KddpGFdaqEe
Attachment #8968724 - Flags: review?(emilio)
Attachment #8968724 - Flags: review?(bzbarsky)
(Assignee)

Updated

9 months ago
Attachment #8968594 - Attachment is obsolete: true
Attachment #8968594 - Flags: review?(emilio)
Attachment #8968594 - Flags: review?(bzbarsky)
(In reply to Bobby Holley (:bholley) from comment #28)
> > @@ +217,5 @@
> > > +  // (3) The stylesheet is a chrome stylesheet, since those can use -moz-bool-pref,
> > > +  //     which needs to access the pref service, which is not threadsafe.
> > > +
> > > +  bool allowParallel = StaticPrefs::layout_css_parsing_parallel();
> > > +  if (allowParallel && aLoader->GetDocument() &&
> > 
> > I don't think this is enough to catch error reporting in shadow dom, since:
> > 
> >  
> > https://searchfox.org/mozilla-central/rev/
> > a30a60eadcff99d2a043241955d215dbeccad876/layout/style/ServoStyleSheet.cpp#280
> > 
> > Mind double-checking that? The fix may be just looking at
> > GetContainingShadow() instead of just mDocument.
> 
> Hm, I'm not convinced. The _stylesheet_ may have a null mDocument, but I
> think the loader always has one, unless it's being torn down. In particular,
> the constructor requires it, and the only caller of the DocGroup constructor
> is LoadAdditionalSheet, which likely doesn't concern us here.

Hmm, there's a constructor that doesn't require a document. Indeed I pointed you to a line using it, and using it when the stylesheet document is nullptr...
(Assignee)

Comment 32

9 months ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #31)
> > Hm, I'm not convinced. The _stylesheet_ may have a null mDocument, but I
> > think the loader always has one, unless it's being torn down. In particular,
> > the constructor requires it, and the only caller of the DocGroup constructor
> > is LoadAdditionalSheet, which likely doesn't concern us here.
> 
> Hmm, there's a constructor that doesn't require a document. Indeed I pointed
> you to a line using it, and using it when the stylesheet document is
> nullptr...

Whoops, you're right.

However, that argument-less constructor is still only used in cases where it doesn't matter, AFAICT. All but one of the callers (including the one you linked to) call ParseSheetSync, so those cases don't need to decide about async parsing.

The only caller that doesn't force a sync parse is the nsStyleSheetService::PreloadSheetAsync stuff, but that's all non-document-associated anyway.
Comment on attachment 8968724 [details] [diff] [review]
Parse sheets on the thread pool. v2

Review of attachment 8968724 [details] [diff] [review]:
-----------------------------------------------------------------

Rust bits look fine to me. Thinking more about it the case I was concerned about shadow DOM maybe doesn't matter much, since we have a handle to the sheet, and the reparsing stuff is synchronous... Please double-check it works though.

::: layout/style/ServoBindings.cpp
@@ +2610,5 @@
> +                                                  media = Move(mediaList),
> +                                                  import = Move(importRule)]() mutable {
> +    MOZ_ASSERT(NS_IsMainThread());
> +    RefPtr<ServoStyleSheet> sheet =
> +      LoadStyleSheet(data->get()->mLoader, data->get()->mSheet->AsServo(), data->get(),

Maybe save data->get() in a local?

::: servo/ports/geckolib/glue.rs
@@ +1197,5 @@
> +        line_number_offset
> +    );
> +
> +    if let Some(thread_pool) = STYLE_THREAD_POOL.style_thread_pool.as_ref() {
> +        thread_pool.spawn(|| {

Huh, I would've expected you to need `move` :)

::: servo/ports/geckolib/stylesheet_loader.rs
@@ +122,5 @@
> +        lock: &SharedRwLock,
> +        media: Arc<Locked<MediaList>>,
> +    ) -> Arc<Locked<ImportRule>> {
> +        let stylesheet = ImportSheet::new_pending(self.origin, self.quirks_mode);
> +        let rule = Arc::new(lock.wrap(ImportRule { url: url.clone(), source_location, stylesheet }));

Please, do add a test for the case @import takes time to load and you try to access cssRules[0].media on the parent sheet or something, to ensure we don't panic because of the placeholder sheet.
Attachment #8968724 - Flags: review?(emilio) → review+
(Assignee)

Comment 34

9 months ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #33)
> Comment on attachment 8968724 [details] [diff] [review]
> Parse sheets on the thread pool. v2
> 
> Review of attachment 8968724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Rust bits look fine to me. Thinking more about it the case I was concerned
> about shadow DOM maybe doesn't matter much, since we have a handle to the
> sheet, and the reparsing stuff is synchronous... Please double-check it
> works though.
> 
> ::: layout/style/ServoBindings.cpp
> @@ +2610,5 @@
> > +                                                  media = Move(mediaList),
> > +                                                  import = Move(importRule)]() mutable {
> > +    MOZ_ASSERT(NS_IsMainThread());
> > +    RefPtr<ServoStyleSheet> sheet =
> > +      LoadStyleSheet(data->get()->mLoader, data->get()->mSheet->AsServo(), data->get(),
> 
> Maybe save data->get() in a local?

Sure.

> 
> ::: servo/ports/geckolib/glue.rs
> @@ +1197,5 @@
> > +        line_number_offset
> > +    );
> > +
> > +    if let Some(thread_pool) = STYLE_THREAD_POOL.style_thread_pool.as_ref() {
> > +        thread_pool.spawn(|| {
> 
> Huh, I would've expected you to need `move` :)

I thought the compiler could often infer it? |parse| consumes |self|, so it should be straightforward to infer.

> 
> ::: servo/ports/geckolib/stylesheet_loader.rs
> @@ +122,5 @@
> > +        lock: &SharedRwLock,
> > +        media: Arc<Locked<MediaList>>,
> > +    ) -> Arc<Locked<ImportRule>> {
> > +        let stylesheet = ImportSheet::new_pending(self.origin, self.quirks_mode);
> > +        let rule = Arc::new(lock.wrap(ImportRule { url: url.clone(), source_location, stylesheet }));
> 
> Please, do add a test for the case @import takes time to load and you try to
> access cssRules[0].media on the parent sheet or something, to ensure we
> don't panic because of the placeholder sheet.

I checked with bz - CSSOM will throw when trying to access stylesheets that haven't finished loading yet [1]. So we should be good here, and he thinks we don't need to verify this with an additional test.

[1] https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/layout/style/StyleSheet.cpp#696
Comment on attachment 8968724 [details] [diff] [review]
Parse sheets on the thread pool. v2

>+++ b/layout/style/ServoBindings.cpp
>+static already_AddRefed<ServoStyleSheet>
>+LoadStyleSheet(css::Loader* aLoader,

This is only used for @import, right?  Can we call this LoadImportedSheet or so?

>+Gecko_LoadStyleSheetAsync(mozilla::css::SheetLoadDataHolder* aParentData,

You shouldn't need the "mozilla::" bit there, right?

Can this be an already_AddRefed so we don't do extra refcounting?

I really wish we had some documentation for what these functions mean semantically.

>+++ b/layout/style/ServoStyleSheet.cpp
>+  // If this is a chrome stylesheet, it might use -moz-bool-pref, which needs to
>+  // access the pref service

I'm a bit confused by that.  Parsing -moz-bool-pref doesn't use the pref service (and it can be parsed in non-system sheets, afaict).  Evaluating it does need the pref service, but do we evaluate it at parse time?

If this condition does need to be here, please use dom::IsChromeURI instead of duplicating it, but not quite.  That's what the URLExtraData ctor uses.

That said, if this condition does need to be here, it doesn't look right.  eval_moz_bool_pref() will get the pref if either chrome rules are enabled or the sheet is a UA sheet (independent of the URL, in the latter case).

> ServoStyleSheet::ParseSheet(css::Loader* aLoader,
>-                            css::LoaderReusableStyleSheets* aReusableSheets)

Why is this being removed?  That should be explained in the commit message.

>+++ b/layout/style/ServoStyleSheet.h
>+  void FinishAsyncParse(already_AddRefed<RawServoStyleSheetContents> aSheetContents);

Please document.

r=me with the above, especially the bool pref bits, fixed.
Attachment #8968724 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 36

9 months ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #35)
> Comment on attachment 8968724 [details] [diff] [review]
> Parse sheets on the thread pool. v2
> 
> >+++ b/layout/style/ServoBindings.cpp
> >+static already_AddRefed<ServoStyleSheet>
> >+LoadStyleSheet(css::Loader* aLoader,
> 
> This is only used for @import, right?  Can we call this LoadImportedSheet or
> so?

Sure.

> 
> >+Gecko_LoadStyleSheetAsync(mozilla::css::SheetLoadDataHolder* aParentData,
> 
> You shouldn't need the "mozilla::" bit there, right?

Fixed.

> 
> Can this be an already_AddRefed so we don't do extra refcounting?

I don't think we can easily pass already_AddRefed for Gecko types over FFI without some boilerplate, so I'm going to skip that.

> 
> I really wish we had some documentation for what these functions mean
> semantically.
> 
> >+++ b/layout/style/ServoStyleSheet.cpp
> >+  // If this is a chrome stylesheet, it might use -moz-bool-pref, which needs to
> >+  // access the pref service
> 
> I'm a bit confused by that.  Parsing -moz-bool-pref doesn't use the pref
> service (and it can be parsed in non-system sheets, afaict).  Evaluating it
> does need the pref service, but do we evaluate it at parse time?

It does, via: https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/servo/components/style/stylesheets/rule_parser.rs#480

> If this condition does need to be here, please use dom::IsChromeURI instead
> of duplicating it, but not quite.  That's what the URLExtraData ctor uses.

Fixed.

> 
> That said, if this condition does need to be here, it doesn't look right. 
> eval_moz_bool_pref() will get the pref if either chrome rules are enabled or
> the sheet is a UA sheet (independent of the URL, in the latter case).

UA sheets are always parsed synchronously, so they'll never hit the parallel path. I'll add comments in various places explaining this.

> 
> > ServoStyleSheet::ParseSheet(css::Loader* aLoader,
> >-                            css::LoaderReusableStyleSheets* aReusableSheets)
> 
> Why is this being removed?  That should be explained in the commit message.

It's a dead optional arg. I'll mention it in the commit message.

> 
> >+++ b/layout/style/ServoStyleSheet.h
> >+  void FinishAsyncParse(already_AddRefed<RawServoStyleSheetContents> aSheetContents);
> 
> Please document.

Fixed.

Thanks for the reviews!
(Assignee)

Updated

9 months ago
Blocks: 1455115

Comment 37

9 months ago
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/98845151aee3
Parse sheets on the thread pool. r=bz,r=emilio

Comment 38

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/98845151aee3
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
relnote-firefox: --- → 61+
Update mentioned in Fx61 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/61#CSS
Keywords: dev-doc-needed → dev-doc-complete
See Also: → bug 1469859
You need to log in before you can comment on or make changes to this bug.