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

about support protobuf arena #1175

Closed
wants to merge 3 commits into from

Conversation

heyuyi0906
Copy link
Contributor

@heyuyi0906 heyuyi0906 commented Jul 18, 2020

#535
麻烦看下这个实现是否可以被接受,当前先只改了baidu rpc protocol。

@yjhjstz
Copy link
Member

yjhjstz commented Aug 10, 2020

赞,好奇有多少的性能提升?

@heyuyi0906
Copy link
Contributor Author

在大包情况下,自测发现对于性能提升还是比较明显的。
support_pb_arena分支测试结果:
image
master分支测试结果:
image

测试条件:
机器:8核 Intel(R) Xeon(R) Platinum 8255C CPU @ 2.50GHz
protobuf版本:v3.6.1
压测工具:rpc_press thread_num=50
协议:

syntax="proto2";
package example;

option cc_generic_services = true;

message Obj {
    optional uint64 val = 1;
};

message EchoRequest {
    required string message = 1;
    repeated double array = 2;
    repeated Obj objs = 3;
};

message EchoResponse {
    required string message = 1;
    repeated double array = 2;
    repeated Obj objs = 3;
};

service EchoService {
      rpc Echo(EchoRequest) returns (EchoResponse);
};

server处理逻辑:

        response->set_message(request->message());

        for (int i = 0; i < request->array_size(); ++i) {
            response->add_array(request->array(i));
        }

        for (int i = 0; i < request->objs_size(); ++i) {
            auto o = response->add_objs();
            o->set_val(request->objs(i).val());
        }

压测input:

{
  "message":"xxx......", 
  "array":[2.9,2.9,......],
  "objs":[{"val":10},......]
}

message字符串长度16384
array包含8192个double
objs包含8192个对象

@zyearn
Copy link
Member

zyearn commented Aug 10, 2020

大包看起来效果不错,方便看下不同包大小的场景下(比如16B,32B,64B,128B,256B,512B...)的性能差异么,这些包更典型些

@heyuyi0906
Copy link
Contributor Author

好,我补充下这块的数据


template<class T>
struct ArenaObjDeleter {
constexpr ArenaObjDeleter() noexcept = default;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为什么不把这个 mark 成 delete 呢?

del(!cntl->get_memory_arena()->OwnObject());
std::unique_ptr<const google::protobuf::Message,
butil::ArenaObjDeleter<const google::protobuf::Message>
> recycle_req(req, del);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这种indent方式有些怪,可以统一成下面这样吗


    std::unique_ptr<google::protobuf::Message, 
            butil::ArenaObjDeleter<google::protobuf::Message>> req;
    std::unique_ptr<google::protobuf::Message, 
            butil::ArenaObjDeleter<google::protobuf::Message>> res;

}
return _arena;
}
void share_memory_arena(Controller *cntl) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为保持和现有代码风格一致,引用和指针符号统一对齐到左边与类型挨着吧


void operator()(T *ptr) const {
if (_own_obj) {
VLOG(199) << "delete!";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VLOG(RPC_VLOG_LEVEL)

>
CreateMessage(const google::protobuf::Message &proto_type) {
google::protobuf::Message *msg = proto_type.New();
VLOG(199) << __FUNCTION__;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FUNCTION 是哪里定义的宏,用__func__变量会更标准些吧

@limindlmu
Copy link

请问这个pr没合入是还存在什么问题么?

@wwbmmm
Copy link
Contributor

wwbmmm commented Sep 2, 2024

请问这个pr没合入是还存在什么问题么?

现在可以使用 #2718 中提供的功能,通过自定义RpcPBMessageFactory来支持arena

@chenBright
Copy link
Contributor

chenBright commented Sep 2, 2024

请问这个pr没合入是还存在什么问题么?

@limindlmu 可以看看#2751

@chenBright
Copy link
Contributor

Closed this as completed #2751 .

@chenBright chenBright closed this Oct 29, 2024
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

Successfully merging this pull request may close these issues.

8 participants