-
Notifications
You must be signed in to change notification settings - Fork 53
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
Python: Changed handling of large requests to transfer them as leaked pointers #1655
Conversation
9e9eb9f
to
0f8224b
Compare
""" | ||
|
||
# TODO: Allow passing different encoding options | ||
return bytes(arg, encoding="utf8") |
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.
It is not completely platform agnostic - we need to add handling in utf8 usage.
For example AIX and Solaris are unix based OS that don't use utf8 in their default. It is possible to configure new versions to use utf8, but the old versions don't even have the option.
If at some point we'll want to support windows, it can be even more problematic.
It might not be relevant enough at that point, but we need more than allowing the passing of different encoding. To avoid crashing or producing incorrect results, we need to check the OS we are running on when no specific encoding is passed.
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.
ATM we will support only utf8 strings and bytes as passed arguments and return values. Later on we will support encoder as a client configuration which will allow more encoding options. If the user doesn't use utf8 strings, he can use bytes instead
|
||
from glide.constants import TResult | ||
|
||
DEFAULT_TIMEOUT_IN_MILLISECONDS: int = ... | ||
MAX_REQUEST_ARGS_LEN: int = ... |
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.
Please explain somewhere what is that size, and how is it possible that this size come from rust but is relevant for all the languages. Something here is a bit confusing.
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.
The variable is defined and documented in socket_listener.rs, i'm only importing it here and exporting it to the python wrapper
It is not clear enough what are you doing here. |
We discussed in the office - this is a part of the client's design. Since large messages aren't being properly handled with protobuf, we are using an FFI call with the args vec, allocating it on the heap on the rust side and leaking it so it won't be deleted by the GC. Then we pass only the pointer on the protobuf message, sending the protobuf message through the socket, and getting back the leaked memory from the received pointer in the core side. |
Large protobuf messages are extremely slow to decode/encode, and with large messages (e.g., 512MB), the client becomes unresponsive. We resolve this issue by passing the arguments as a leaked pointer instead of a bytes vector when the message size exceeds the limit.
When we merge in the bytes/string support, we should change these functions to the relevant types. Added TODOs in the code for that matter.