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

Zero copy for send #238

Closed
wants to merge 57 commits into from
Closed

Zero copy for send #238

wants to merge 57 commits into from

Conversation

@ice1000
Copy link
Contributor

@ice1000 ice1000 commented Oct 9, 2018

TODOs

  • Fix benchmark compilation
  • Fix link error (multiple symbol definitions)
  • Fix size_t 6 in Rust becomes size_t in C 140184721559552 via FFI (140184721559552+6 is 2^47)
  • Sync master changes
  • Improve GrpcSlice
  • Improve GrpcByteBuffer
  • (New!) Support bytes in prost

Now (updated at: 23, April 2019)

Since @nrc has introduced prost support (instead of rust-protobuf) which is natively using "bytes", we no longer have that one copy for send which is stated to be necessary below.
Yay!

Before:

  • User code deserialize (particularly, the protobuf library), producing a list of &[u8], we copied the data into a Vec<u8>, this is done internally in the protobuf library (copy 0)
  • We create a grpc_slice (a ref-counted single string) by copying the Vec<u8> mentioned above (copy 1)
  • We create a grpc_byte_buffer (a ref-counted list of strings) from only one grpc_slice
  • Send

Total copy: 2

After:

  • User code deserialize (particularly, the protobuf library), producing a list of &[u8], we copied each produced &[u8] and collect them into a grpc_byte_buffer (copy 0)
  • Send

Total copy: 1

The reason why still one copy:

  • Rust manages its own allocated memory'a lifetime
  • C-side uses a ref-count mechanism to manage the lifetime of objects
  • We either need to longer the Rust objects' lifetime or copy once
  • Tried to longer Rust objects' lifetime in #225
    • Result: grpc supports buffer-hint which requires a much longer lifetime, it's too hard to manage the lifetime in such case
    • This is why I closed #225
@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Oct 9, 2018

@overvenus PTAL

Copy link
Contributor Author

@ice1000 ice1000 left a comment

Writing down some notes

grpc-sys/src/lib.rs Outdated Show resolved Hide resolved
src/call/mod.rs Outdated Show resolved Hide resolved
@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Oct 10, 2018

Tests fixed, please review @overvenus @BusyJay

src/codec.rs Outdated Show resolved Hide resolved
@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Oct 10, 2018

error: item `call::MessageWriter` has a public `len` method but no corresponding `is_empty` method
   --> src/call/mod.rs:123:1
    |
123 | / impl MessageWriter {
124 | |     pub fn new() -> MessageWriter {
125 | |         MessageWriter {
126 | |             data: Vec::new(),
...   |
147 | |     }
148 | | }
    | |_^
    |
    = note: `-D clippy::len-without-is-empty` implied by `-D warnings`
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#len_without_is_empty
error: using `clone` on a `Copy` type
   --> src/call/mod.rs:134:44
    |
134 |                 grpc_sys::grpc_slice_unref(slice.clone());
    |                                            ^^^^^^^^^^^^^ help: try dereferencing it: `*slice`
    |
    = note: `-D clippy::clone-on-copy` implied by `-D warnings`
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#clone_on_copy
error: identical conversion
   --> src/call/mod.rs:158:30
    |
158 |         let in_len: size_t = size_t::from(buf.len());
    |                              ^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `size_t::from()`: `buf.len()`
    |
    = note: `-D clippy::identity-conversion` implied by `-D warnings`
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#identity_conversion

Oh my god. I'll fix them

Copy link
Contributor Author

@ice1000 ice1000 left a comment

Writing down some notes.

Also: rustfmt is complaining about the order of uses, should fix

Ref: https://travis-ci.org/pingcap/grpc-rs/jobs/439651311

src/call/mod.rs Outdated Show resolved Hide resolved
src/call/mod.rs Outdated Show resolved Hide resolved
src/call/mod.rs Outdated Show resolved Hide resolved
grpc-sys/src/lib.rs Outdated Show resolved Hide resolved
src/call/mod.rs Outdated Show resolved Hide resolved
src/call/mod.rs Show resolved Hide resolved
ice1000 added 17 commits Sep 30, 2018
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
…e not merging my PR)

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Oct 22, 2018

Edit: added some more TODOs

# Conflicts:
#	benchmark/src/bench.rs
#	grpc-sys/src/lib.rs
#	src/call/client.rs
#	src/call/mod.rs
#	src/call/server.rs
#	src/codec.rs
#	src/lib.rs
ice1000 added 3 commits Jan 20, 2019
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
ice1000 added 4 commits Jan 20, 2019
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Feb 10, 2019

Re-benched.

Qps:
2019-02-09 19-08-40

Latency:
2019-02-09 19-09-20

Hoverbear and others added 5 commits Feb 20, 2019
# Conflicts:
#	benchmark/src/bench.rs
#	src/call/client.rs
#	src/call/mod.rs
#	src/call/server.rs
#	src/codec.rs
#	src/lib.rs
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Apr 23, 2019

Now this PR is up-to-date! I've added bytes support for MessageWriter!

For reviewers, you may only be interested in d5e7226 and f634dfa

@ice1000 ice1000 changed the title Reduce one copy for send Zero copy for send Apr 23, 2019
@siddontang
Copy link
Collaborator

@siddontang siddontang commented Apr 24, 2019

Thanks @ice1000

Have you already integrated it into TiKV and run the tests, benchmarks?

@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Apr 24, 2019

Have you already integrated it into TiKV and run the tests, benchmarks?

Not yet in TiKV. Are you hurry? I have an essay due 5 days later while I've just get started (and I'm really really sorry about that 🙏) :(

@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Apr 24, 2019

Travis CI GGed with this warning:

error: mutable borrow from immutable input(s)
   --> grpc-sys/src/lib.rs:415:62
    |
415 |     pub unsafe fn range_from_unsafe(&self, offset: usize) -> &mut [u8] {
    |                                                              ^^^^^^^^^
    |
    = note: `-D clippy::mut-from-ref` implied by `-D clippy::all`
note: immutable borrow here
   --> grpc-sys/src/lib.rs:415:37
    |
415 |     pub unsafe fn range_from_unsafe(&self, offset: usize) -> &mut [u8] {
    |                                     ^^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#mut_from_ref
error: aborting due to previous error
nrc added a commit to nrc/grpc-rs that referenced this pull request Sep 16, 2019
Original work by @ice1000

Signed-off-by: Nick Cameron <nrc@ncameron.org>
nrc added a commit to nrc/grpc-rs that referenced this pull request Sep 29, 2019
Original work by @ice1000

Signed-off-by: Nick Cameron <nrc@ncameron.org>
@nrc nrc mentioned this pull request Sep 30, 2019
@Hoverbear
Copy link
Contributor

@Hoverbear Hoverbear commented Oct 21, 2019

Supersceded by #382 ?

@nrc
Copy link
Contributor

@nrc nrc commented Oct 22, 2019

Supersceded by #382 ?

Yes

@nrc nrc closed this Oct 22, 2019
@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Oct 22, 2019

\o/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants