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

Added sender.send_borrowed(data), to reduce copying #161

Closed
wants to merge 1 commit into from

Conversation

asajeffrey
Copy link
Member

For compatibility with channels, senders take ownership of their data, and drop it immediately after sending. This PR adds the ability to send borrowed data, including nested data, for example sending a &(&str,u32) on a sender expecting a (String,u32).

cc @nox @antrik.

Fixes #156.

@jdm
Copy link
Member

jdm commented Mar 30, 2017

Someone else who has been more involved in the discussions about #156 should probably review this.

@nox
Copy link
Contributor

nox commented Mar 30, 2017 via email

@asajeffrey
Copy link
Member Author

@nox: Just to confirm... Stuff we agree about: having a RoundTrip<T> trait, with #[derive(RoundTrip)]. Stuff we don't: the round_trip(&self) -> T method in RoundTrip<T>, and the implementation using SameDeserialization. AFAICT this only affects users who implement RoundTrip by hand, it's all hidden from people using the custom derive?

@antrik r?

@asajeffrey
Copy link
Member Author

@jdm very wise.

@Gankra
Copy link
Contributor

Gankra commented May 11, 2017

FYI, this would let us reuse the allocation of BuiltDisplayLists (~1MB/frame) in webrender.

@asajeffrey
Copy link
Member Author

r? @gankro then? (This PR has been languishing while I've been busy implementing worklets.)

@antrik
Copy link
Contributor

antrik commented May 11, 2017

@asajeffrey sorry for slacking on this :-( I did glance at briefly it when you published it, and had a bunch of questions, but didn't manage to write them down at the time...

Please give it another couple of days while I look into it. If I still fail to follow up, feel free to ignore me...

@antrik
Copy link
Contributor

antrik commented May 11, 2017

@gankro doesn't Webrender use BytesChannel though to avoid serialisation overhead? That one already takes a reference...

@asajeffrey
Copy link
Member Author

@antrik?

@antrik
Copy link
Contributor

antrik commented May 28, 2017

Since my concerns pertain to the general approach, rather than the specific pull request, I deemed it more appropriate to follow up on the discussion at #156 for now.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #164) made this pull request unmergeable. Please resolve the merge conflicts.

@Gankra
Copy link
Contributor

Gankra commented Jun 15, 2017

Sorry I didn't follow up on this. I think I convinced myself that webrender doesn't need this, for the reasons stated by antrik.

@asajeffrey
Copy link
Member Author

Closing in favour of experimenting with something like https://github.com/asajeffrey/shared-data

@nox nox closed this Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce copying when sending
7 participants