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

Open
wants to merge 55 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ice1000
Copy link
Member

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 ice1000 force-pushed the ice1000:zero-copy-send-new branch from dccdf74 to 0c11b65 Oct 9, 2018

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2018

@overvenus PTAL

@ice1000
Copy link
Member Author

left a comment

Writing down some notes

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

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

Tests fixed, please review @overvenus @BusyJay

Show resolved Hide resolved src/codec.rs Outdated
@ice1000

This comment has been minimized.

Copy link
Member Author

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

@ice1000 ice1000 self-assigned this Oct 10, 2018

@ice1000 ice1000 added the Enhancement label Oct 10, 2018

@ice1000 ice1000 referenced this pull request Oct 10, 2018

Closed

*: support zero copy #134

@ice1000 ice1000 force-pushed the ice1000:zero-copy-send-new branch from 2bf6523 to 98eb3c1 Oct 10, 2018

@ice1000
Copy link
Member Author

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

Show resolved Hide resolved 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

ice1000 added some commits Sep 30, 2018

Add basic char vector
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Some basic works
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Done `drop`
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Add `reserve`
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Refactoring old code
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Stop working with `CharVector`, move to `grpc_slice_buffer`
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Done interface migration
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Done migration!
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Improve dropping logic
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Fix compilation error of benchmark
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Some bug fixes
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Remove some debug leftover
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Use `size_t` instead of `usize`
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Fix **Some** tests (copy-paste some codes from #222 since you guys ar…
…e not merging my PR)

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Fix rustc warnings on CI
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Fix CI (formatting
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>

@ice1000 ice1000 force-pushed the ice1000:zero-copy-send-new branch from 4b43d32 to 641122f Oct 13, 2018

@overvenus
Copy link
Member

left a comment

LGTM

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Nov 12, 2018

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Nov 12, 2018

@hicqu PTAL

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

Benchmark result for latency:

rust_generic_async_streaming_ping_pong
rust_protobuf_async_streaming_ping_pong
rust_protobuf_async_streaming_qps_unconstrained
rust_protobuf_async_unary_ping_pong
rust_protobuf_async_unary_ping_pong_1mb
rust_protobuf_async_unary_qps_unconstrained
rust_protobuf_sync_to_async_unary_ping_pong

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

In one pic:
image

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

Benchmark result for qps:

rust_generic_async_streaming_ping_pong
rust_protobuf_async_streaming_ping_pong
rust_protobuf_async_streaming_qps_unconstrained
rust_protobuf_async_unary_ping_pong
rust_protobuf_async_unary_ping_pong_1mb
rust_protobuf_async_unary_qps_unconstrained
rust_protobuf_sync_to_async_unary_ping_pong

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

In one pic:
image

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

ice1000 added some commits Jan 20, 2019

Trying to support `reserve`
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Add tests, fully support `reserve`
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Bring the improvements to `codec.rs`
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>

ice1000 added some commits Jan 20, 2019

cargo fmt
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Apply suggestion from clippy
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>

@ice1000 ice1000 force-pushed the ice1000:zero-copy-send-new branch from 12e40ef to 8e6ca8a Jan 21, 2019

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2019

Re-benched.

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

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

Hoverbear and others added some commits Feb 20, 2019

Merge branch 'master' into zero-copy-send-new
# Conflicts:
#	benchmark/src/bench.rs
#	src/call/client.rs
#	src/call/mod.rs
#	src/call/server.rs
#	src/codec.rs
#	src/lib.rs
Support bytes for `MessageWriter`
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Collapse `use` statements
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Collapse `use` statements, fix writer bug
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000

This comment has been minimized.

Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.