-
Notifications
You must be signed in to change notification settings - Fork 131
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
Make IpcSender::send take a reference #138
Comments
@nox well, yes and no. It's true that the IPC transfer (as well as the However, On a more fundamental level, Admittedly, I'm having second thoughts about that change as well... But if we start going down that road, there is actually a ton of things we could do differently to make IPC more efficient, if we disregard In fact |
I would gladly abandon all I'm not sure what you are thinking about when you say that serde always adds a bunch of extra data copies though, especially when compared with capnproto, could you develop? |
@nox well, I'm all in favour of adding another API; I'm just not convinced it would be a good idea to change/abandon the existing drop-in API. That would require huge changes throughout Servo -- I suppose that's why @pcwalton conceived As for The worst case here is that a data structure sees a lot of access before or after transfer, in which case working on the serialised representation would be inefficient; so you still need to translate between serialised and "native" representations, thus not buying (or loosing) anything. For data structures that see only little access however (or are in fact assembled right before sending, which is a very common case I believe), this is strictly more efficient. Note that Cap'n Proto was designed for network RPC AIUI, and isn't necessarily optimal for local IPC. (It deals with endianness for example, which is irrelevant for |
How does cap'n'proto cope with opaque types like That may interest you, btw: serde-rs/serde#492 |
@nox not sure in what sense If the actual question is how dynamically sized types are handled, I have to admit that I don't know (yet)... I haven't actually worked with Cap'n Proto so far -- I just know (and like) the fundamental concept. If we are serious about adding an alternative interface to |
@antrik We don't know what the memory representation of |
@nox one fundamental thing to understand about the Cap'n Proto approach is that it doesn't just take the memory representation of any old native type and calls it serialised. That wouldn't work for In other words, the serialised representations of data are often actually distinct from native ones -- the idea is just that in many cases, you can do all work on the serialised ones directly, rather then converting between native and serialised. (To make this ergonomic, the Rust implementation of Cap'n Proto -- or some custom format based on the same ideas -- would have to implement many of Rust's standard library functions on the serialised types I guess...) Regarding Of course we could also try to implement Cap'n Proto concepts in |
What I meant about capnproto is that I find quite unfortunate that you wouldn't be able to do IPC anymore with plain old Rust types such as |
Correction: the lack of scatter/gather costs an extra copy regardless of the serialisation approach I guess; so zero-copy serialisation would still be useful there... The other conditions stand though. |
@nox depends on where the However, if the Same for any other type. |
Yes, I'm just saying it's more codegen to have, this time with external files (AFAIK) in a specific language to feed it to capnproto, right? |
@nox I don't think so? I haven't checked how the existing Cap'n Proto implementation for Rust works -- but I don't see any fundamental reason why we couldn't just derive the serialised equivalents from native Rust types... (Just like |
I hit this problem too, having to clone data just in order to hand it to an ipc sender and then discard it. If we had |
@asajeffrey I'd actually call it It's probably not a huge win, but should be noticeable (in the order of 10% I'd guess?) -- and it's very low hanging fruit :-) |
Actually, calling it |
I realized after I'd written this that this deals with the case where you want to send an &T, but not with the case I actually have, which is sending a T which implements ToOwned, and deserializing it at the other end as T::Owned. We can get very close using Cow, but then we send a Cow<'a,T> and receive a Cow<'static,T> so the lifetimes don't quite line up. We could work round this by defining a unsafe trait ToStatic {
type Static: 'static;
fn to_static(&self) -> Self::Static;
} with the requirement that serializing In particular, we'd have: unsafe impl<'a,T> ToStatic for Cow<'a,T> where T: ToStatic {
type Static = Cow<'static,T::Static>;
fn to_static(&self) -> Self::Static { Cow::Owned(self.deref().to_static()) }
} In order to make this usable, though, we'd have to support deriving |
On thinking about it, perhaps the cleanest interface would be something like a trait DeepClone {
type DeepCloned;
fn deep_clone(&self) -> Self::DeepCloned;
} for example: impl DeepClone for str {
type DeepCloned = String;
fn deep_clone(&self) -> String { self.to_owned() }
} This would be different from impl<T> DeepClone for Rc<T> where T: DeepClone {
type DeepCloned = Rc<T::DeepCloned>;
fn deep_clone(&self) -> Rc<T::DeepCloned> { Rc::new(self.deref().deep_clone()) }
} The expectation is that a serialization then deserialization is the same as a deep clone, in psrticular the serialization format of The change to |
Some mucking around in the rust playground: https://play.rust-lang.org/?gist=c5e14d5a9ab2a4ac7dd2aa7190011125 |
And some more mucking around, this time making sure that implementing |
How is it related to the issue?
|
We can then implement send_deep_clone as well as send_clone. The problem is that send_clone doesn't allow sending data whose borrowing involves a change of type, for example sending a &[T] and receiving it as a Vec. If you'd rather have a separate issue for this, I can open a new one. |
@asajeffrey I'm not sure I follow. Why do you need (And wouldn't you say this should be handled within the Serde framework?...) |
Let's make a new issue. |
Cc @dtolnay for the concept of serialisation equivalence in serde. |
IRC conversation with @nox: http://logs.glob.uno/?c=mozilla%23servo&s=10+Mar+2017&e=10+Mar+2017#c628689 TL;DR: we agree with |
@antrik yes, the idea is to have a way to express that you can serialize a |
Created a new issue for reducing copying in the case that this requires changing type: #156. |
I just remembered that the problem is not with embedded (It's not a technical problem, since |
Hmm, and even putting a T:Clone requirement doesn't fix this, because there might be an embedded Rc<IpcReceiver>. |
@asajeffrey I have been thinking about this on and off; and this is my current idea: how about adding both a
While each of them is limited in certain ways, my current impression is that together they should actually cover all use cases, while avoiding the semantic problems. The plethora of variants might seem confusing at first: but together with the original What do you think? |
Correction: |
For IPC, a sender never needs to take ownership of the sent value.
The text was updated successfully, but these errors were encountered: