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 receive #222

Merged
merged 25 commits into from Oct 19, 2018

Conversation

Projects
None yet
6 participants
@ice1000
Copy link
Member

commented Aug 25, 2018

This is not ready to merge yet, but I'm opening this PR so we can discuss changes related to #134 .

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Aug 25, 2018

I think the use of Option<MessageReader> in #134 should be eliminated. The only usage of passing None is

execute(self, cq, None, handler.cb());

. Seems that we need some kind of MessageReader that reads nothing.

@BusyJay

This comment has been minimized.

Copy link
Member

commented Aug 26, 2018

Why send another pull request? If you are going to "rework" somebody's pull request, better talk about it with the author first.

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2018

@BusyJay Because I cannot find you on Slack when I started work on this (I tried to, not once, because as you see I have some questions about your code).
@overvenus said you rarely respond to people online.

I opened this new PR because I consider #134 too old and it's completely diverse with the current master branch.

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2018

@BusyJay If you have any thoughts about this PR, don't hesitate to tell me now.

@BusyJay

This comment has been minimized.

Copy link
Member

commented Aug 26, 2018

I'm not always online on weekend, better contact me on weekdays if you expect an instant response.

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2018

If you are going to "rework" somebody's pull request, better talk about it with the author first.

Now I'm talking to you. Do you have something to say? Or is it fine to continue this PR?

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2018

I have another question: none of the implemented functions of Read/BufRead for MessageReader are returning Errs. (Some seems to, say, read_to_end returns Err only when read returns Err, read returns Err only when fill_buf returns Err. I assumed these functions not to returning Err at all and you wrote them like that only to fit the interface's functions' type signatures. Then I provided *_directly versions for them. Is this proper? Have I missed anything? The old functions are not removed but changed implementations so there's backward compatibility.

c9ae7e9

@ice1000 ice1000 force-pushed the ice1000:master branch from 4202969 to ea50fbc Aug 26, 2018

@BusyJay

This comment has been minimized.

Copy link
Member

commented Aug 26, 2018

Read and BufRead are traits from std library. MessageReader are only used to support serialization/deserialization, where both traits are commonly used. Implementing them by default makes it easier for people to write their own ser/de hooks.

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2018

This is basically done. PTAL @BusyJay @overvenus

@ice1000
Copy link
Member Author

left a comment

I have some questions, please review.

loop {
if !self.read_done {
if let Some(ref mut msg_f) = self.msg_f {
bytes = try_ready!(msg_f.poll());
if bytes.is_none() {
if bytes.empty {

This comment has been minimized.

Copy link
@ice1000

ice1000 Aug 26, 2018

Author Member

I think I should make sure something here. IMO the expected behavior is:

  • bytes is created by MessageReader::default(), do read_done = true
  • bytes is created with recv_message
    • reads 0, do NOT read_done = true
    • reads non-0, do read_done = true

Because I want to reduce unnecessary Options but here I've changed the semantics and it's probably incorrect. Maybe I should still use Option here, but I'm just making sure.

This comment has been minimized.

Copy link
@BusyJay

BusyJay Aug 27, 2018

Member

Original code makes more sense to me. Option<MessageReader> is not the same as MessageReader. When Option<MessageReader> is None, it's not supposed to be read in the first place. Current implementation is easier to write bugs.

This comment has been minimized.

Copy link
@overvenus

overvenus Aug 27, 2018

Member

Because I want to reduce unnecessary Options ...

Does MessageReader::default() bring extra overhead compare to Options?

This comment has been minimized.

Copy link
@ice1000

ice1000 Aug 27, 2018

Author Member

@overvenus No, I've added a bool to mark whether it's created as default, so MessageReader::default() is different from a zero-sized result of recv_message

@@ -422,8 +428,8 @@ impl<H: ShareCallHolder, T> ResponseStreamImpl<H, T> {
self.msg_f.take();
let msg_f = self.call.call(|c| c.call.start_recv_message())?;
self.msg_f = Some(msg_f);
if let Some(ref data) = bytes {
let msg = (self.resp_de)(data)?;
if !bytes.empty {

This comment has been minimized.

Copy link
@ice1000

ice1000 Aug 26, 2018

Author Member

Same above

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
@ice1000

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2018

Only one test under nightly is not passed, please tell me what have I done wrong. All tests pass locally.

Show resolved Hide resolved src/call/mod.rs Outdated
@ice1000

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

Fixed, PTAL @BusyJay @overvenus

@ice1000
Copy link
Member Author

left a comment

@BusyJay @overvenus I still have some questions

Show resolved Hide resolved src/call/client.rs
Show resolved Hide resolved src/call/client.rs Outdated
Show resolved Hide resolved src/call/server.rs Outdated

@ice1000 ice1000 force-pushed the ice1000:master branch from c9fb36a to 60b2c3f Aug 28, 2018

@siddontang

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

em, any different with origin PR #134 now?

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2018

@siddontang no. I thought there can be some improvements but now not. And PTAL my questions above.

@ice1000
Copy link
Member Author

left a comment

I have another question to @BusyJay . This PR is currently just a migrated version of #134 created by you, and I'm doubting that whether #134 is actually reduce copying.

Show resolved Hide resolved src/call/mod.rs
Show resolved Hide resolved src/call/mod.rs
@ice1000

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2018

The ideal approach is to use the C++ memory directly, which is not present in #134 . Am I expected to achieve that, or keep the current implementation? (Cus it can be lazily read now and it's better than in-place copy)

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2018

And since the destruction of MessageReader is done in Drop::drop, does that mean we can actually read the data without copying if we only call fill_buf_directly without read_directly or read_to_end_directly?

@BusyJay

This comment has been minimized.

Copy link
Member

commented Aug 29, 2018

The ideal approach is to use the C++ memory directly, which is not present in #134 .

That's not true. #134 supports this via BufRead trait.

does that mean we can actually read the data without copying if we only call fill_buf_directly without read_directly or read_to_end_directly?

That's the expected behavior when someone want to achieve zero copy.

When people want a continuous slice, then zero copy is nearly impossible as the underlying buffer is hardly continuous. For such case, Read is the trait they want to use. If they are OK with several byte chunks, then they can use BufRead to get what they want.

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2018

Information get. That looks good

@ice1000 ice1000 force-pushed the ice1000:master branch from 2591000 to 66a0e8d Aug 29, 2018

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2018

image

I've found a lot of C# codes in the submodule, are they used in this project?

Show resolved Hide resolved src/call/mod.rs Outdated
@ice1000
Copy link
Member Author

left a comment

Show resolved Hide resolved src/call/mod.rs Outdated
Show resolved Hide resolved grpc-sys/src/lib.rs Outdated
Show resolved Hide resolved grpc-sys/grpc_wrap.c 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/grpc_wrap.c Outdated
@siddontang

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

seem zero has no any big difference in unary_qps_unconstrained? @overvenus

@ice1000 ice1000 self-assigned this Oct 10, 2018

@ice1000 ice1000 added the Enhancement label Oct 10, 2018

@kennytm
Copy link
Member

left a comment

Rest LGTM I guess 🙃

// Hack: apparently its lifetime depends on `self.slice`. However
// it's not going to be accessed by others directly, hence it's safe
// to mark it static here.
bytes: &'static [u8],

This comment has been minimized.

Copy link
@kennytm

kennytm Oct 10, 2018

Member

Will MessageReader ever be moved? If bytes refer to an inlined GrpcSlice this would point to an invalid location?

This comment has been minimized.

Copy link
@ice1000

ice1000 Oct 10, 2018

Author Member

Reasonable. I'll take a look at it soon but friendly ping @BusyJay here

This comment has been minimized.

Copy link
@ice1000

ice1000 Oct 10, 2018

Author Member

In fill_buf (zero copy read), the self.bytes is returned a borrowed value. Once the MessageReader gets moved, you will no longer ba able to access this returned value, so it becomes invalid and inaccessible. By the way it's expected to call consume each time before fill_buf, it's rather safe in actual usage. Otherwise I expect a segfault (how can we check for that circumstance?).

This comment has been minimized.

Copy link
@ice1000

ice1000 Oct 11, 2018

Author Member

@kennytm Ping

This comment has been minimized.

Copy link
@kennytm

kennytm Oct 11, 2018

Member

@ice1000 At the minimum please add the explanation in the code.


Also, this should be possible without the unsafe hack, by eliminating the self-reference:

  • you could separate MessageReader into two types:

    struct MessageBuffer { 
        buf: *mut GrpcByteBuffer,
        slice: Option<GrpcSlice>,
    }
    
    struct MessageReader<'b> {   // <-- note lifetime
        buf: &'b mut MessageBuffer,
        bytes: &'b [u8],
        length: usize,
    }

    however this probably involves a lot of plumbing to push through this extra lifetime

  • alternatively, you could record the offset instead

    struct MessageReader {
        buf: *mut GrpcByteBuffer,
        slice: Option<GrpcSlice>,
        offset: usize,
        capacity: usize,  
        length: usize,
    }

    and change all references to self.bytes into slice::from_raw_parts(GRPC_SLICE_START_PTR(slice) + offset, capacity - offset). This might cause a little overhead.

The point is that, by removing these lifetime-related unsafe code, we don't need to manually verify when the MessageReader becomes inaccessible or needs a human to check the consumefill_buf order (which is error prone), the compiler itself can assure us these wrong usages will not cause big troubles (panic instead of segfault in the worst case).

This comment has been minimized.

Copy link
@kennytm

kennytm Oct 16, 2018

Member

What I meant was entirely remove the bytes field 🙂

This comment has been minimized.

Copy link
@ice1000

ice1000 Oct 16, 2018

Author Member

@kennytm No, you can't. consume.

This comment has been minimized.

Copy link
@kennytm

kennytm Oct 16, 2018

Member

@ice1000 That's why I said you record the offset in #222 (comment). consume will bump the offset.

This comment has been minimized.

Copy link
@ice1000

ice1000 Oct 16, 2018

Author Member

How reasonable

This comment has been minimized.

Copy link
@ice1000

ice1000 Oct 18, 2018

Author Member

Applied a patch provided by @overvenus , slightly modified to improve readability. This should be fixed by the coming commit.

ice1000 added a commit to ice1000/grpc-rs that referenced this pull request Oct 10, 2018

Fix **Some** tests (copy-paste some codes from pingcap#222 since you …
…guys are not merging my PR)

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>

ice1000 added some commits Oct 10, 2018

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

@overvenus overvenus referenced this pull request Oct 13, 2018

Open

Zero copy for send #238

7 of 7 tasks complete

ice1000 added a commit to ice1000/grpc-rs that referenced this pull request Oct 13, 2018

Fix **Some** tests (copy-paste some codes from pingcap#222 since you …
…guys are not merging my PR)

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>

ice1000 added some commits Oct 13, 2018

Address comment
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Run `cargo fmt` (it's finally working on my computer)
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>

@ice1000 ice1000 force-pushed the ice1000:master branch from bffa547 to d5570e6 Oct 16, 2018

Address comment (Applied a patch by @overvenus, refactored to be more…
… readable)

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@kennytm

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

LGTM. The CI is failing though (missing an unsafe block and some formatting issue).

Fix compilation
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

Fixed. BTW cargo fmt is not displaying anything locally.

@hicqu

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

LGTM.

Fix formatting issues
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

I've finally fixed all the formatting issues from https://travis-ci.org/pingcap/grpc-rs/jobs/443011498

@ice1000

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

Oh no!
image

😭

@overvenus overvenus merged commit c93d7d4 into pingcap:master Oct 19, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@overvenus

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

Thank you! It finally lands on grpc-rs.

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.