script: Add support for parsing CSS in parallel #40639
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🔨 Triggering try run (#19358341926) for Linux (WPT) |
Member
|
Did you run any tests to check the performance impact? |
|
Test results for linux-wpt from try job (#19358341926): Flaky unexpected result (49)
Stable unexpected results that are known to be intermittent (29)
|
|
✨ Try run (#19358341926) succeeded. |
Member
Author
|
I have not run performance tests and I suspect it depends on the hardware Servo is running on. The idea here is to add this support and then disable or enable it based on later performance testing. I'm very curious to hear about how this affects performance on embedded systems, for instance. |
TimvdLippe
approved these changes
Nov 14, 2025
Contributor
TimvdLippe
left a comment
There was a problem hiding this comment.
Overall this LGTM! I mainly have some nits and clarifying questions about code structure. Glad to see this land 😄
| // From <https://html.spec.whatwg.org/multipage/#link-type-stylesheet>: | ||
| // > A link element of this type is implicitly potentially render-blocking if the element | ||
| // > was created by its node document's parser. | ||
| if matches!(self.source, StylesheetContextSource::LinkElement) && owner.parser_inserted() { |
Contributor
There was a problem hiding this comment.
Nit: move this if into the other if so we only check owner.parser_inserted() once
Member
Author
There was a problem hiding this comment.
Oh, I meant to do this earlier, but I didn't get around to it. I've added an early return here.
| .expect("Stylesheet not loaded by <style> or <link> element!"); | ||
|
|
||
| if self.is_for_a_previous_generation(&element) { | ||
| self.decrement_load_and_render_blockers(owner, &document); |
Contributor
There was a problem hiding this comment.
Should we also return early here, since we shouldn't process it any longer?
Member
Author
There was a problem hiding this comment.
Yes! Thank you for spotting this. I meant to do an early return here. This would have caused all kinds of chaos in the future as-is.
| document.invalidate_stylesheets(); | ||
| } | ||
|
|
||
| self.decrement_load_and_render_blockers(owner, &document); |
Contributor
There was a problem hiding this comment.
If we don't early return above, then why do we decrement twice here?
Comment on lines
230
to
236
| let event = match any_failed { | ||
| true => atom!("error"), | ||
| false => atom!("load"), | ||
| }; |
Contributor
There was a problem hiding this comment.
Ultra-nit: why not use if instead of match on a boolean?
Suggested change
| let event = match any_failed { | |
| true => atom!("error"), | |
| false => atom!("load"), | |
| }; | |
| let event = if any_failed { atom!("error") } else { atom!("load") }; |
Member
Author
There was a problem hiding this comment.
I went with a match because it was one less line, but your approach is even better.
Member
Author
There was a problem hiding this comment.
Hrm. It seems that rustfmt doesn't like the single-line version, so maybe I'll stick with the match since it is one less line than this:
let event = match any_failed {
true => atom!("error"),
false => atom!("load"),
};
Comment on lines
293
to
294
| let mime: Mime = content_type.into_inner().into(); | ||
| mime.type_() == mime::TEXT && mime.subtype() == mime::CSS |
Contributor
There was a problem hiding this comment.
Nit: consider using
servo/components/shared/net/mime_classifier.rs
Lines 323 to 325 in 178e6b9
Member
Author
There was a problem hiding this comment.
Okay. I've made this public and reused it.
| // > external resource is not a supported style sheet type, the user agent must | ||
| // > instead assume it to be text/css. | ||
| if document.quirks_mode() == QuirksMode::Quirks && | ||
| document.url().origin() == self.url.origin() |
Contributor
There was a problem hiding this comment.
Why do we need to do this twice? Above we are checking final_url and here we are checking url. Can we consolidate these or remove one of the branches maybe?
Member
Author
There was a problem hiding this comment.
Looks like this is due to two PRs doing the same thing landing around the same time, but one originating from around 4 years ago. I landed the old one (after the new one) when I was trying to adopt old PRs. It seems I made a mistake in not removing the duplicate check. Since this existed before my change, I'll preserve it, but try to open a followup to fix it.
Here are the two PRs:
- Fix: Add support for stylesheet MIME type quirk in quirks mode #36338 (newer, landed first)
- Use spec compliant content-type extraction in more places and enable a
<stylesheet>quirk #28321 (older, I landed second and didn't notice the duplicate check)
Comment on lines
327
to
332
| let loader = match pref!(dom_parallel_css_parsing_enabled) { | ||
| true => { | ||
| ElementStylesheetLoader::Asynchronous(AsynchronousStylesheetLoader::new(&element)) | ||
| }, | ||
| false => ElementStylesheetLoader::Synchronous { element: &element }, | ||
| }; |
Contributor
There was a problem hiding this comment.
Same ultra-nit about matching booleans:
Suggested change
| let loader = match pref!(dom_parallel_css_parsing_enabled) { | |
| true => { | |
| ElementStylesheetLoader::Asynchronous(AsynchronousStylesheetLoader::new(&element)) | |
| }, | |
| false => ElementStylesheetLoader::Synchronous { element: &element }, | |
| }; | |
| let loader = if pref!(dom_parallel_css_parsing_enabled) { | |
| ElementStylesheetLoader::Asynchronous(AsynchronousStylesheetLoader::new(&element)) | |
| } else { | |
| ElementStylesheetLoader::Synchronous { element: &element } | |
| }; |
Member
Author
There was a problem hiding this comment.
This makes a lot of sense. I'll make the change.
This change is a rework of servo#22478, originally authored by @vimpunk. It adds parsing of CSS in parallel with the main script thread. The big idea here is that when the transfer of stylesheet bytes is finished, the actual parsing is pushed to a worker thread from the Stylo thread pool. This also applies for subsequent loads triggered by `@import` statements. The design is quite similar to the previous PR with a few significant changes: - Error handling works properly. The `CSSErrorReporter` is a crossbeam `Sender` and a `PipelineId` so it can be trivially cloned and sent to the worker thread. - Generation checking is done both before and after parsing, in order to both remove the race condition and avoid extra work when the generations do not match. - The design is reworked a bit to avoid code duplication, dropping added lines from 345 to 160. - Now that `process_response_eof` gives up ownership to the `FetchResponseListener`, this change avoids all extra copies. Co-authored-by: mandreyel <mandreyel@protonmail.com> Signed-off-by: Martin Robinson <mrobinson@igalia.com>
189b70b to
30c3b2d
Compare
|
🔨 Triggering try run (#19372634583) for Linux (WPT) |
|
|
|
🔨 Triggering try run (#19377936064) for Linux (WPT) |
3af54ed to
1311cce
Compare
- Cancel load blockers when exited early due to generation change - Make generation change more general by having the method check whether the element still contributes to the styling processing model in particular whether it is still connected to the DOM. Signed-off-by: Martin Robinson <mrobinson@igalia.com>
1311cce to
d873cc3
Compare
|
🔨 Triggering try run (#19386855605) for Linux (WPT) |
Member
Author
|
Hrm. So it seems that the old code was relying on "ignored" loads to finish in order to send events, depending on the order that things arrived in. We hit this path more with the extra step of asynchronicity. I think that this should be a bit more closely tied to the generation concept that the |
|
Test results for linux-wpt from try job (#19386855605): Flaky unexpected result (43)
Stable unexpected results that are known to be intermittent (31)
|
|
✨ Try run (#19386855605) succeeded. |
|
🔨 Triggering try run (#19387255095) for Linux (WPT) |
|
Test results for linux-wpt from try job (#19387255095): Flaky unexpected result (39)
Stable unexpected results that are known to be intermittent (29)
|
|
✨ Try run (#19387255095) succeeded. |
Loirooriol
added a commit
to Loirooriol/servo
that referenced
this pull request
Nov 24, 2025
After servo#40639, this variant holds a single value, and I don't think think using named fields is providing much. Signed-off-by: Oriol Brufau <obrufau@igalia.com>
Loirooriol
added a commit
to Loirooriol/servo
that referenced
this pull request
Nov 24, 2025
After servo#40639, this variant holds a single value, and I don't think think using named fields was providing much extra clarity. Signed-off-by: Oriol Brufau <obrufau@igalia.com>
Loirooriol
added a commit
to Loirooriol/servo
that referenced
this pull request
Nov 24, 2025
After servo#40639, this variant holds a single value, and I don't think think using named fields was providing much extra clarity. Signed-off-by: Oriol Brufau <obrufau@igalia.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change is a rework of #22478, originally authored by @vimpunk.
It adds parsing of CSS in parallel with the main script thread. The
big idea here is that when the transfer of stylesheet bytes is
finished, the actual parsing is pushed to a worker thread from the Stylo
thread pool. This also applies for subsequent loads triggered by
@importstatements.The design is quite similar to the previous PR with a few significant
changes:
CSSErrorReporteris a crossbeamSenderand aPipelineIdso it can be trivially cloned and sent tothe worker thread.
to both remove the race condition and avoid extra work when the
generations do not match.
lines from 345 to 160.
process_response_eofgives up ownership to theFetchResponseListener, this change avoids all extra copies.Testing: This shouldn't change observable behavior, so is covered
by existing tests.
Fixes: #20721
Closes: #22478