-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
rpc : backend refactoring #9912
Conversation
ggml/src/ggml-rpc.cpp
Outdated
// output serialization format: | ptr (8 bytes) | size (8 bytes) | | ||
memcpy(&response.ptr, output.data(), sizeof(response.ptr)); | ||
memcpy(&response.size, output.data() + sizeof(response.ptr), sizeof(response.size)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is much better than the previous version. The idea would be to read directly into the struct from the network.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this, I get what you mean. Could you take another look and let me know if you agree?
I also noticed that all commands have a known output size, maybe we can skip sending output size from the server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's exactly what I meant.
I also noticed that all commands have a known output size, maybe we can skip sending output size from the server?
I don't have a strong opinion about this either way. Some commands may have variable output size in the future, for example get_description
in the device interface, and having the output size may help simplify the code a bit in that case, but it is probably not very important either way.
138c9b2
to
4631edc
Compare
4631edc
to
38a671a
Compare
Use structs for RPC request/response messages
38a671a
to
c9e549c
Compare
#pragma pack(1) | ||
struct rpc_msg_alloc_buffer_req { | ||
uint64_t size; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not really how pack(1)
works. every struct after a single pack(1)
will be affected, only after another pack()
(empty), it will be the value from before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, I have posted #9959 to fix this
* rpc : refactor backend Use structs for RPC request/response messages * rpc : refactor server
* rpc : refactor backend Use structs for RPC request/response messages * rpc : refactor server
* rpc : refactor backend Use structs for RPC request/response messages * rpc : refactor server
* rpc : refactor backend Use structs for RPC request/response messages * rpc : refactor server
Introduce structs for each request/response and separate the code which deals with serialization/deserialization.