Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign uplayout/context: Wrap `image_cache_thread` in a `Mutex<>` as well #14048
Conversation
`SharedLayoutContext.image_cache_thread` didn't get the `Mutex<>` treatment along with all the other channels in there, because `ipc-channel::Sender` is presently inherently `Sync`. I believe this to be an implementation accident though, that should be rectified in the future -- not something users should actually rely on... Note that sharing senders (be it `mpsc` or `ipc-channel`) in is probably not a good idea anyway: just cloning them -- and letting them handle the sharing internally -- should be both simpler and cheaper. But right now that's how things are handled here; so let's go with the flow...
highfive
commented
Nov 3, 2016
|
Heads up! This PR modifies the following files:
|
highfive
commented
Nov 3, 2016
|
|
|
Not a real failure there, just a hiccup as far as I can tell. |
|
@bors-servo r+ |
|
|
bors-servo
added a commit
that referenced
this pull request
Nov 4, 2016
layout/context: Wrap `image_cache_thread` in a `Mutex<>` as well `SharedLayoutContext.image_cache_thread` didn't get the `Mutex<>` treatment along with all the other channels in there, because `ipc-channel::Sender` is presently inherently `Sync`. I believe this to be an implementation accident though, that should be rectified in the future -- not something users should actually rely on... Note that sharing senders (be it `mpsc` or `ipc-channel`) in is probably not a good idea anyway: just cloning them -- and letting them handle the sharing internally -- should be both simpler and cheaper. But right now that's how things are handled here; so let's go with the flow... <!-- 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/14048) <!-- Reviewable:end -->
|
|
bors-servo
added a commit
to servo/ipc-channel
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.
bors-servo
added a commit
to servo/ipc-channel
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.
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.
antrik commentedNov 3, 2016
•
edited by larsbergstrom
SharedLayoutContext.image_cache_threaddidn't get theMutex<>treatment along with all the other channels in there, because
ipc-channel::Senderis presently inherentlySync. I believe this tobe an implementation accident though, that should be rectified in the
future -- not something users should actually rely on...
Note that sharing senders (be it
mpscoripc-channel) in is probablynot a good idea anyway: just cloning them -- and letting them handle the
sharing internally -- should be both simpler and cheaper. But right now
that's how things are handled here; so let's go with the flow...
This change is