diff --git a/CHANGELOG.md b/CHANGELOG.md index 30f6c1a..d4c6db8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +# [5.0.3](https://github.com/fraillt/bitsery/compare/v5.0.2...v5.0.3) (2020-01-29) + +### Improvements +* rewritten buffer adapters (and `BasicBufferedOutputStreamAdapter`) to fix UB when incrementing past the end iterator, and added an additional read/write method that accepts a number of bytes to be read/written at compile time. +This provides additional optimization opportunities. + # [5.0.2](https://github.com/fraillt/bitsery/compare/v5.0.1...v5.0.2) (2020-01-17) ### Bug fixes diff --git a/CMakeLists.txt b/CMakeLists.txt index c0c6f8e..441961d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,7 +1,7 @@ cmake_minimum_required(VERSION 3.1) project(bitsery LANGUAGES CXX - VERSION 5.0.1) + VERSION 5.0.3) #======== build options =================================== option(BITSERY_BUILD_EXAMPLES "Build examples" OFF) diff --git a/README.md b/README.md index 81aaa7d..3db83f3 100644 --- a/README.md +++ b/README.md @@ -18,28 +18,27 @@ All cross-platform requirements are enforced at compile time, so serialized data * Configurable runtime error checking on deserialization. * Can read/write from any source: stream (file, network stream. etc... ), or buffer (vector, c-array, etc...). * Don't pay for what you don't use! - customize your serialization via **extensions**. Some notable *extensions* allow: - * forward/backward compatibility for your types. - * smart and raw pointers with customizable runtime polymorphism support. * fine-grained bit-level serialization control. + * forward/backward compatibility for your types. + * smart and raw pointers with allocators support and customizable runtime polymorphism. * Easily extendable for any type. -* Allows brief or/and verbose syntax for better serialization control. +* Allows brief (similar to [cereal](https://uscilab.github.io/cereal/)) or/and verbose syntax for better serialization control. * Configurable endianness support. * No macros. -## Why to use bitsery +## Why use bitsery Look at the numbers and features list, and decide yourself. -| | data size | serialize | deserialize | -|------------------|-----------|-----------|-------------| -| bitsery | 6913B | 1252ms | 1170ms | -| bitsery_compress | 4213B | 1445ms | 1325ms | -| boost | 11037B | 9952ms | 8767ms | -| cereal | 10413B | 6497ms | 5470ms | -| flatbuffers | 14924B | 6762ms | 2173ms | -| yas | 10463B | 1352ms | 1109ms | -| yas_compress | 7315B | 1673ms | 1598ms | - +| library | data size | serialize | deserialize | +| ---------------- | --------- | --------- | ----------- | +| bitsery | 6913B | 959ms | 927ms | +| bitsery_compress | 4213B | 1282ms | 1115ms | +| boost | 11037B | 9826ms | 8313ms | +| cereal | 10413B | 6324ms | 5698ms | +| flatbuffers | 14924B | 5129ms | 2142ms | +| protobuf | 10018B | 11966ms | 13919ms | +| yas | 10463B | 1908ms | 1217ms | *benchmarked on Ubuntu with GCC 8.3.0, more details can be found [here](https://github.com/fraillt/cpp_serializers_benchmark.git)* @@ -103,9 +102,12 @@ Works with C++11 compiler, no additional dependencies, include ` + void readInternalValue(TValue *data) { + readInternalValueChecked(data, std::integral_constant{}); + } + + void readInternalBuffer(TValue *data, size_t size) { + readInternalBufferChecked(data, size, std::integral_constant{}); + } + + template + void readInternalValueChecked(TValue *data, std::false_type) { + const auto newOffset = _currOffset + SIZE; + assert(newOffset <= _endReadOffset); + std::copy_n(_beginIt + _currOffset, SIZE, data); + _currOffset = newOffset; + } + + template + void readInternalValueChecked(TValue *data, std::true_type) { + const auto newOffset = _currOffset + SIZE; + if (newOffset <= _endReadOffset) { + std::copy_n(_beginIt + _currOffset, SIZE, data); + _currOffset = newOffset; + } else { + //set everything to zeros + std::memset(data, 0, SIZE); + if (_overflowOnReadEndPos) + error(ReaderError::DataOverflow); + } + } + + void readInternalBufferChecked(TValue *data, size_t size, std::false_type) { const auto newOffset = _currOffset + size; assert(newOffset <= _endReadOffset); std::copy_n(_beginIt + _currOffset, size, data); _currOffset = newOffset; } - void readChecked(TValue *data, size_t size, std::true_type) { + void readInternalBufferChecked(TValue *data, size_t size, std::true_type) { const auto newOffset = _currOffset + size; if (newOffset <= _endReadOffset) { std::copy_n(_beginIt + _currOffset, size, data); @@ -124,10 +155,6 @@ namespace bitsery { } } - void readInternal(TValue *data, size_t size) { - readChecked(data, size, std::integral_constant{}); - } - void currentReadPosChecked(size_t pos, std::true_type) { if (_bufferSize >= pos && error() == ReaderError::NoError) { _currOffset = pos; @@ -203,8 +230,13 @@ namespace bitsery { private: using TResizable = std::integral_constant::isResizable>; - void writeInternal(const TValue *data, size_t size) { - writeInternalImpl(data, size, TResizable{}); + template + void writeInternalValue(const TValue *data) { + writeInternalValueImpl(data, TResizable{}); + } + + void writeInternalBuffer(const TValue *data, size_t size) { + writeInternalBufferImpl(data, size, TResizable{}); } Buffer* _buffer; @@ -225,7 +257,20 @@ namespace bitsery { updateIteratorAndSize(); } - void writeInternalImpl(const TValue *data, const size_t size, std::true_type) { + template + void writeInternalValueImpl(const TValue *data, std::true_type) { + const auto newOffset = _currOffset + SIZE; + if (newOffset <= _bufferSize) { + std::copy_n(data, SIZE, _beginIt + _currOffset); + _currOffset = newOffset; + } else { + traits::BufferAdapterTraits::increaseBufferSize(*_buffer); + updateIteratorAndSize(); + writeInternalValueImpl(data, std::true_type{}); + } + } + + void writeInternalBufferImpl(const TValue *data, const size_t size, std::true_type) { const auto newOffset = _currOffset + size; if (newOffset <= _bufferSize) { std::copy_n(data, size, _beginIt + _currOffset); @@ -233,7 +278,7 @@ namespace bitsery { } else { traits::BufferAdapterTraits::increaseBufferSize(*_buffer); updateIteratorAndSize(); - writeInternalImpl(data, size, std::true_type{}); + writeInternalBufferImpl(data, size, std::true_type{}); } } @@ -254,7 +299,15 @@ namespace bitsery { updateIteratorAndSize(); } - void writeInternalImpl(const TValue *data, size_t size, std::false_type) { + template + void writeInternalValueImpl(const TValue *data, std::false_type) { + const auto newOffset = _currOffset + SIZE; + assert(newOffset <= _bufferSize); + std::copy_n(data, SIZE, _beginIt + _currOffset); + _currOffset = newOffset; + } + + void writeInternalBufferImpl(const TValue *data, size_t size, std::false_type) { const auto newOffset = _currOffset + size; assert(newOffset <= _bufferSize); std::copy_n(data, size, _beginIt + _currOffset); diff --git a/include/bitsery/adapter/stream.h b/include/bitsery/adapter/stream.h index 27ad994..a397678 100644 --- a/include/bitsery/adapter/stream.h +++ b/include/bitsery/adapter/stream.h @@ -83,7 +83,12 @@ namespace bitsery { private: - void readInternal(TValue* data, size_t size) { + template + void readInternalValue(TValue* data) { + readChecked(data, SIZE, std::integral_constant{}); + } + + void readInternalBuffer(TValue* data, size_t size) { readChecked(data, size, std::integral_constant{}); } @@ -139,8 +144,12 @@ namespace bitsery { private: - void writeInternal(const TValue* data, size_t size) { - //for optimization + template + void writeInternalValue(const TValue* data) { + _ios->rdbuf()->sputn( data , SIZE ); + } + + void writeInternalBuffer(const TValue* data, size_t size) { _ios->rdbuf()->sputn( data , size ); } @@ -163,9 +172,12 @@ namespace bitsery { BasicBufferedOutputStreamAdapter(std::basic_ios& ostream, size_t bufferSize = 256) :_ios(std::addressof(ostream)), _buf{}, - _outIt{} + _beginIt{std::begin(_buf)}, + _currOffset{0} { init(bufferSize, TResizable{}); + // buffer size must be atleast 16, because writeIntervalValue expect that atleast one value fits to buffer. + assert(_bufferSize >= 16); } //we need to explicitly declare move logic, because after move buffer might be invalidated @@ -174,20 +186,19 @@ namespace bitsery { BasicBufferedOutputStreamAdapter(BasicBufferedOutputStreamAdapter&& rhs) : _ios{rhs._ios}, - _buf{}, - _outIt{} + _buf{std::move(rhs._buf)}, + _beginIt{std::begin(_buf)}, + _currOffset{rhs._currOffset}, + _bufferSize{rhs._bufferSize} { - auto size = std::distance(std::begin(rhs._buf), rhs._outIt); - _buf = std::move(rhs._buf); - _outIt = std::next(std::begin(_buf), size); }; BasicBufferedOutputStreamAdapter& operator = (BasicBufferedOutputStreamAdapter&& rhs) { _ios = rhs._ios; - //get current written size, before move - auto size = std::distance(std::begin(rhs._buf), rhs._outIt); _buf = std::move(rhs._buf); - _outIt = std::next(std::begin(_buf), size); + _beginIt = std::begin(_buf); + _currOffset = rhs._currOffset; + _bufferSize = rhs._bufferSize; return *this; }; @@ -201,9 +212,7 @@ namespace bitsery { } void flush() { - auto begin = std::begin(_buf); - writeToStream(std::addressof(*begin), static_cast(std::distance(begin, _outIt))); - _outIt = begin; + writeBufferToStream(); if (auto ostream = dynamic_cast*>(_ios)) ostream->flush(); } @@ -217,44 +226,50 @@ namespace bitsery { private: using TResizable = std::integral_constant::isResizable>; - void writeInternal(const TValue* data, size_t size) { - auto tmp = _outIt; + template + void writeInternalValue(const TValue* data) { + auto newOffset = _currOffset + SIZE; + if (newOffset > _bufferSize) { + writeBufferToStream(); + newOffset = SIZE; + } + std::copy_n(data, SIZE, _beginIt + _currOffset); + _currOffset = newOffset; + } -#if defined(_MSC_VER) && (_ITERATOR_DEBUG_LEVEL > 0) - using TDistance = typename std::iterator_traits::difference_type; - if (std::distance(_outIt , std::end(_buf)) >= static_cast(size)) { - std::memcpy(std::addressof(*_outIt), data, size); - _outIt += size; - } -#else - _outIt += size; - if (std::distance(_outIt , std::end(_buf)) >= 0) { - std::memcpy(std::addressof(*tmp), data, size); - } -#endif - else { - //when buffer is full write out to stream - _outIt = std::begin(_buf); - writeToStream(std::addressof(*_outIt), static_cast(std::distance(_outIt, tmp))); - writeToStream(data, size); + void writeInternalBuffer(const TValue* data, size_t size) { + const auto newOffset = _currOffset + size; + if (newOffset <= _bufferSize) { + std::copy_n(data, size, _beginIt + _currOffset); + _currOffset = newOffset; + } else { + writeBufferToStream(); + // write buffer directly to stream + _ios->rdbuf()->sputn(data, size); } } - void writeToStream(const TValue* data, size_t size) { - _ios->rdbuf()->sputn( data , size ); + void writeBufferToStream() { + _ios->rdbuf()->sputn(std::addressof(*_beginIt), _currOffset); + _currOffset = 0; } - void init (size_t bufferSize, std::true_type) { - _buf.resize(bufferSize); - _outIt = std::begin(_buf); + void init (size_t buffSize, std::true_type) { + // resize buffer + _bufferSize = buffSize; + _buf.resize(_bufferSize); + _beginIt = std::begin(_buf); } - void init (size_t, std::false_type) { - _outIt = std::begin(_buf); + void init (size_t , std::false_type) { + // ignore buffer size parameter, and instead take actual buffer size + _bufferSize = traits::ContainerTraits::size(_buf); } std::basic_ios* _ios; TBuffer _buf; - BufferIt _outIt; + BufferIt _beginIt; + size_t _currOffset; + size_t _bufferSize{0}; }; template diff --git a/include/bitsery/bitsery.h b/include/bitsery/bitsery.h index c97361d..386a814 100644 --- a/include/bitsery/bitsery.h +++ b/include/bitsery/bitsery.h @@ -26,7 +26,7 @@ #define BITSERY_MAJOR_VERSION 5 #define BITSERY_MINOR_VERSION 0 -#define BITSERY_PATCH_VERSION 1 +#define BITSERY_PATCH_VERSION 3 #define BITSERY_QUOTE_MACRO(name) #name #define BITSERY_BUILD_VERSION_STR(major,minor, patch) \ diff --git a/include/bitsery/details/adapter_common.h b/include/bitsery/details/adapter_common.h index 1422262..2297840 100644 --- a/include/bitsery/details/adapter_common.h +++ b/include/bitsery/details/adapter_common.h @@ -192,15 +192,14 @@ namespace bitsery { void writeBytes(const T &v) { static_assert(std::is_integral(), ""); static_assert(sizeof(T) == SIZE, ""); - writeSwapped(&v, 1, ShouldSwap{}); - + writeSwappedValue(&v, ShouldSwap{}); } template void writeBuffer(const T *buf, size_t count) { static_assert(std::is_integral(), ""); static_assert(sizeof(T) == SIZE, ""); - writeSwapped(buf, count, ShouldSwap{}); + writeSwappedBuffer(buf, count, ShouldSwap{}); } template @@ -222,16 +221,27 @@ namespace bitsery { private: template - void writeSwapped(const T *v, size_t count, std::true_type) { + void writeSwappedValue(const T *v, std::true_type) { + const auto res = details::swap(*v); + static_cast(this)->template writeInternalValue(reinterpret_cast(&res)); + } + + template + void writeSwappedValue(const T *v, std::false_type) { + static_cast(this)->template writeInternalValue(reinterpret_cast(v)); + } + + template + void writeSwappedBuffer(const T *v, size_t count, std::true_type) { std::for_each(v, std::next(v, count), [this](const T &v) { const auto res = details::swap(v); - static_cast(this)->writeInternal(reinterpret_cast(&res), sizeof(T)); + static_cast(this)->template writeInternalValue(reinterpret_cast(&res)); }); } template - void writeSwapped(const T *v, size_t count, std::false_type) { - static_cast(this)->writeInternal(reinterpret_cast(v), count * sizeof(T)); + void writeSwappedBuffer(const T *v, size_t count, std::false_type) { + static_cast(this)->writeInternalBuffer(reinterpret_cast(v), count * sizeof(T)); } }; @@ -245,14 +255,16 @@ namespace bitsery { void readBytes(T& v) { static_assert(std::is_integral(), ""); static_assert(sizeof(T) == SIZE, ""); - directRead(&v, 1); + static_cast(this)->template readInternalValue(reinterpret_cast(&v)); + swapDataBits(v, ShouldSwap{}); } template void readBuffer(T* buf, size_t count) { static_assert(std::is_integral(), ""); static_assert(sizeof(T) == SIZE, ""); - directRead(buf, count); + static_cast(this)->readInternalBuffer(reinterpret_cast(buf), sizeof(T) * count); + swapDataBits(buf, count, ShouldSwap{}); } template @@ -277,23 +289,26 @@ namespace bitsery { private: template - void directRead(T *v, size_t count) { - static_assert(!std::is_const::value, ""); - static_cast(this)->readInternal(reinterpret_cast(v), sizeof(T) * count); - //swap each byte if necessary - _swapDataBits(v, count, ShouldSwap{}); - } - - template - void _swapDataBits(T *v, size_t count, std::true_type) { + void swapDataBits(T *v, size_t count, std::true_type) { std::for_each(v, std::next(v, count), [](T &x) { x = details::swap(x); }); } template - void _swapDataBits(T *, size_t , std::false_type) { + void swapDataBits(T *, size_t , std::false_type) { //empty function because no swap is required } + template + void swapDataBits(T &v, std::true_type) { + v = details::swap(v); + } + + template + void swapDataBits(T &, std::false_type) { + //empty function because no swap is required + } + + }; } diff --git a/tests/adapter.cpp b/tests/adapter.cpp index 37f9210..101dad7 100644 --- a/tests/adapter.cpp +++ b/tests/adapter.cpp @@ -502,13 +502,13 @@ using BufferedAdapterInternalBufferTypes = ::testing::Types< TYPED_TEST_CASE(OutputStreamBuffered, BufferedAdapterInternalBufferTypes); -TYPED_TEST(OutputStreamBuffered, WhenInternalBufferIsFullThenWriteBufferAndRemainingDataToStream) { +TYPED_TEST(OutputStreamBuffered, WhenInternalBufferIsFullThenWriteBufferToStream) { uint8_t x{}; for (auto i = 0u; i < TestFixture::InternalBufferSize; ++i) this->writer.template writeBytes<1>(x); EXPECT_TRUE(this->stream.str().empty()); this->writer.template writeBytes<1>(x); - EXPECT_THAT(this->stream.str().size(), Eq(TestFixture::InternalBufferSize + 1)); + EXPECT_THAT(this->stream.str().size(), Eq(TestFixture::InternalBufferSize)); } TYPED_TEST(OutputStreamBuffered, WhenFlushThenWriteImmediately) {