-
Notifications
You must be signed in to change notification settings - Fork 6k
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
os: Transaction uses append_hole() to minimize bl:_buffers inflation. #24459
os: Transaction uses append_hole() to minimize bl:_buffers inflation. #24459
Conversation
{ | ||
bufferptr other_op_bl_ptr(other.op_bl.length()); | ||
other.op_bl.copy(0, other.op_bl.length(), other_op_bl_ptr.c_str()); | ||
other_op_bl.append(std::move(other_op_bl_ptr)); |
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 just a side quest about std::move
.
src/include/buffer.h
Outdated
@@ -491,12 +486,7 @@ namespace buffer CEPH_BUFFER_API { | |||
iterator(bl_t *l, unsigned o=0); | |||
iterator(bl_t *l, unsigned o, list_iter_t ip, unsigned po); | |||
|
|||
#ifdef BL_BACKWARD_COMPAT | |||
void advance(int o); |
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.
my concern is that this could break existing librados clients at compile-time and run-time, which still call buffer::list::iterator::advance(int)
.
at compile time, we will have:
use of deleted function ‘void ceph::list::iterator::advance(int)’
at run-time, we will have
./test: symbol lookup error: ./test: undefined symbol: _ZN4ceph6buffer3Foo4funcEi
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.
Yeah, I was afraid about the backward compatibility as too. However, @liewegas pointed out we need to care only between minor releases. If my understanding is correct, then it would be enough just to not backport such changes.
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.
we do have compatibility tests which
- install librados client of an old version, for example, mimic, using rpm/deb
- backup the executable of the client to somewhere
- install librados of current version, i.e., nautilus, using rpm/deb
- put back the executable
- run it.
see
for examples of how we broke/fixed the ABI 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.
Hrm, I thought this was only a problem when we did it in a minor bugfix update. @jdurgin ?
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.
Oh we've made that argument before, but if that's our claim we also probably need to get better about bumping the librados version and shipping multiple versions within each release cycle so that users have a sane upgrade cycle for their clients as well as their servers. :/
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 reason we don't bump the so version is because these sorts of changes only affect the C++ interface, but librados.so also contains the C library, which libvirt, qemu, and others depend on.
Since c++ build changes already break the ABI regularly between major versions, I'm fine with doing that. I guess the concern is more around API changes.
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 managed to break the C++ ABI couple times before. before breaking it again, i think we probably need to take the following issues into consideration, otherwise we will need to revert this change later
- http://tracker.ceph.com/issues/21573
- http://tracker.ceph.com/issues/17809
- http://tracker.ceph.com/issues/17809 (this issue resembles the one we could run into if this PR gets merged)
cc @dillaman
The new patch here (just the final one in ObjectStore, right?) LGTM, anyway. |
This could be useful as the used `::append(const ptr&)` variant does not provide any measures (like coalescing adjacent bptrs) to prevent inflation of the internal structures of a bufferlist. That is, each call was reflected in dynamic node allocation for the `_buffers` list. Secondary effect of this behavior is increased iteration cost driven by large number of elements likely scattered over memory. Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
208e0ef
to
bf67929
Compare
Rebased to current |
* refs/pull/24459/head: os: Transaction uses append_hole() to minimize bl:_buffers inflation. Reviewed-by: Josh Durgin <jdurgin@redhat.com>
This could be useful as the currently employed
::append(const ptr&)
variant does not provide any measures (like coalescing adjacent bptrs) to prevent inflation of the internal structures of a bufferlist. That is, each call was reflected in dynamic node allocation for the_buffers
list. Secondary effect of this behavior is increased iteration cost driven by large number of elements likely scattered over memory.This is derivative work of PR #24229.