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

Support arena allocation for server mode in native protocol #2671

Open
oathdruid opened this issue Jun 25, 2024 · 7 comments
Open

Support arena allocation for server mode in native protocol #2671

oathdruid opened this issue Jun 25, 2024 · 7 comments

Comments

@oathdruid
Copy link
Contributor

Is your feature request related to a problem? (你需要的功能是否与某个问题有关?)
protobuf introduce arena allocation technology long ago from 3.x version, which could help to reduce dynamic memory allocation cost when large message is met.

brpc do request and response heap allocation deep inside the implementation of protocol, make it hard extend to enable arena allocation.
hard like this example use-arena-with-brpc

thanks to the depend on head style inside baidu, tricks like this could also be maintained by keep sync to brpc head version
but in out side world, do this patching once and for all seems impossible

Describe the solution you'd like (描述你期望的解决方法)
design an standalone plugin point make request and response creation easy to customize. or just provide arena allocation mode in native protocol

Describe alternatives you've considered (描述你想到的折衷方案)

Additional context/screenshots (更多上下文/截图)

@chenBright
Copy link
Contributor

class ProtobufMessageManager {
public:
  virtual google::protobuf::Message* GetMessage(const google::protobuf::Message* message_prototype) = 0;
  virtual void ReturnMessage(google::protobuf::Message* message) = 0;
};

User implements ProtobufMessageManager and sets the derived class instance to ServerOptions::pb_message_manager。Then, the server will get request and response from the derived class instance in ProcessRpcRequest, return them in SendRpcResponse

@oathdruid Any suggestion?

@oathdruid
Copy link
Contributor Author

Sounds good. But i'm not sure whether to pack request and response messages up to one transaction, or just leave them separated as above.

Implementation with combined interface could place them on same arena, this make some swap tricks happy. But make the interface weird indeed.

@chenBright
Copy link
Contributor

chenBright commented Jul 10, 2024

Good suggestion. I've thought about this question too. Maybe we should use such an interface:

class ProtobufMessageManager {
public:
    virtual google::protobuf::Message* GetRequestMessage(
        const google::protobuf::Service& service,
        const google::protobuf::MethodDescriptor& method) = 0;

    virtual void ReturnRequestMessage(google::protobuf::Message* message) = 0;

    virtual google::protobuf::Message* GetResponseMessage(
        const google::protobuf::Service& service,
        const google::protobuf::MethodDescriptor& method) = 0;

    virtual void ReturnResponseMessage(google::protobuf::Message* message) = 0;
};

Let the user decide whether to pack request and response messages up to one transaction, or just leave them separated.

@oathdruid
Copy link
Contributor Author

Single ProtobufMessageManager is shared between concurrent rpcs. So two concurrent rpc may invoke their get request and get response interleaved.

But without some identifier, ProtobufMessageManager could not distinguish them, and cannot pair them up properly on same arena.

@chenBright
Copy link
Contributor

struct ProtobufMessage {
    google::protobuf::Message* request{NULL};
    google::protobuf::Message* response{NULL};
};

class ProtobufMessageManager {
public:
    virtual ProtobufMessage Get(const google::protobuf::Service& service,
                                const google::protobuf::MethodDescriptor& method) = 0;

    virtual void Return(ProtobufMessage protobuf_message) = 0;
};

How about this interface?

@oathdruid
Copy link
Contributor Author

Yes, that's what I said a little weird but useful one.

@chenBright
Copy link
Contributor

chenBright commented Jul 12, 2024

Maybe we can change ProtobufMessage to RpcPBMessages, and ProtobufMessageManager to RpcPBMessageManager, which are only used for rpc. This would make more sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants