From 681afb7b7673696ef05ca9c6c9dabd728e7f1eb5 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Tue, 23 May 2017 15:28:23 +0800 Subject: [PATCH] denc: add with_dencv to denc_traits the "v" in dencv is like the "v" in readv(2), so the decode() will not try to copy the bufferlist for a continous one if the bufferlist is segmented *and* its length is greater than a CEPH_PAGE_SIZE (4K). copying a memory chunk could be expensive if the decoded bufferlist is huge, so we should try to avoid this. this could happen when we read the buffer from bluestore. and drop the partial specialization for denc() which tries to differentiate traits::featured and !traits::featured, it does not matter to decode() if the type supports feature or not. the encode() does. Signed-off-by: Kefu Chai --- src/include/denc.h | 175 ++++++++++++++++++++++++++--- src/include/fs_types.h | 1 + src/include/interval_set.h | 12 ++ src/include/object.h | 1 + src/os/bluestore/bluestore_types.h | 1 + src/test/encoding.cc | 104 +++++++++++++++++ 6 files changed, 279 insertions(+), 15 deletions(-) diff --git a/src/include/denc.h b/src/include/denc.h index e3ff107fb1e8c3..758d3545850c4e 100644 --- a/src/include/denc.h +++ b/src/include/denc.h @@ -44,11 +44,12 @@ #include "buffer.h" #include "byteorder.h" -template +template struct denc_traits { static constexpr bool supported = false; static constexpr bool featured = false; static constexpr bool bounded = false; + static constexpr bool with_dencv = false; }; @@ -138,6 +139,7 @@ struct denc_traits { static constexpr bool supported = true; static constexpr bool bounded = false; static constexpr bool featured = false; + static constexpr bool with_denc = false; static void bound_encode(const T &o, size_t& p, uint64_t f=0); static void encode(const T &o, buffer::list::contiguous_appender& p, uint64_t f=0); @@ -151,6 +153,7 @@ struct denc_traits { static constexpr bool supported = true; static constexpr bool bounded = false; static constexpr bool featured = true; + static constexpr bool with_denc = false; static void bound_encode(const T &o, size_t& p, uint64_t f); static void encode(const T &o, buffer::list::contiguous_appender& p, uint64_t f); @@ -190,6 +193,12 @@ struct denc_traits { declared via WRITE_CLASS_DENC, although you can also invoke them explicitly in your code. + - These methods are optimised for contiguous buffer, but denc() will try + rebuild a contigous one if the decoded bufferlist is segmented. If you are + concerned about the cost, you might want to define yet another method: + + void decode(buffer::list::iterator &p); + - These can be defined either explicitly (as above), or can be "magically" defined all in one go using the DENC macro and DENC_{START,FINISH} helpers (which work like the legacy {ENCODE,DECODE}_{START,FINISH} macros): @@ -236,6 +245,7 @@ struct denc_traits< static constexpr bool supported = true; static constexpr bool featured = false; static constexpr bool bounded = true; + static constexpr bool with_dencv = true; static void bound_encode(const T &o, size_t& p, uint64_t f=0) { p += sizeof(T); } @@ -248,6 +258,9 @@ struct denc_traits< uint64_t f=0) { o = *(T *)p.get_pos_add(sizeof(o)); } + static void decode(T& o, buffer::list::iterator &p) { + p.copy(sizeof(T), reinterpret_cast(&o)); + } }; @@ -302,6 +315,7 @@ struct denc_traits< static constexpr bool supported = true; static constexpr bool featured = false; static constexpr bool bounded = true; + static constexpr bool with_dencv = true; using etype = typename _denc::ExtType::type; static void bound_encode(const T &o, size_t& p, uint64_t f=0) { p += sizeof(etype); @@ -314,6 +328,11 @@ struct denc_traits< uint64_t f=0) { o = *(etype*)p.get_pos_add(sizeof(etype)); } + static void decode(T& o, buffer::list::iterator &p) { + etype e; + p.copy(sizeof(etype), reinterpret_cast(&e)); + o = e; + } }; // varint @@ -594,6 +613,40 @@ inline typename std::enable_if + struct has_legacy_denc : std::false_type + {}; + template + struct has_legacy_denc() + .decode(std::declval()))> : std::true_type + { + static void decode(T& v, bufferlist::iterator& p) { + v.decode(p); + } + }; + template + struct has_legacy_denc::with_dencv>::type> : std::true_type + { + static void decode(T& v, bufferlist::iterator& p) { + denc_traits::decode(v, p); + } + }; +} + +template, + typename has_legacy_denc=_denc::has_legacy_denc> +inline typename std::enable_if::type denc( + T& o, + buffer::list::iterator& p, + uint64_t features=0) +{ + has_legacy_denc::decode(o, p); +} // --------------------------------------------------------------------- // base types and containers @@ -610,6 +663,7 @@ struct denc_traits,A>> { static constexpr bool supported = true; static constexpr bool featured = false; static constexpr bool bounded = false; + static constexpr bool with_dencv = true; static void bound_encode(const value_type& s, size_t& p, uint64_t f=0) { p += sizeof(uint32_t) + s.size(); @@ -627,6 +681,13 @@ struct denc_traits,A>> { ::denc(len, p); decode_nohead(len, s, p); } + static void decode(value_type& s, buffer::list::iterator& p) + { + uint32_t len; + ::denc(len, p); + s.clear(); + p.copy(len, s); + } static void decode_nohead(size_t len, value_type& s, buffer::ptr::iterator& p) { s.clear(); @@ -648,6 +709,7 @@ struct denc_traits { static constexpr bool supported = true; static constexpr bool featured = false; static constexpr bool bounded = false; + static constexpr bool with_dencv = true; static void bound_encode(const bufferptr& v, size_t& p, uint64_t f=0) { p += sizeof(uint32_t) + v.length(); } @@ -661,6 +723,18 @@ struct denc_traits { ::denc(len, p); v = p.get_ptr(len); } + static void decode(bufferptr& v, buffer::list::iterator& p) { + uint32_t len; + ::denc(len, p); + bufferlist s; + p.copy(len, s); + if (len) { + if (s.get_num_buffers() == 1) + v = s.front(); + else + v = buffer::copy(s.c_str(), s.length()); + } + } }; // @@ -671,6 +745,7 @@ struct denc_traits { static constexpr bool supported = true; static constexpr bool featured = false; static constexpr bool bounded = false; + static constexpr bool with_dencv = true; static void bound_encode(const bufferlist& v, size_t& p, uint64_t f=0) { p += sizeof(uint32_t) + v.length(); } @@ -685,6 +760,12 @@ struct denc_traits { v.clear(); v.push_back(p.get_ptr(len)); } + static void decode(bufferlist& v, buffer::list::iterator& p) { + uint32_t len; + ::denc(len, p); + v.clear(); + p.copy(len, v); + } static void encode_nohead(const bufferlist& v, buffer::list::contiguous_appender& p) { p.append(v); @@ -712,6 +793,8 @@ struct denc_traits< static constexpr bool supported = true; static constexpr bool featured = a_traits::featured || b_traits::featured ; static constexpr bool bounded = a_traits::bounded && b_traits::bounded; + static constexpr bool with_dencv = (a_traits::with_dencv && + b_traits::with_dencv); template static typename std::enable_if + static typename std::enable_if::type + decode(std::pair& v, buffer::list::iterator& p, + uint64_t f = 0) { + denc(v.first, p); + denc(v.second, p); + } }; namespace _denc { @@ -763,6 +853,7 @@ namespace _denc { static constexpr bool supported = true; static constexpr bool featured = traits::featured; static constexpr bool bounded = false; + static constexpr bool with_dencv = traits::with_dencv; template static typename std::enable_if + static typename std::enable_if::type + decode(container& s, buffer::list::iterator& p) { + uint32_t num; + denc(num, p); + decode_nohead(num, s, p); + } // nohead template @@ -862,6 +960,18 @@ namespace _denc { Details::insert(s, std::move(t)); } } + template + static typename std::enable_if::type + decode_nohead(size_t num, container& s, + buffer::list::iterator& p) { + s.clear(); + Details::reserve(s, num); + while (num--) { + T t; + denc(t, p); + Details::insert(s, std::move(t)); + } + } }; template @@ -998,6 +1108,7 @@ struct denc_traits< static constexpr bool supported = true; static constexpr bool featured = traits::featured; static constexpr bool bounded = traits::bounded; + static constexpr bool with_dencv = traits::with_dencv; template static typename std::enable_if::bounded); static constexpr bool featured = (denc_traits::featured || tuple_traits::featured); + static constexpr bool with_dencv = (denc_traits::with_dencv || + tuple_traits::with_dencv); }; template<> struct tuple_traits<> { static constexpr bool supported = true; static constexpr bool bounded = true; static constexpr bool featured = false; + static constexpr bool with_dencv = true; }; } @@ -1207,6 +1321,7 @@ struct denc_traits< static constexpr bool supported = true; static constexpr bool featured = traits::featured; static constexpr bool bounded = traits::bounded; + static constexpr bool with_dencv = traits::with_dencv; template @@ -1269,6 +1384,7 @@ struct denc_traits< static constexpr bool supported = true; static constexpr bool featured = traits::featured; static constexpr bool bounded = false; + static constexpr bool with_dencv = traits::with_dencv; template static typename std::enable_if::type @@ -1313,6 +1429,19 @@ struct denc_traits< } } + template + static typename std::enable_if::type + decode(boost::optional& v, buffer::list::iterator& p) { + bool x; + denc(x, p); + if (x) { + v = T{}; + denc(*v, p); + } else { + v = boost::none; + } + } + template static typename std::enable_if::type encode_nohead(const boost::optional& v, @@ -1345,6 +1474,7 @@ struct denc_traits { static constexpr bool supported = true; static constexpr bool featured = false; static constexpr bool bounded = true; + static constexpr bool with_dencv = true; static void bound_encode(const boost::none_t& v, size_t& p) { p += sizeof(bool); @@ -1369,6 +1499,7 @@ struct denc_traits { static constexpr bool supported = true; \ static constexpr bool featured = false; \ static constexpr bool bounded = b; \ + static constexpr bool with_dencv = _denc::has_legacy_denc::value;\ static void bound_encode(const T& v, size_t& p, uint64_t f=0) { \ v.bound_encode(p); \ } \ @@ -1388,6 +1519,7 @@ struct denc_traits { static constexpr bool supported = true; \ static constexpr bool featured = true; \ static constexpr bool bounded = b; \ + static constexpr bool with_dencv = _denc::has_legacy_denc::value;\ static void bound_encode(const T& v, size_t& p, uint64_t f) { \ v.bound_encode(p, f); \ } \ @@ -1432,34 +1564,47 @@ inline typename std::enable_if> +template> inline typename std::enable_if::type decode( + traits::with_dencv>::type decode( T& o, bufferlist::iterator& p) { if (p.end()) throw buffer::end_of_buffer(); - // ensure we get a contigous buffer... until the end of the - // bufferlist. we don't really know how much we'll need here, - // unfortunately. hopefully it is already contiguous and we're just - // bumping the raw ref and initializing the ptr tmp fields. - bufferptr tmp; - bufferlist::iterator t = p; - t.copy_shallow(p.get_bl().length() - p.get_off(), tmp); - auto cp = tmp.begin(); - traits::decode(o, cp); - p.advance((ssize_t)cp.get_offset()); + auto bl = p.get_bl(); + auto remaining = bl.length() - p.get_off(); + // it is expensive to rebuild a contigous buffer and drop it, so avoid this. + if (bl.get_num_buffers() > 1 && remaining > CEPH_PAGE_SIZE) { + traits::decode(o, p); + } else { + // ensure we get a contigous buffer... until the end of the + // bufferlist. we don't really know how much we'll need here, + // unfortunately. hopefully it is already contiguous and we're just + // bumping the raw ref and initializing the ptr tmp fields. + bufferptr tmp; + bufferlist::iterator t = p; + t.copy_shallow(remaining, tmp); + auto cp = tmp.begin(); + traits::decode(o, cp); + p.advance((ssize_t)cp.get_offset()); + } } -template> +template> inline typename std::enable_if::type decode( + !traits::with_dencv>::type decode( T& o, bufferlist::iterator& p) { if (p.end()) throw buffer::end_of_buffer(); + // ensure we get a contigous buffer... until the end of the + // bufferlist. we don't really know how much we'll need here, + // unfortunately. hopefully it is already contiguous and we're just + // bumping the raw ref and initializing the ptr tmp fields. bufferptr tmp; bufferlist::iterator t = p; t.copy_shallow(p.get_bl().length() - p.get_off(), tmp); diff --git a/src/include/fs_types.h b/src/include/fs_types.h index 5513fcefd79222..e84205ac58628a 100644 --- a/src/include/fs_types.h +++ b/src/include/fs_types.h @@ -32,6 +32,7 @@ struct denc_traits { static constexpr bool supported = true; static constexpr bool featured = false; static constexpr bool bounded = true; + static constexpr bool with_dencv = false; static void bound_encode(const inodeno_t &o, size_t& p) { denc(o.val, p); } diff --git a/src/include/interval_set.h b/src/include/interval_set.h index de7cf930894ba6..f792f9b81e219a 100644 --- a/src/include/interval_set.h +++ b/src/include/interval_set.h @@ -259,6 +259,12 @@ class interval_set { i++) _size += i->second; } + void decode(bufferlist::iterator& p) { + denc_traits>::decode(m, p); + _size = 0; + for (const auto& i : m) + _size += i.second; + } void encode_nohead(bufferlist::contiguous_appender& p) const { denc_traits>::encode_nohead(m, p); @@ -576,6 +582,7 @@ struct denc_traits> { static constexpr bool supported = true; static constexpr bool bounded = false; static constexpr bool featured = false; + static constexpr bool with_dencv = denc_traits::with_dencv; static void bound_encode(const interval_set& v, size_t& p) { v.bound_encode(p); } @@ -586,6 +593,11 @@ struct denc_traits> { static void decode(interval_set& v, bufferptr::iterator& p) { v.decode(p); } + template + static typename std::enable_if::type + decode(interval_set& v, bufferlist::iterator& p) { + v.decode(p); + } static void encode_nohead(const interval_set& v, bufferlist::contiguous_appender& p) { v.encode_nohead(p); diff --git a/src/include/object.h b/src/include/object.h index 672fbc4c3f91c3..efb4d42472d946 100644 --- a/src/include/object.h +++ b/src/include/object.h @@ -127,6 +127,7 @@ struct denc_traits { static constexpr bool supported = true; static constexpr bool featured = false; static constexpr bool bounded = true; + static constexpr bool with_dencv = false; static void bound_encode(const snapid_t& o, size_t& p) { denc(o.val, p); } diff --git a/src/os/bluestore/bluestore_types.h b/src/os/bluestore/bluestore_types.h index f60b53baf1dd76..33dde78ce41a19 100644 --- a/src/os/bluestore/bluestore_types.h +++ b/src/os/bluestore/bluestore_types.h @@ -168,6 +168,7 @@ struct denc_traits { static constexpr bool supported = true; static constexpr bool bounded = false; static constexpr bool featured = false; + static constexpr bool with_dencv = false; static void bound_encode(const PExtentVector& v, size_t& p) { p += sizeof(uint32_t); const auto size = v.size(); diff --git a/src/test/encoding.cc b/src/test/encoding.cc index af3346e1a0916d..0b7e3a5a364f86 100644 --- a/src/test/encoding.cc +++ b/src/test/encoding.cc @@ -1,6 +1,7 @@ #include "include/buffer.h" #include "include/encoding.h" +#include "include/types.h" #include "gtest/gtest.h" using namespace std; @@ -288,6 +289,109 @@ TEST(EncodingRoundTrip, Integers) { i = 42; test_encode_and_decode(i); } + // vector + { + vector v = {0, 1, 1, 2, 3, 5}; + test_encode_and_decode(v); + } +} + + +struct Legacy { + static unsigned n_denc; + static unsigned n_decode; + uint8_t value = 0; + DENC(Legacy, v, p) { + n_denc++; + denc(v.value, p); + } + void decode(buffer::list::iterator& p) { + n_decode++; + ::decode(value, p); + } + static void reset() { + n_denc = n_decode = 0; + } + static bufferlist encode_n(unsigned n, unsigned segments); +}; +WRITE_CLASS_DENC(Legacy) +unsigned Legacy::n_denc = 0; +unsigned Legacy::n_decode = 0; + +bufferlist Legacy::encode_n(unsigned n, unsigned segments) { + vector v; + for (unsigned i = 0; i < n; i++) { + v.push_back(Legacy()); + } + bufferlist bl(n * sizeof(uint8_t)); + ::encode(v, bl); + bufferlist segmented; + auto p = bl.begin(); + for (unsigned i = 0; i < segments; i++) { + buffer::ptr seg; + p.copy_deep(bl.length() / segments, seg); + segmented.push_back(seg); + } + if (bl.length() % segments) { + buffer::ptr seg; + p.copy_deep(bl.length() % segments, seg); + segmented.push_back(seg); + } + return segmented; +} + +TEST(EncodingRoundTrip, NoCopyIfSegmentedAndLengthy) +{ + static_assert(_denc::has_legacy_denc::value, + "Legacy do have legacy denc"); + { + // use denc() which shallow_copy() if the buffer is small + constexpr unsigned N_COPIES = 42; + constexpr unsigned N_SEGS = 2; + bufferlist segmented = Legacy::encode_n(N_COPIES, N_SEGS); + ASSERT_GT(segmented.get_num_buffers(), 1u); + ASSERT_LT(segmented.length(), CEPH_PAGE_SIZE); + auto i(segmented.begin()); + vector v; + // denc() is shared by encode() and decode(), so reset() right before + // decode() + Legacy::reset(); + ::decode(v, i); + ASSERT_EQ(N_COPIES, v.size()); + ASSERT_EQ(N_COPIES, Legacy::n_denc); + ASSERT_EQ(0u, Legacy::n_decode); + } + { + // use decode() which avoids deep copy if the buffer is segmented and large + const unsigned N_COPIES = CEPH_PAGE_SIZE * 2; + const unsigned N_SEGS = 2; + bufferlist segmented = Legacy::encode_n(N_COPIES, N_SEGS); + ASSERT_GT(segmented.get_num_buffers(), 1u); + ASSERT_GT(segmented.length(), CEPH_PAGE_SIZE); + auto i(segmented.begin()); + vector v; + Legacy::reset(); + ::decode(v, i); + ASSERT_EQ(N_COPIES, v.size()); + ASSERT_EQ(0u, Legacy::n_denc); + ASSERT_EQ(N_COPIES, Legacy::n_decode); + } + { + // use denc() which shallow_copy() if the buffer is not segmented and large + const unsigned N_COPIES = CEPH_PAGE_SIZE * 2; + const unsigned N_SEGS = 1; + Legacy::reset(); + bufferlist segmented = Legacy::encode_n(N_COPIES, N_SEGS); + ASSERT_EQ(segmented.get_num_buffers(), 1u); + ASSERT_GT(segmented.length(), CEPH_PAGE_SIZE); + auto i(segmented.begin()); + vector v; + Legacy::reset(); + ::decode(v, i); + ASSERT_EQ(N_COPIES, v.size()); + ASSERT_EQ(N_COPIES, Legacy::n_denc); + ASSERT_EQ(0u, Legacy::n_decode); + } } const char* expected_what[] = {