Don't make OsIpcSender Sync
#115
Merged
Conversation
This file contains 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
`mpsc::Sender` isn't `Sync` -- and the `ipc-channel` equivalent probably shouldn't be either. Sharing references to senders is usually not a good idea: they are rather meant to be cloned instead; and they implement internal sharing to make this efficient and robust -- sharing references to senders just adds an unnecessary extra layer of sharing on top. For the `inprocess` back-end, implementing `Sync` was necessitating use of an `Arc<Mutex<>>`, adding considerable overhead even when not used -- so this change is an optimisation. It effectively reverts most of the sender part of 30b2024 (the receiver part was already backed out in 77469c2 ), except that it obviously doesn't re-introduce the bogus explicit `Sync` claim. For the `macos` and `unix` back-ends, the sender implementations were inherently `Sync`; so we explicitly mark them `!Sync` now, to get a consistent API. Also bumping the `ipc-channel` version here, as this is a breaking change.
Unfortunately this requires an "unstable" feature to be enabled, because it relies on specialization... Can't think of any other way to handle this.
|
I'm wondering whether maybe it would be better to squash these commits; or some of them at least?... |
|
Have you tried https://github.com/laumann/compiletest-rs? That strikes me as the simplest way to make sure |
|
@pcwalton with lots of tears and sweat, I managed to get it working with However, |
|
@bors-servo: r+ |
|
📌 Commit ad19e55 has been approved by |
bors-servo
pushed a commit
that referenced
this pull request
Nov 7, 2016
Don't make `OsIpcSender` `Sync` `mpsc::Sender` isn't `Sync` -- and the `ipc-channel` equivalent probably shouldn't be either. Sharing references to senders is usually not a good idea: they are rather meant to be cloned instead; and they implement internal sharing to make this efficient and robust -- sharing references to senders just adds an unnecessary extra layer of sharing on top. For the `inprocess` back-end, implementing `Sync` was necessitating use of an `Arc<Mutex<>>`, adding considerable overhead even when not used -- so this change is an optimisation. It effectively reverts most of the sender part of 30b2024 (the receiver part was already backed out in 77469c2 ), except that it obviously doesn't re-introduce the bogus explicit `Sync` claim. For the `macos` and `unix` back-ends, the sender implementations were inherently `Sync`; so we explicitly mark them `!Sync` now, to get a consistent API. Also bumping the `ipc-channel` version here, as this is a breaking change. (It affects Servo only in one place though, see servo/servo#14048 ) This also comes with a test case to verify that `OsIpcSender` indeed isn't `Sync` any more; and a CI change to activate it.
|
💔 Test failed - status-appveyor |
|
@bors-servo retry |
bors-servo
pushed a commit
that referenced
this pull request
Nov 8, 2016
Don't make `OsIpcSender` `Sync` `mpsc::Sender` isn't `Sync` -- and the `ipc-channel` equivalent probably shouldn't be either. Sharing references to senders is usually not a good idea: they are rather meant to be cloned instead; and they implement internal sharing to make this efficient and robust -- sharing references to senders just adds an unnecessary extra layer of sharing on top. For the `inprocess` back-end, implementing `Sync` was necessitating use of an `Arc<Mutex<>>`, adding considerable overhead even when not used -- so this change is an optimisation. It effectively reverts most of the sender part of 30b2024 (the receiver part was already backed out in 77469c2 ), except that it obviously doesn't re-introduce the bogus explicit `Sync` claim. For the `macos` and `unix` back-ends, the sender implementations were inherently `Sync`; so we explicitly mark them `!Sync` now, to get a consistent API. Also bumping the `ipc-channel` version here, as this is a breaking change. (It affects Servo only in one place though, see servo/servo#14048 ) This also comes with a test case to verify that `OsIpcSender` indeed isn't `Sync` any more; and a CI change to activate it.
|
☀️ Test successful - status-appveyor, status-travis |
This was referenced Feb 8, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
mpsc::Senderisn'tSync-- and theipc-channelequivalent probablyshouldn't be either. Sharing references to senders is usually not a good
idea: they are rather meant to be cloned instead; and they implement
internal sharing to make this efficient and robust -- sharing references
to senders just adds an unnecessary extra layer of sharing on top.
For the
inprocessback-end, implementingSyncwas necessitatinguse of an
Arc<Mutex<>>, adding considerable overhead even when notused -- so this change is an optimisation. It effectively reverts most
of the sender part of 30b2024 (the
receiver part was already backed out in
77469c2 ), except that it obviously
doesn't re-introduce the bogus explicit
Syncclaim.For the
macosandunixback-ends, the sender implementations wereinherently
Sync; so we explicitly mark them!Syncnow, to get aconsistent API.
Also bumping the
ipc-channelversion here, as this is a breakingchange.
(It affects Servo only in one place though, see servo/servo#14048 )
This also comes with a test case to verify that
OsIpcSenderindeed isn'tSyncany more; and a CI change to activate it.