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

include/encoding: do not try to be clever with list encoding #7913

Merged
merged 1 commit into from Mar 14, 2016

Conversation

liewegas
Copy link
Member

@liewegas liewegas commented Mar 3, 2016

I'm skeptical that this is faster, since the copy_in call needs to
figure out where to place the length value.

Signed-off-by: Sage Weil sage@redhat.com

@yuyuyu101
Copy link
Member

I'm not sure the performance effect about this PR, I guess you may pay attention to bufferlist and encode/decode performance part.

I have two ideas for this topic:

  1. avoid decode memcpy: the performance problem of decode isn't we need to copy a lot of bytes, but is we need call memcpy too many times. Llike map<string, bufferlist> or vector<string/bufferlist> structure, we need to call copy ops at least 2*element number. So for Message receive, I think we may could build a readonly map/vector alike structure, this structure just is owns some pointers to reference the raw message data buffer, we only need to keep message ref. Because for normal osd ops, we won't change map/vector from message data. So we could replace actual vector/map to a simple reference structure.
  2. implement a lightweight bufferlist. I think we could provide with a lightweight bufferlist which use uint64_t as ref type instead of atomic. Because for most certain cases, we could keep lightweight bufferlist internal module use. For good design module, we only need to use a little shared bufferlist(current impl). This change also can help to avoid potential bufferlist abuse problem. From my last perf, each rados op will cause at least 200 atomic inc and 200 dec(only from bufferlist). We may not get the detail performance effect for the atomic ops, but I think the bus lock shouldn't be free enough to ignore.

In c++11 std::list::size() is O(1).  No need to be clever.

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
Copy link
Member Author

Rebased, updated comment.

@liewegas liewegas added this to the jewel milestone Mar 10, 2016
liewegas added a commit that referenced this pull request Mar 14, 2016
include/encoding: do not try to be clever with list encoding
@liewegas liewegas merged commit 4b27b34 into ceph:master Mar 14, 2016
@liewegas liewegas deleted the wip-list-encode branch March 14, 2016 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants