New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Implement parallel CSS parsing #22478

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@mandreyel
Copy link
Contributor

mandreyel commented Dec 16, 2018

Opening for discussion.

The basic idea is to spawn a parser thread on a global thread-pool (like in Gecko) with an ThreadSafeStylesheetLoader (instead of the previous StylesheetLoader), used when encountering an @import rule to send a task to script thread from the parser thread, which simply instantiates an async fetch on the corresponding document. After the parsing is done, another task is sent to script thread to do the post-parsing steps, like setting the parsed stylesheet on its link element (if applicable), firing the load event, etc.

A problem was that verifying the request generation id before parsing (still on the script thread) introduced a race condition, so I moved that to the post-parse task run on the script thread. This (seems to have) solved the race condition, but it differs in semantics from the sync parsing in that it's now no longer checked before doing the parsing, but after, which incurs an unnecessary parse overhead if the sheet is not applicable (I think).

Also, there seems to be an occasional time-out in one of the CSS tests, and the code needs some cleaning up.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #20721 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because existing CSS tests should cover them.

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Dec 16, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/htmllinkelement.rs, components/script/stylesheet_loader.rs
  • @KiChjang: components/script/dom/htmllinkelement.rs, components/script/stylesheet_loader.rs
  • @emilio: components/style/Cargo.toml, components/style/lib.rs, components/style/servo/global_thread_pool.rs, components/style/servo/mod.rs
@highfive

This comment has been minimized.

Copy link

highfive commented Dec 16, 2018

warning Warning warning

  • These commits modify style and script code, but no tests are modified. Please consider adding a test!
@mandreyel

This comment has been minimized.

Copy link
Contributor

mandreyel commented Dec 16, 2018

r? @emilio

@highfive highfive assigned emilio and unassigned nox Dec 16, 2018

@emilio
Copy link
Member

emilio left a comment

So, it looks generally pretty good! I haven't gone through it entirely (it's been only a quick skim), but I have a few comments, specially regarding the code duplication with the thread-pool.

In any case this is looking great, nice work!

enum ThreadSafeStylesheetSource {
// Same as with `StylesheetSource`, we put the media list in an
// option so we can take it out without having to clone it.
// TODO(mandreyel): Is there a better way to make this `Send`?

This comment has been minimized.

@emilio

emilio Dec 16, 2018

Member

Why is MediaList not send? Looks to me it should be.

This comment has been minimized.

@mandreyel

mandreyel Dec 17, 2018

Contributor

You're right, it is send... I must have been confused by an unrelated error message. I'm glad I don't need to add another heap allocation.


lazy_static! {
/// The global style thread pool instance.
pub static ref STYLE_THREAD_POOL: StyleThreadPool = {

This comment has been minimized.

@emilio

emilio Dec 16, 2018

Member

Hmm, servo already has a thread-pool for layout-related tasks, we should avoid having a new one. That one is created in layout_thread/lib.rs (see LayoutThread::new). Though looks to me that a thread-pool per document is way overkill.

@jdm would you be fine to move to a model where we have a layout thread-pool per process, defined in style? I'm moderately sure that a threadpool per process is not going to fly on pages with 10+ iframes or such.

That way we could share the code with Gecko, and it'd certainly make this kind of patch way easier, since these loads are triggered from script, and right now the thread-pool owner is the layout thread, which doesn't necessarily have the same lifetime as the script thread.

We could still keep the run layout in parallel only if it's worth it logic in the layout thread of course.

This comment has been minimized.

@jdm

jdm Dec 16, 2018

Member

Yes, that sounds sensible.

This comment has been minimized.

@mandreyel

mandreyel Dec 17, 2018

Contributor

Hmm, I think this might be better as a separate PR to isolate changes. What do you think?

This comment has been minimized.

@emilio

emilio Dec 17, 2018

Member

Yeah, that'd be great.

This comment has been minimized.

@mandreyel

mandreyel Dec 17, 2018

Contributor

Alright, I'll open an issue for it once this is merged.

This comment has been minimized.

@emilio

emilio Dec 17, 2018

Member

Actually I meant doing it the other way around, first ensuring that we have a single thread-pool, instead of duplicating all this code. I just did that in #22487, I needed to stop debugging graphics stuff for an hour or so :)

This comment has been minimized.

@mandreyel

mandreyel Dec 19, 2018

Contributor

Oh, just saw this. Yeah, that sounds reasonable, I'll rebase once that's merged :)

// We also need to invalidate a document's stylesheets if this was an
// import rule load, not just in the case of top-level sheet loads.
if self.invalidate_stylesheets {
document.invalidate_stylesheets();

This comment has been minimized.

@emilio

emilio Dec 16, 2018

Member

So, this is a pre-existing problem, but I don't think we can get away with just invalidating stylesheets if the request succeeds. Doesn't that mean that doing something like:

<link href="valid-sheet.css" onload="this.href = 'invalid-sheet.css';">

Will keep applying valid-sheet.css? We should do the set_stylesheet call and such unconditionally. Anyhow this is pre-existing so instead of fixing it here we should file an issue and address that in a followup patch.

This comment has been minimized.

@emilio

emilio Dec 16, 2018

Member

Unless this is the issue you're running into with that test, in which case we'll need to fix it probably! Or we could mark it as failing for now pointing to this bug.

This comment has been minimized.

@mandreyel

mandreyel Dec 17, 2018

Contributor

I haven't run into issues since doing the changes described earlier, but I won't lie I might be a little too tired right now to understand what the pre-existing problem is, so I may be misunderstanding what you meant :) Why invalidate previous sheets and set the new stylesheet if there was no stylesheet that could be parsed?

This comment has been minimized.

@emilio

emilio Dec 17, 2018

Member

No, it's alright, let's not add behavior changes, and fix that in a followup if it's a problem.

/// here. This function sets up the load context, channels to communicate fetch
/// responses, the request object, and issues an async fetch on the element's
/// document.
fn start_stylesheet_load<'a>(

This comment has been minimized.

@emilio

emilio Dec 16, 2018

Member

nit: No need for <'a>, can just use &HTMLElement below.

This comment has been minimized.

@mandreyel

mandreyel Dec 17, 2018

Contributor

Oh, accidentally left that in!

let document = document_from_node(elem);
let gen = elem
.downcast::<HTMLLinkElement>()
.map(HTMLLinkElement::get_request_generation_id);

This comment has been minimized.

@emilio

emilio Dec 16, 2018

Member

Hmm.... This is really suspicious, <style> elements should also have a generation id, otherwise we could get import loads wrong, right?

This comment has been minimized.

@emilio

emilio Dec 16, 2018

Member

Ditch this, the comment above is wrong.

@mandreyel

This comment has been minimized.

Copy link
Contributor

mandreyel commented Dec 17, 2018

@emilio Impressive response time :D Thanks for the review, I'll address your points today if I get home in time!

@emilio

This comment has been minimized.

Copy link
Member

emilio commented Dec 17, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 17, 2018

⌛️ Trying commit da5ec86 with merge d828396...

bors-servo added a commit that referenced this pull request Dec 17, 2018

Auto merge of #22478 - mandreyel:parallel-css-parsing, r=<try>
[WIP]  Implement parallel CSS parsing

<!-- Please describe your changes on the following line: -->
Opening for discussion.

The basic idea is to spawn a parser thread on a global thread-pool (like in Gecko) with an `AsyncStylesheetLoader` (instead of the previous `StylesheetLoader`), used when encountering an `@import` rule to send a task to script thread from the parser thread, which simply instantiates an async fetch on the corresponding document. After the parsing is done, another task is sent to script thread to do the post-parsing steps, like setting the parsed stylesheet on its link element (if applicable), firing the load event, etc.

A problem was that verifying the request generation id before parsing (still on the script thread) introduced a race condition, so I moved that to the post-parse task run on the script thread. This (seems to have) solved the race condition, but it differs in semantics from the sync parsing in that it's now no longer checked before doing the parsing, but after, which incurs an unnecessary parse overhead if the sheet is not applicable (I think).

Also, there seems to be an occasional time-out in one of the CSS tests, and the code needs some cleaning up.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #20721 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because existing CSS tests should cover them.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22478)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 17, 2018

💔 Test failed - status-taskcluster

@mandreyel mandreyel force-pushed the mandreyel:parallel-css-parsing branch from da5ec86 to f00b500 Dec 17, 2018

@mandreyel

This comment has been minimized.

Copy link
Contributor

mandreyel commented Dec 17, 2018

Oh damn, forgot to run tidy again...

@mandreyel mandreyel force-pushed the mandreyel:parallel-css-parsing branch from f00b500 to 177f3be Dec 17, 2018

@emilio

This comment has been minimized.

Copy link
Member

emilio commented Dec 17, 2018

@bors-servo try

@jdm is there a bors feature-request somewhere to add some sort of delegate-try=foo syntax, so that @mandreyel can run try runs, but not commit (yet at least :P)?

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 17, 2018

⌛️ Trying commit 177f3be with merge 08d4348...

bors-servo added a commit that referenced this pull request Dec 17, 2018

Auto merge of #22478 - mandreyel:parallel-css-parsing, r=<try>
[WIP]  Implement parallel CSS parsing

<!-- Please describe your changes on the following line: -->
Opening for discussion.

The basic idea is to spawn a parser thread on a global thread-pool (like in Gecko) with an `ThreadSafeStylesheetLoader` (instead of the previous `StylesheetLoader`), used when encountering an `@import` rule to send a task to script thread from the parser thread, which simply instantiates an async fetch on the corresponding document. After the parsing is done, another task is sent to script thread to do the post-parsing steps, like setting the parsed stylesheet on its link element (if applicable), firing the load event, etc.

A problem was that verifying the request generation id before parsing (still on the script thread) introduced a race condition, so I moved that to the post-parse task run on the script thread. This (seems to have) solved the race condition, but it differs in semantics from the sync parsing in that it's now no longer checked before doing the parsing, but after, which incurs an unnecessary parse overhead if the sheet is not applicable (I think).

Also, there seems to be an occasional time-out in one of the CSS tests, and the code needs some cleaning up.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #20721 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because existing CSS tests should cover them.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22478)
<!-- Reviewable:end -->
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 17, 2018

I do not believe that has been requested before. We should give mandreyel try access, though.

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 17, 2018

💔 Test failed - status-taskcluster

emilio added a commit to emilio/servo that referenced this pull request Dec 17, 2018

style: Make Servo use a single thread-pool for layout-related task pe…
…r-process.

Instead of per-document. This also allows to reuse this thread-pool if needed
for other stuff, like parallel CSS parsing (servo#22478), and to share more code with
Gecko, which is always nice.

emilio added a commit to emilio/servo that referenced this pull request Dec 17, 2018

style: Make Servo use a single thread-pool for layout-related tasks p…
…er-process.

Instead of per-document. This also allows to reuse this thread-pool if needed
for other stuff, like parallel CSS parsing (servo#22478), and to share more code with
Gecko, which is always nice.
@emilio

This comment has been minimized.

Copy link
Member

emilio commented Dec 17, 2018

There are a few tests timing out with these patches that look relevant:

https://taskcluster-artifacts.net/Zp_9dkhbSpitqpzNKAYJEA/0/public/filtered-wpt-errorsummary.log

@emilio emilio referenced this pull request Dec 17, 2018

Merged

Give mandreyel try access. #928

bors-servo added a commit that referenced this pull request Dec 17, 2018

Auto merge of #22487 - emilio:single-layout-thread-pool, r=<try>
style: Make Servo use a single thread-pool for layout-related tasks per process.

Instead of per-document. This also allows to reuse this thread-pool if needed
for other stuff, like parallel CSS parsing (#22478), and to share more code with
Gecko, which is always nice.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22487)
<!-- Reviewable:end -->
@mandreyel

This comment has been minimized.

Copy link
Contributor

mandreyel commented Dec 18, 2018

I guess homu needs to be restarted.

@CYBAI

This comment has been minimized.

Copy link
Collaborator

CYBAI commented Dec 19, 2018

@bors-servo try

@mandreyel yes, you'll need to wait someone to redeploy it XD Let me help you to try it first!

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 19, 2018

⌛️ Trying commit 15bbf80 with merge be0dc84...

bors-servo added a commit that referenced this pull request Dec 19, 2018

Auto merge of #22478 - mandreyel:parallel-css-parsing, r=<try>
[WIP]  Implement parallel CSS parsing

<!-- Please describe your changes on the following line: -->
Opening for discussion.

The basic idea is to spawn a parser thread on a global thread-pool (like in Gecko) with an `ThreadSafeStylesheetLoader` (instead of the previous `StylesheetLoader`), used when encountering an `@import` rule to send a task to script thread from the parser thread, which simply instantiates an async fetch on the corresponding document. After the parsing is done, another task is sent to script thread to do the post-parsing steps, like setting the parsed stylesheet on its link element (if applicable), firing the load event, etc.

A problem was that verifying the request generation id before parsing (still on the script thread) introduced a race condition, so I moved that to the post-parse task run on the script thread. This (seems to have) solved the race condition, but it differs in semantics from the sync parsing in that it's now no longer checked before doing the parsing, but after, which incurs an unnecessary parse overhead if the sheet is not applicable (I think).

Also, there seems to be an occasional time-out in one of the CSS tests, and the code needs some cleaning up.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #20721 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because existing CSS tests should cover them.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22478)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 19, 2018

emilio added a commit to emilio/servo that referenced this pull request Dec 23, 2018

style: Make Servo use a single thread-pool for layout-related tasks p…
…er-process.

Instead of per-document. This also allows to reuse this thread-pool if needed
for other stuff, like parallel CSS parsing (servo#22478), and to share more code with
Gecko, which is always nice.
@emilio

This comment has been minimized.

Copy link
Member

emilio commented Dec 23, 2018

So the issue I have with the 1024-bytes limit is that it makes us lose test coverage, since most stylesheets in WPT are really small...

I suspect #22487 will make the scheduler much happier about scheduling our work on time, I don't see any reason why the thread roundtrip should be long enough to make the test time out.

bors-servo added a commit that referenced this pull request Dec 23, 2018

Auto merge of #22487 - emilio:single-layout-thread-pool, r=jdm
style: Make Servo use a single thread-pool for layout-related tasks per process.

Instead of per-document. This also allows to reuse this thread-pool if needed
for other stuff, like parallel CSS parsing (#22478), and to share more code with
Gecko, which is always nice.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22487)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request Dec 23, 2018

Auto merge of #22487 - emilio:single-layout-thread-pool, r=jdm
style: Make Servo use a single thread-pool for layout-related tasks per process.

Instead of per-document. This also allows to reuse this thread-pool if needed
for other stuff, like parallel CSS parsing (#22478), and to share more code with
Gecko, which is always nice.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22487)
<!-- Reviewable:end -->
@mandreyel

This comment has been minimized.

Copy link
Contributor

mandreyel commented Dec 23, 2018

@emilio Hmm, I suspected test stylesheets might be too small. I'm not convinced inter-thread communication overhead is negligible in case of small sheets, but I'll rebase and rerun tests to see.

Are there tests somewhere that use big stylesheets? How did Bobby Holley (and co) test parallel parsing speedups? Is there some infrastructure in place for these tests?

@emilio

This comment has been minimized.

Copy link
Member

emilio commented Dec 23, 2018

@emilio Hmm, I suspected test stylesheets might be too small. I'm not convinced inter-thread communication overhead is negligible in case of small sheets, but I'll rebase and rerun tests to see.

I'm sure it's not negligible, though it seems to me it should be small enough to not be very relevant, and having a single code path is really useful. Also, parsing tons of small stylesheets in parallel seems likely to be a win over doing them on the same thread... maybe.

Anyhow, Gecko has an automated performance test-suite called tp6 which measures load time in multiple places, see https://bugzilla.mozilla.org/show_bug.cgi?id=1455115#c3 and co. I think the test-cases are just snapshots of those websites. Not sure off-hand where can you download it, though I believe they're public.

Note that for it to be a win we had to fix the locking contention during atomization (interning) of strings: https://bugzilla.mozilla.org/show_bug.cgi?id=1440824. We may have the same contention issues in Servo, though Servo inlines small atoms without locking, which may be enough to mitigate / resolve this. If the lock call in:

https://github.com/servo/string-cache/blob/6d5ddc812574c9120bb7223eb991701a2f15c0fe/src/atom.rs#L310

was a huge bottleneck, then we'd need to do something of the sort splitting the table among multiple ones.

In any case, nothing like that necessarily blocks you here. If after rebasing try is not green, we can do the small stylesheet thing, or put it behind a pref or such.

@mandreyel

This comment has been minimized.

Copy link
Contributor

mandreyel commented Dec 23, 2018

So I rebased and ran the tests and it unfortunately still times out. I guess the overhead really shouldn't be so large as to make any of the tests time out, so there must be some other issue. I'll look into it, but I might not be able to finish this until beginning of next year.

@mandreyel mandreyel force-pushed the mandreyel:parallel-css-parsing branch from 15bbf80 to 09184ff Dec 24, 2018

@mandreyel

This comment has been minimized.

Copy link
Contributor

mandreyel commented Dec 24, 2018

The two tests that timed out locally were both related to mismatched origins. I didn't fully understand the wpt tests themselves, but I suspected it was the error path timing out.
For some reason I was going on the parser thread unconditionally--even when the fetch status reported errors--which resulted in immediately enqueuing a "finish async sheet parse" task on script, even though it could have been done without going on the thread in the first place. I moved that bit out of the thread and it seems to have fixed it.

@mandreyel mandreyel force-pushed the mandreyel:parallel-css-parsing branch from 09184ff to c8fa668 Dec 24, 2018

@mandreyel

This comment has been minimized.

Copy link
Contributor

mandreyel commented Dec 24, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 24, 2018

@mandreyel: 🔑 Insufficient privileges: not in try users

@mandreyel

This comment has been minimized.

Copy link
Contributor

mandreyel commented Dec 24, 2018

Well, damn. @CYBAI Would you be willing to lend a try-hand again? :D Or anyone currently available, really curious if this fixed it.

} else {
atom!("load")
};
elem.upcast::<EventTarget>().fire_event(event);

This comment has been minimized.

@mandreyel

mandreyel Dec 24, 2018

Contributor

Since this branch is only executed on fetch failure, can we unconditionally fire an error event here, @emilio?

This comment has been minimized.

@emilio

emilio Dec 24, 2018

Member

Yes, though then you should assert!(any_failed), probably.

@CYBAI

This comment has been minimized.

Copy link
Collaborator

CYBAI commented Dec 24, 2018

@bors-servo try

  • Sure :)
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 24, 2018

⌛️ Trying commit c8fa668 with merge 83f9048...

bors-servo added a commit that referenced this pull request Dec 24, 2018

Auto merge of #22478 - mandreyel:parallel-css-parsing, r=<try>
[WIP]  Implement parallel CSS parsing

<!-- Please describe your changes on the following line: -->
Opening for discussion.

The basic idea is to spawn a parser thread on a global thread-pool (like in Gecko) with an `ThreadSafeStylesheetLoader` (instead of the previous `StylesheetLoader`), used when encountering an `@import` rule to send a task to script thread from the parser thread, which simply instantiates an async fetch on the corresponding document. After the parsing is done, another task is sent to script thread to do the post-parsing steps, like setting the parsed stylesheet on its link element (if applicable), firing the load event, etc.

A problem was that verifying the request generation id before parsing (still on the script thread) introduced a race condition, so I moved that to the post-parse task run on the script thread. This (seems to have) solved the race condition, but it differs in semantics from the sync parsing in that it's now no longer checked before doing the parsing, but after, which incurs an unnecessary parse overhead if the sheet is not applicable (I think).

Also, there seems to be an occasional time-out in one of the CSS tests, and the code needs some cleaning up.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #20721 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because existing CSS tests should cover them.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22478)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 24, 2018

@emilio
Copy link
Member

emilio left a comment

Need to take a closer look after Christmas, so only a bit of high-level feedback. Feel free to not take any action yet if you don't want to :)

} else {
atom!("load")
};
elem.upcast::<EventTarget>().fire_event(event);

This comment has been minimized.

@emilio

emilio Dec 24, 2018

Member

Yes, though then you should assert!(any_failed), probably.

};

// Unroot document and move that into the thread. TODO(mandreyel):
// Should we move a rooted or an unrooted document into the thread?

This comment has been minimized.

@emilio

emilio Dec 24, 2018

Member

This comment makes no sense. Trusted<> values are "rooted" by definition, and you can't use normal Root<T> objects off-thread, so...

This comment has been minimized.

@mandreyel

mandreyel Dec 24, 2018

Contributor

I think I was a bit tired when I wrote this and got confused. Thanks for the clarification.

StylesheetSource::LinkElement(Some(media.take().unwrap()))
},
StylesheetSource::Import(ref mut sheet) => StylesheetSource::Import(sheet.clone()),
};

This comment has been minimized.

@emilio

emilio Dec 24, 2018

Member

This feels a bit hacky, and clones the Import unnecessarily. Seems like process_response_eof should take self instead of &mut self.

In any case that can be a TODO or a follow-up.

This comment has been minimized.

@mandreyel

mandreyel Dec 24, 2018

Contributor

Agreed, this is a conscious workaround for what you say. I'll add the TODO and open an issue and do the follow-up PR, if you don't mind :)

This comment has been minimized.

@mandreyel
media.take().unwrap(),
shared_lock,
Some(&loader),
// No error reporting in async parse mode.

This comment has been minimized.

@emilio

emilio Dec 24, 2018

Member

We should probably fall back to sync parsing if we want CSS errors... But we don't have a good way of doing that, so this is probably ok for now.

This comment has been minimized.

@mandreyel

mandreyel Dec 24, 2018

Contributor

I was thinking of this too but didn't see or know of any flag that explicitly sets this. I'll open an issue on this once this is merged.

let final_url = metadata.final_url;
// FIXME: Revisit once consensus is reached at:
// https://github.com/whatwg/html/issues/1142
let successful = metadata.status.map_or(false, |(code, _)| code == 200);

This comment has been minimized.

@emilio

emilio Dec 24, 2018

Member

So, it is a bit weird that we fire the sync error event for status.is_err(), but not for this. It'd be great to do this consistently.

This comment has been minimized.

@mandreyel

mandreyel Dec 24, 2018

Contributor

It used to be consistent, in that error event was fired in the FinishAsyncStylesheetLoadTask in all cases, but it seems (asynchronously) firing this event there, on status.is_err(), caused the timeouts. I haven't had the time to figure out exactly why.
I don't like the inconsistency either, but the difficulty I see is that in the sync version successful is determined after parsing and setting the sheet. So if we wanted consistency, we'd have to go on the parser thread only to send a task to script to fire the error event without parsing anything, which causes timeouts and what my latest commit fixes.
(I tried if only parsing the sheet if status.is_ok() && metadata.status.map_or(false, |(code, _)| code == 200) was true worked, because it would simplify the branching logic, but this also caused timeouts.)

This comment has been minimized.

@mandreyel

mandreyel Jan 4, 2019

Contributor

@emilio What do you think?

This comment has been minimized.

@emilio

emilio Jan 6, 2019

Member

Ahh, sorry for the lag here, mostly christmas vacation. I should try to take a look at those timeouts. Have you ever used rr? It's very useful to diagnose this kind of stuff. There's something really fishy there.

This comment has been minimized.

@mandreyel

mandreyel Jan 7, 2019

Contributor

No worries, I was also afk for the holidays :) Happy new year btw! I haven't used it yet, but I'll try it sometime in the next few days.

[edit] Just letting you know that I haven't forgotten about this but a lot of unexpected things have happened that consumed all my time. I'll foreseeably be able to continue in ~2 weeks.

.upcast::<Element>()
.as_stylesheet_owner()
.expect("Stylesheet not loaded by <style> or <link> element!");
owner.set_origin_clean(self.origin_clean);

This comment has been minimized.

@emilio

emilio Dec 24, 2018

Member

It is a bit unfortunate to have this duplicated in two places... :/

This comment has been minimized.

@mandreyel

mandreyel Dec 24, 2018

Contributor

Agreed. This was a quick-and-dirty fix before Christmas descended full force, will refactor in a bit.

This comment has been minimized.

@mandreyel

mandreyel Dec 24, 2018

Contributor

Done.

@@ -67,8 +67,6 @@ extern crate malloc_size_of_derive;
extern crate matches;
#[cfg(feature = "gecko")]
pub extern crate nsstring;
#[cfg(feature = "gecko")]
extern crate num_cpus;

This comment has been minimized.

@emilio

emilio Dec 24, 2018

Member

This cannot go away.

This comment has been minimized.

@mandreyel

mandreyel Dec 24, 2018

Contributor

Oh, sorry, that was a rebase-merge accident.

This comment has been minimized.

@mandreyel

mandreyel Dec 24, 2018

Contributor

Done.

@mandreyel mandreyel force-pushed the mandreyel:parallel-css-parsing branch from c8fa668 to 09ad832 Dec 24, 2018

@mandreyel mandreyel force-pushed the mandreyel:parallel-css-parsing branch from 09ad832 to d395032 Dec 24, 2018

@mandreyel mandreyel force-pushed the mandreyel:parallel-css-parsing branch from d395032 to 0a15173 Dec 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment