do not copy when deserialize #9209
Conversation
| if (!input->GetDirectBufferPointer(&data, &size_to_write)) { | ||
| return false; | ||
| } | ||
| // TODO(gongwb): don't copy if it's aligned? |
There was a problem hiding this comment.
Maybe we need to copy anyway? I guess the buffer that we copy from will become invalid afterwards.
There was a problem hiding this comment.
ByteBuffer is in userspace, and it uses the Slice to manage discontinuous memory.
If our data is in a single slice, we can use it directly--It needs to serialize the send buffer manually and it's dangerous.
There was a problem hiding this comment.
Sorry maybe I did not made it clear, I thought we should not use the underlying buffer, since the buffer belongs to protobuf, and the content could change?
There was a problem hiding this comment.
这个也是猜的：
https://github.com/gongweibao/Paddle/blob/optsend/paddle/fluid/operators/detail/grpc_service.h#L65
好像内存可以保留下来。另外bytes的对应数据就是一块内存。
There was a problem hiding this comment.
I changed this comment to "Can we avoid copy?".
| auto* slr = var->GetMutable<framework::SelectedRows>(); | ||
| int64_t* rows_data = slr->mutable_rows()->data(); | ||
|
|
||
| // copy rows CPU data, GPU data will be copied lazly |
| varmsg.SerializeToString(&str); | ||
|
|
||
| // message bytebuffer | ||
| ::grpc::Slice slices_2[1]; |
There was a problem hiding this comment.
Could you test with a bigger sized buffer? A 1 sized buffer perhaps is not general enough. Or even better, test in a loop, like for i := 1; i < 100; i++.
There was a problem hiding this comment.
There are two styles of repeated field: packed and not packed and protobuf may change it auto even you don't specify packed.
- packed: <tag,data> <tag,data>....
- not packed: <tag,len><data, data,data..>...
This code only guarantees we can parse manual serialized buffer and the standard buffer.
The byte buffer may have many slices already.
| return false; | ||
| } | ||
| // TODO(gongwb): don't copy if it's aligned? | ||
| memcpy(reinterpret_cast<void*>(p), data, size_to_write); |
There was a problem hiding this comment.
can use memory::Copy here
| return true; | ||
| } | ||
|
|
||
| int TensorResponse::Parse(::grpc::ByteBuffer& byte_buffer, |
There was a problem hiding this comment.
we don't actually need a class TensorResponse, just put functions in proto_encoder_helper.h (or .cc) and put this content in DeserializeFromByteBuffer
There was a problem hiding this comment.
This class is needed for parse tensor response.
| namespace operators { | ||
| namespace detail { | ||
|
|
||
| class TensorResponse { |
There was a problem hiding this comment.
Should call it VariableResponse or VariableMessageParser because the response contains not only tensor
There was a problem hiding this comment.
The file name should also be changed I suppose.
| int Parse(const ::grpc::ByteBuffer& byte_buffer, | ||
| const platform::DeviceContext& dev_ctx); | ||
|
|
||
| // should call parse first. |
There was a problem hiding this comment.
Interface comments are too simple and not explaining what the function does.
|
Move to #9271 which is the whole PR. |
Fix part of #8638