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

denc: add encode/decode for basic_sstring #15135

Merged
merged 3 commits into from May 31, 2017

Conversation

Projects
None yet
5 participants
@cbodley
Contributor

cbodley commented May 17, 2017

also adds calls to reserve() when decoding into boost::container::flat_set/flat_map

note: depends on #15124 to fix build issues in osd/osd_types

@cbodley cbodley added the common label May 17, 2017

@cbodley

This comment has been minimized.

Contributor

cbodley commented May 18, 2017

@tchaikov thanks for updating this to use denc_traits - will squash your commit after rerunning dencoder tests

@cbodley

This comment has been minimized.

Contributor

cbodley commented May 18, 2017

had to add a bound_encode() function to make it build, but it's back to passing test/encoding/check-generated.sh

(still depends on #15124 to compile)

{
s.reset();
if (len) {
s.append(reinterpret_cast<const Char*>(p.get_pos_add(len)), len);

This comment has been minimized.

@cbodley

cbodley May 18, 2017

Contributor

@tchaikov so there's some higher-level logic that guarantees all of len will be present in this bufferptr?

This comment has been minimized.

@tchaikov

tchaikov May 18, 2017

Contributor

yes, if we use the old-style decode(), we will be using the wrapper of denc() : https://github.com/ceph/ceph/blob/master/src/include/denc.h#L1432. copy_shallow() tries to do the shallow copy but if what we want exceeds the segment's length, it will do in the expensive way.

This comment has been minimized.

@cbodley

cbodley May 18, 2017

Contributor

ok, i didn't realize it had to reallocate.. and when it does make a copy, it throws it away so later entries can't take advantage of it? it looks like there's room for improvement here (someday)

@adamemerson adamemerson self-assigned this May 18, 2017

cbodley added some commits May 16, 2017

denc: add reserve() to boost::flat_set/map decode
by reserving space up front, we avoid rellocating storage as elements
are inserted

Signed-off-by: Casey Bodley <cbodley@redhat.com>
sstring: add denc() support
Signed-off-by: Casey Bodley <cbodley@redhat.com>
Signed-off-by: Kefu Chai <kchai@redhat.com>
@cbodley

This comment has been minimized.

Contributor

cbodley commented May 18, 2017

rebased now that #15124 merged, should be able to build now

@yuriw

This comment has been minimized.

Contributor

yuriw commented May 24, 2017

@cbodley
suspect this PR broke build
see:

In file included from /build/ceph-12.0.2-1543-g3ec7981/src/test/encoding/types.h:20:0,
                 from /build/ceph-12.0.2-1543-g3ec7981/src/test/encoding/ceph_dencoder.cc:36:
/build/ceph-12.0.2-1543-g3ec7981/src/test/encoding/test_sstring.h: In member function 'void sstring_wrapper::decode(ceph::buffer::list::iterator&)':
/build/ceph-12.0.2-1543-g3ec7981/src/test/encoding/test_sstring.h:23:19: error: no matching function for call to 'decode(sstring_wrapper::sstring16&, ceph::buffer::list::iterator&)'
     ::decode(s1, p);

in https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=xenial,DIST=xenial,MACHINE_SIZE=huge/3490//consoleFull

@cbodley

This comment has been minimized.

Contributor

cbodley commented May 25, 2017

thanks @yuriw, i'll look into it

test: add denc tests for sstring
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley

This comment has been minimized.

Contributor

cbodley commented May 25, 2017

i pushed a fix, and verified that all packages built: https://shaman.ceph.com/builds/ceph/wip-denc-sstring/

@liewegas liewegas merged commit 0a8beea into ceph:master May 31, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@cbodley cbodley deleted the cbodley:wip-denc-sstring branch May 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment