Skip to content
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

Unnecessary use of IpcSender/IpcReceiver in FontCacheThread::exit #9373

Closed
jdm opened this issue Jan 18, 2016 · 4 comments
Closed

Unnecessary use of IpcSender/IpcReceiver in FontCacheThread::exit #9373

jdm opened this issue Jan 18, 2016 · 4 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Jan 18, 2016

While investigating servo/ipc-channel#29 I realized that the existing code is inefficient. We can use a regular std::sync::mpsc channel instead of the equivalent from ipc-channel, since both the Constellation and FontCacheThread code run in the same process.

Code: components/gfx/font_cache_thread.rs, in the exit function

@kishorbhat
Copy link
Contributor

@kishorbhat kishorbhat commented Jan 18, 2016

Can I try my hand at this?

Disclaimer: I might need to ping you if I don't get something at first; I'm new to Rust/Servo. :)

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Jan 18, 2016

Sure thing! Come to #servo on irc.mozilla.org and ask your questions there, someone would definitely be in touch with you soon!

@KiChjang KiChjang added the C-assigned label Jan 18, 2016
@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Jan 19, 2016

Not quite clear this is possible:

   Compiling gfx v0.0.1 (file:///home/kishor/projects/servo/components/servo)
   Compiling script v0.0.1 (file:///home/kishor/projects/servo/components/servo)
/home/kishor/projects/servo/components/gfx/font_cache_thread.rs:89:10: 89:21 error: the trait `core::fmt::Debug` is not implemented for the type `std::sync::mpsc::Sender<()>` [E0277]
/home/kishor/projects/servo/components/gfx/font_cache_thread.rs:89     Exit(Sender<()>),
                                                                            ^~~~~~~~~~~
/home/kishor/projects/servo/components/gfx/font_cache_thread.rs:83:34: 83:39 note: in this expansion of #[derive_Debug] (defined in /home/kishor/projects/servo/components/gfx/font_cache_thread.rs)
/home/kishor/projects/servo/components/gfx/font_cache_thread.rs:89:10: 89:21 help: run `rustc --explain E0277` to see a detailed explanation
/home/kishor/projects/servo/components/gfx/font_cache_thread.rs:89:10: 89:21 note: `std::sync::mpsc::Sender<()>` cannot be formatted using `:?`; if it is defined in your crate, add `#[derive(Debug)]` or manually implement it
/home/kishor/projects/servo/components/gfx/font_cache_thread.rs:89:10: 89:21 note: required for the cast to the object type `core::fmt::Debug`
/home/kishor/projects/servo/components/gfx/font_cache_thread.rs:83:23: 83:32 error: the trait `serde::ser::Serialize` is not implemented for the type `std::sync::mpsc::Sender<()>` [E0277]
/home/kishor/projects/servo/components/gfx/font_cache_thread.rs:83 #[derive(Deserialize, Serialize, Debug)]
                                                                                         ^~~~~~~~~
/home/kishor/projects/servo/components/gfx/font_cache_thread.rs:83:23: 83:32 note: in this expansion of #[derive_Serialize] (defined in /home/kishor/projects/servo/components/gfx/font_cache_thread.rs)
/home/kishor/projects/servo/components/gfx/font_cache_thread.rs:83:23: 83:32 help: run `rustc --explain E0277` to see a detailed explanation
/home/kishor/projects/servo/components/gfx/font_cache_thread.rs:83:23: 83:32 note: required by `serde::ser::Serializer::visit_newtype_variant`
/home/kishor/projects/servo/components/gfx/font_cache_thread.rs:83:21: 83:21 error: the trait `serde::de::Deserialize` is not implemented for the type `std::sync::mpsc::Sender<()>` [E0277]
/home/kishor/projects/servo/components/gfx/font_cache_thread.rs:83 #[derive(Deserialize, Serialize, Debug)]
                                                                                       ^
/home/kishor/projects/servo/components/gfx/font_cache_thread.rs:83:10: 83:21 note: in this expansion of try! (defined in <std macros>)
/home/kishor/projects/servo/components/gfx/font_cache_thread.rs:83:21: 83:21 help: run `rustc --explain E0277` to see a detailed explanation
error: aborting due to 3 previous errors
Build failed, waiting for other jobs to finish...
^Cmach interrupted by signal or user action. Stopping.
emilio added a commit to emilio/servo that referenced this issue Jan 19, 2016
This allows unifying the way we prevent the serialisation of things that
can't be sent over IPC for their in-process nature.

This is a proposed solution to things like:
servo#9373

Where something like the following can be used:

```rust
    Exit(Unserializable<Sender<()>>),
```
emilio added a commit to emilio/servo that referenced this issue Jan 19, 2016
This allows unifying the way we prevent the serialisation of things that
can't be sent over IPC for their in-process nature.

This is a proposed solution to things like:
servo#9373

Where something like the following can be used:

```rust
    Exit(Unserializable<Sender<()>>),
```
@jdm
Copy link
Member Author

@jdm jdm commented Jan 19, 2016

I apologize for proposing an impossible change (at least without @ecoal95's experiment). The proper way to solve this would be to split the Command enum into two, and use mspc::Sender/Receiver<InProcessCommand> for the ones that do not need to be sent cross-process. This in turn requires changing the event loop for FontCacheThread to select over both in-process and out-of-process channels simultaneously, which makes it more complicated, etc. The end result is a bunch of work for something that isn't a significant burden right now. Sorry for nothing thinking it through earlier!

@jdm jdm closed this Jan 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.