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

Replace mpsc with crossbeam-channel #21325

Merged
merged 2 commits into from Sep 12, 2018

Conversation

Projects
None yet
9 participants
@gterzian
Collaborator

gterzian commented Aug 2, 2018

Follow up on #19515


Selecting over multiple channels in std::sync::mpsc is not stable and likely never will be:

rust-lang/rust#27800 (comment)

It seems the only thing keeping mpsc_select around is Servo.

crossbeam-channel is designed specifically to replace std::sync::mpsc and fix many of its shortcomings:
https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md

This is to be landed together with servo/ipc-channel#183.


This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Aug 2, 2018

Heads up! This PR modifies the following files:

  • @jgraham: components/webdriver_server/lib.rs, components/webdriver_server/Cargo.toml
  • @emilio: components/style/animation.rs, components/style/Cargo.toml, components/style/lib.rs, components/style/context.rs, components/layout/animation.rs and 2 more
  • @KiChjang: components/script_traits/lib.rs, components/script/dom/abstractworkerglobalscope.rs, components/script/dom/servoparser/async_html.rs, components/net/http_loader.rs, components/script/dom/workerglobalscope.rs and 42 more
  • @asajeffrey: components/script/dom/abstractworkerglobalscope.rs, components/script/dom/servoparser/async_html.rs, components/script/dom/workerglobalscope.rs, components/script/task_source/dom_manipulation.rs, components/script/Cargo.toml and 39 more
  • @cbrewster: components/constellation/timer_scheduler.rs, components/constellation/network_listener.rs, components/constellation/constellation.rs, components/constellation/lib.rs, components/constellation/pipeline.rs and 1 more
  • @edunham: servo-tidy.toml
  • @paulrouget: components/constellation/timer_scheduler.rs, components/compositing/compositor_thread.rs, components/compositing/compositor.rs, components/compositing/Cargo.toml, components/constellation/network_listener.rs and 8 more

@gterzian gterzian force-pushed the gterzian:crossbeam_integration branch from 6af0967 to cb3a52d Aug 2, 2018

@gterzian

This comment has been minimized.

Collaborator

gterzian commented Aug 2, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Aug 2, 2018

⌛️ Trying commit cb3a52d with merge d6fbaa9...

bors-servo added a commit that referenced this pull request Aug 2, 2018

Auto merge of #21325 - gterzian:crossbeam_integration, r=<try>
Crossbeam integration

<!-- Please describe your changes on the following line: -->
Follow up on #19515

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

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/21325)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Aug 2, 2018

💔 Test failed - linux-dev

@stjepang

This comment has been minimized.

Contributor

stjepang commented Aug 2, 2018

@gterzian @SimonSapin

In order to resolve the tidy errors, I'm currently updating dependencies of the crossbeam crates. Just waiting for the Travis tests to pass... (will take an hour or so)

@stjepang

This comment has been minimized.

Contributor

stjepang commented Aug 2, 2018

Done! Can we retry the tests?

@highfive highfive removed the S-tests-failed label Aug 2, 2018

@gterzian

This comment has been minimized.

Collaborator

gterzian commented Aug 2, 2018

{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_mozilla/webgl/conformance-1.0.3/conformance/renderbuffers/framebuffer-object-attachment.html", "line": 103695, "action": "test_result", "expected": "CRASH"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_mozilla/webgl/conformance-2.0.0/conformance2/renderbuffers/framebuffer-object-attachment.html", "line": 106426, "action": "test_result", "expected": "CRASH"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_mozilla/webgl/conformance-1.0.3/conformance/canvas/drawingbuffer-test.html", "line": 42832, "action": "test_result", "expected": "CRASH"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_mozilla/webgl/conformance-1.0.3/conformance/canvas/drawingbuffer-static-canvas-test.html", "line": 50434, "action": "test_result", "expected": "CRASH"}

Actually getting a bunch of positive unexpected test results. Also some websocket timeouts, but I think those are intermittents.

I just removed another similar default in serviceworker...

@stjepang thanks, retrying now

@bors-servo try

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Aug 2, 2018

⌛️ Trying commit ca86ae9 with merge 0443bcf...

bors-servo added a commit that referenced this pull request Aug 2, 2018

Auto merge of #21325 - gterzian:crossbeam_integration, r=<try>
Crossbeam integration

<!-- Please describe your changes on the following line: -->
Follow up on #19515

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

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/21325)
<!-- Reviewable:end -->
@gterzian

This comment has been minimized.

Collaborator

gterzian commented Aug 2, 2018


{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "DOMParser", "test": "/html/semantics/embedded-content/the-img-element/non-active-document.html", "line": 132002, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "createHTMLDocument", "test": "/html/semantics/embedded-content/the-img-element/non-active-document.html", "line": 132003, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "<template>", "test": "/html/semantics/embedded-content/the-img-element/non-active-document.html", "line": 132004, "action": "test_result", "expected": "FAIL"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_mozilla/webgl/conformance-1.0.3/conformance/renderbuffers/framebuffer-object-attachment.html", "line": 232346, "action": "test_result", "expected": "CRASH"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_mozilla/webgl/conformance-2.0.0/conformance2/renderbuffers/framebuffer-object-attachment.html", "line": 232868, "action": "test_result", "expected": "CRASH"}

awesome, let's see if those hold up on the next test run... Wasn't there something about the channel to the layout thread crashing sometimes? Could it be that this is fixed which is preventing those crashes of those webgl tests?

The source for this is http://build.servo.org/grid, under the column for merge commit corresponding to what the bot announces...

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Aug 2, 2018

💔 Test failed - linux-dev

@gterzian

This comment has been minimized.

Collaborator

gterzian commented Aug 2, 2018


./Cargo.lock:1: duplicate versions for package `crossbeam-epoch`
	The following packages depend on version 0.3.1 from 'crates.io':
		crossbeam-deque
	The following packages depend on version 0.5.0 from 'crates.io':
		crossbeam-channel

./Cargo.lock:1: duplicate versions for package `crossbeam-utils`
	The following packages depend on version 0.2.2 from 'crates.io':
		crossbeam-deque
		crossbeam-epoch
	The following packages depend on version 0.4.1 from 'crates.io':
		crossbeam-channel
		crossbeam-epoch

./Cargo.lock:1: duplicate versions for package `parking_lot`
	The following packages depend on version 0.5.5 from 'crates.io':
		crossbeam-channel
	The following packages depend on version 0.6.3 from 'crates.io':
		layout
		layout_thread
		script
		style
		style_tests
		winit

Do we need to do something on this side too? (this isn't going to prevent the other tests from running, which will still provide useful info)

@stjepang

This comment has been minimized.

Contributor

stjepang commented Aug 2, 2018

Oh, I see what the problem is.

So rayon-core depends on an old version of crossbeam-deque, which depends on old versions of crossbeam-epoch and crossbeam-utils. The reason why Rayon doesn't update to the newest version of crossbeam-deque is because it requires Rust 1.25, and Rayon wants to support older versions of Rust.

I don't see an easy way out of this mess. :(

Can we just allow using multiple versions of the Crossbeam crates and parking_lot in Servo?

@jdm

This comment has been minimized.

Member

jdm commented Aug 2, 2018

Yes, our servo-tidy.toml has a list of exclusions for the duplicate crate check.

@gterzian

This comment has been minimized.

Collaborator

gterzian commented Aug 2, 2018

this looks like progress:

{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "DOMParser", "test": "/html/semantics/embedded-content/the-img-element/non-active-document.html", "line": 139976, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "createHTMLDocument", "test": "/html/semantics/embedded-content/the-img-element/non-active-document.html", "line": 139977, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "<template>", "test": "/html/semantics/embedded-content/the-img-element/non-active-document.html", "line": 139978, "action": "test_result", "expected": "FAIL"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_mozilla/webgl/conformance-1.0.3/conformance/renderbuffers/framebuffer-object-attachment.html", "line": 226820, "action": "test_result", "expected": "CRASH"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_mozilla/webgl/conformance-2.0.0/conformance2/renderbuffers/framebuffer-object-attachment.html", "line": 232829, "action": "test_result", "expected": "CRASH"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_mozilla/webgl/conformance-1.0.3/conformance/canvas/drawingbuffer-static-canvas-test.html", "line": 40960, "action": "test_result", "expected": "CRASH"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "DOMParser", "test": "/html/semantics/embedded-content/the-img-element/non-active-document.html", "line": 48774, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "createHTMLDocument", "test": "/html/semantics/embedded-content/the-img-element/non-active-document.html", "line": 48775, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "<template>", "test": "/html/semantics/embedded-content/the-img-element/non-active-document.html", "line": 48776, "action": "test_result", "expected": "FAIL"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_mozilla/webgl/conformance-1.0.3/conformance/canvas/drawingbuffer-test.html", "line": 53963, "action": "test_result", "expected": "CRASH"}

{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_mozilla/webgl/conformance-1.0.3/conformance/renderbuffers/framebuffer-object-attachment.html", "line": 99171, "action": "test_result", "expected": "CRASH"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_mozilla/webgl/conformance-2.0.0/conformance2/renderbuffers/framebuffer-object-attachment.html", "line": 105733, "action": "test_result", "expected": "CRASH"}

those look intermittent(failing on different boxes on different runs):

{"status": "FAIL", "group": "default", "message": "assert_true: expected true got false", "stack": "@http://web-platform.test:8000/html/semantics/interactive-elements/the-details-element/toggleEvent.html:115:5\nTest.prototype.step_timeout/<@http://web-platform.test:8000/resources/testharness.js:1596:20\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20\nTest.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1562:20\n", "subtest": "Calling open twice on 'details' fires only one toggle event", "test": "/html/semantics/interactive-elements/the-details-element/toggleEvent.html", "line": 155693, "action": "test_result", "expected": "PASS"}
{"status": "FAIL", "group": "default", "message": "assert_true: expected true got false", "stack": "@http://web-platform.test:8000/html/semantics/interactive-elements/the-details-element/toggleEvent.html:144:5\nTest.prototype.step_timeout/<@http://web-platform.test:8000/resources/testharness.js:1596:20\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20\nTest.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1562:20\n", "subtest": "Setting open=true to opened 'details' element should not fire a toggle event at the 'details' element", "test": "/html/semantics/interactive-elements/the-details-element/toggleEvent.html", "line": 155696, "action": "test_result", "expected": "PASS"}

I'll update the test exptectations and the servo-tidy.toml(thanks @jdm) in the morning, and squash all the commits into one I guess.

@highfive highfive removed the S-tests-failed label Aug 3, 2018

@gterzian gterzian force-pushed the gterzian:crossbeam_integration branch from fc3c844 to 690e6d3 Aug 3, 2018

@gterzian

This comment has been minimized.

Collaborator

gterzian commented Aug 3, 2018

bors-servo added a commit that referenced this pull request Aug 3, 2018

Auto merge of #21325 - gterzian:crossbeam_integration, r=<try>
Crossbeam integration

<!-- Please describe your changes on the following line: -->
Follow up on #19515

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

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/21325)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Aug 3, 2018

⌛️ Trying commit 690e6d3 with merge ff40f1d...

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Aug 3, 2018

💔 Test failed - linux-rel-css

@gterzian

This comment has been minimized.

Collaborator

gterzian commented Sep 12, 2018

screen shot 2018-09-12 at 7 42 21 pm

One of the builds passed entirely, and I haven't seen any suite-wide timeouts anymore. It looks like the issue was fixed indeed, thanks @stjepang !

@jdm r?

@jdm

This comment has been minimized.

Member

jdm commented Sep 12, 2018

@bors-servo r=SimonSapin,jdm

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Sep 12, 2018

📌 Commit 2a996fb has been approved by SimonSapin,jdm

@highfive highfive assigned SimonSapin and unassigned jdm Sep 12, 2018

bors-servo added a commit that referenced this pull request Sep 12, 2018

Auto merge of #21325 - gterzian:crossbeam_integration, r=SimonSapin,jdm
Replace mpsc with crossbeam-channel

Follow up on #19515

---

Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be:

rust-lang/rust#27800 (comment)
> It seems the only thing keeping `mpsc_select` around is Servo.

crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings:
https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md

This is to be landed together with servo/ipc-channel#183.

<!-- 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/21325)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Sep 12, 2018

⌛️ Testing commit 2a996fb with merge b3f084e...

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Sep 12, 2018

💔 Test failed - mac-rel-wpt4

@jdm

This comment has been minimized.

Member

jdm commented Sep 12, 2018

bors-servo added a commit that referenced this pull request Sep 12, 2018

Auto merge of #21325 - gterzian:crossbeam_integration, r=SimonSapin,jdm
Replace mpsc with crossbeam-channel

Follow up on #19515

---

Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be:

rust-lang/rust#27800 (comment)
> It seems the only thing keeping `mpsc_select` around is Servo.

crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings:
https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md

This is to be landed together with servo/ipc-channel#183.

<!-- 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/21325)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Sep 12, 2018

⌛️ Testing commit 2a996fb with merge 910cc23...

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Sep 12, 2018

💔 Test failed - mac-rel-css1

@jdm

This comment has been minimized.

Member

jdm commented Sep 12, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Sep 12, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Sep 12, 2018

@bors-servo bors-servo merged commit 2a996fb into servo:master Sep 12, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
homu Test successful
Details

bors-servo added a commit that referenced this pull request Sep 12, 2018

Auto merge of #21644 - Eijebong:hyperup, r=<try>
WIP: Update hyper to 0.12

Left to do:
 - Fix the influent::Client call
 - servo/webrender#3034
 - hyperium/mime#91
 - sfackler/typed-headers#3 (still need a lot of work)
 - https://bugzilla.mozilla.org/show_bug.cgi?id=1489792
 - Merge and release the ws stuff (I'll do it once I know everything is ok)
 - Merge and release the influent stuff (Same here, I'll do it once I know everything is ok)
 - #21325
 - Fix unit tests
<!-- 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/21644)
<!-- Reviewable:end -->

@gterzian gterzian deleted the gterzian:crossbeam_integration branch Sep 13, 2018

@gterzian

This comment has been minimized.

Collaborator

gterzian commented Sep 13, 2018

247 comments on a PR, must be a record of some kind...

Thanks for everyone's help and involvement!

@jdm

This comment has been minimized.

Member

jdm commented Sep 13, 2018

Thanks again for tackling this, and @stjepang for so much assistance!

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 15, 2018

Bug 1491577 - Replace mpsc with crossbeam/servo channel, update ipc-c…
…hannel. r=emilio

This cherry-picks servo/servo#21325.

Co-authored-by: Gregory Terzian <gterzian@users.noreply.github.com>

jankeromnes pushed a commit to jankeromnes/gecko that referenced this pull request Sep 16, 2018

Bug 1491577 - Replace mpsc with crossbeam/servo channel, update ipc-c…
…hannel. r=emilio

This cherry-picks servo/servo#21325.

Co-authored-by: Gregory Terzian <gterzian@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment