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
Merged

Zero copy for receive #222

merged 25 commits into from Oct 19, 2018

Conversation

@ice1000
Copy link
Contributor

@ice1000 ice1000 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
Copy link
Contributor Author

@ice1000 ice1000 commented Aug 25, 2018

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

https://github.com/pingcap/grpc-rs/blob/8fff94905ca9bccc1875f7f5a9cae7d12aa17196/src/call/server.rs#L79

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

@BusyJay
Copy link
Member

@BusyJay BusyJay 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
Copy link
Contributor Author

@ice1000 ice1000 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
Copy link
Contributor Author

@ice1000 ice1000 commented Aug 26, 2018

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

@BusyJay
Copy link
Member

@BusyJay BusyJay commented Aug 26, 2018

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

@ice1000
Copy link
Contributor Author

@ice1000 ice1000 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
Copy link
Contributor Author

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

@BusyJay
Copy link
Member

@BusyJay BusyJay 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
Copy link
Contributor Author

@ice1000 ice1000 commented Aug 26, 2018

This is basically done. PTAL @BusyJay @overvenus

Copy link
Contributor Author

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

@ice1000

ice1000 Aug 26, 2018
Author Contributor

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.

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

@overvenus

overvenus Aug 27, 2018
Contributor

Because I want to reduce unnecessary Options ...

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

This comment has been minimized.

@ice1000

ice1000 Aug 27, 2018
Author Contributor

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

@ice1000

ice1000 Aug 26, 2018
Author Contributor

Same above

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

@ice1000 ice1000 commented Aug 26, 2018

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

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

@ice1000 ice1000 commented Aug 28, 2018

Fixed, PTAL @BusyJay @overvenus

Copy link
Contributor Author

@ice1000 ice1000 left a comment

@BusyJay @overvenus I still have some questions

src/call/client.rs Show resolved Hide resolved
src/call/client.rs Outdated Show resolved Hide resolved
src/call/server.rs Outdated Show resolved Hide resolved
@siddontang
Copy link
Collaborator

@siddontang siddontang commented Aug 28, 2018

em, any different with origin PR #134 now?

@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Aug 29, 2018

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

Copy link
Contributor Author

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

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

@ice1000 ice1000 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
Copy link
Contributor Author

@ice1000 ice1000 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
Copy link
Member

@BusyJay BusyJay 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
Copy link
Contributor Author

@ice1000 ice1000 commented Aug 29, 2018

Information get. That looks good

@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Sep 1, 2018

image

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

src/call/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@ice1000 ice1000 left a comment

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 Show resolved Hide resolved
@kennytm kennytm dismissed their stale review Sep 3, 2018

(resolved)

@ice1000 ice1000 closed this Sep 3, 2018
@@ -858,3 +836,18 @@ grpcwrap_ssl_server_credentials_create(
}

#endif

/* Sanity check for complicated types */
//#define alignof(type)((size_t) & ((struct {char c; type d; } *) 0)->d)

This comment has been minimized.

@overvenus

overvenus Oct 10, 2018
Contributor

Remove it.

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@siddontang
Copy link
Collaborator

@siddontang siddontang commented Oct 10, 2018

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

@ice1000 ice1000 self-assigned this Oct 10, 2018
Copy link

@kennytm kennytm 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.

@kennytm

kennytm Oct 10, 2018

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

This comment has been minimized.

@ice1000

ice1000 Oct 10, 2018
Author Contributor

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

This comment has been minimized.

@ice1000

ice1000 Oct 10, 2018
Author Contributor

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.

@ice1000

ice1000 Oct 11, 2018
Author Contributor

@kennytm Ping

This comment has been minimized.

@kennytm

kennytm Oct 11, 2018

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

@kennytm

kennytm Oct 16, 2018

What I meant was entirely remove the bytes field 🙂

This comment has been minimized.

@ice1000

ice1000 Oct 16, 2018
Author Contributor

@kennytm No, you can't. consume.

This comment has been minimized.

@kennytm

kennytm Oct 16, 2018

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

This comment has been minimized.

@ice1000

ice1000 Oct 16, 2018
Author Contributor

How reasonable

This comment has been minimized.

@ice1000

ice1000 Oct 18, 2018
Author Contributor

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

ice1000 added 2 commits Oct 10, 2018
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@overvenus overvenus mentioned this pull request Oct 13, 2018
7 tasks done
ice1000 added 4 commits Oct 13, 2018
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
… readable)

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

@kennytm kennytm commented Oct 18, 2018

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

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

@ice1000 ice1000 commented Oct 18, 2018

Fixed. BTW cargo fmt is not displaying anything locally.

@hicqu
Copy link
Contributor

@hicqu hicqu commented Oct 18, 2018

LGTM.

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

@ice1000 ice1000 commented Oct 19, 2018

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

@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Oct 19, 2018

Oh no!
image

😭

@overvenus overvenus merged commit c93d7d4 into tikv:master Oct 19, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@overvenus
Copy link
Contributor

@overvenus overvenus 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants