From c34f3254f24536885dc911951b7f73699138a18e Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Wed, 8 Jul 2020 04:21:39 +0300 Subject: [PATCH 01/17] Improvements in PODArray --- src/Common/PODArray.h | 59 +++++++++++++++++++++++----- src/Common/tests/gtest_pod_array.cpp | 28 +++++++++++++ 2 files changed, 78 insertions(+), 9 deletions(-) diff --git a/src/Common/PODArray.h b/src/Common/PODArray.h index 839990ca9b4..28f3de53950 100644 --- a/src/Common/PODArray.h +++ b/src/Common/PODArray.h @@ -102,7 +102,7 @@ protected: void alloc_for_num_elements(size_t num_elements) { - alloc(roundUpToPowerOfTwoOrZero(minimum_memory_for_elements(num_elements))); + alloc(minimum_memory_for_elements(num_elements)); } template @@ -211,6 +211,13 @@ public: realloc(roundUpToPowerOfTwoOrZero(minimum_memory_for_elements(n)), std::forward(allocator_params)...); } + template + void reserve_exact(size_t n, TAllocatorParams &&... allocator_params) + { + if (n > capacity()) + realloc(minimum_memory_for_elements(n), std::forward(allocator_params)...); + } + template void resize(size_t n, TAllocatorParams &&... allocator_params) { @@ -218,6 +225,13 @@ public: resize_assume_reserved(n); } + template + void resize_exact(size_t n, TAllocatorParams &&... allocator_params) + { + reserve_exact(n, std::forward(allocator_params)...); + resize_assume_reserved(n); + } + void resize_assume_reserved(const size_t n) { c_end = c_start + byte_size(n); @@ -428,6 +442,7 @@ public: void insertSmallAllowReadWriteOverflow15(It1 from_begin, It2 from_end, TAllocatorParams &&... allocator_params) { static_assert(pad_right_ >= 15); + static_assert(sizeof(T) == sizeof(*from_begin)); insertPrepare(from_begin, from_end, std::forward(allocator_params)...); size_t bytes_to_copy = this->byte_size(from_end - from_begin); memcpySmallAllowReadWriteOverflow15(this->c_end, reinterpret_cast(&*from_begin), bytes_to_copy); @@ -438,23 +453,40 @@ public: void insert(iterator it, It1 from_begin, It2 from_end) { size_t bytes_to_copy = this->byte_size(from_end - from_begin); - size_t bytes_to_move = (end() - it) * sizeof(T); + size_t bytes_to_move = this->byte_size(end() - it); insertPrepare(from_begin, from_end); if (unlikely(bytes_to_move)) memcpy(this->c_end + bytes_to_copy - bytes_to_move, this->c_end - bytes_to_move, bytes_to_move); - memcpy(this->c_end - bytes_to_move, reinterpret_cast(&*from_begin), bytes_to_copy); + if constexpr (sizeof(*from_begin) == sizeof(T)) + { + memcpy(this->c_end - bytes_to_move, reinterpret_cast(&*from_begin), bytes_to_copy); + } + else + { + for (auto from_it = from_begin; from_it != from_end; ++from_it, ++it) + *it = *from_it; + } + this->c_end += bytes_to_copy; } template void insert_assume_reserved(It1 from_begin, It2 from_end) { - size_t bytes_to_copy = this->byte_size(from_end - from_begin); - memcpy(this->c_end, reinterpret_cast(&*from_begin), bytes_to_copy); - this->c_end += bytes_to_copy; + if constexpr (sizeof(*from_begin) == sizeof(T)) + { + size_t bytes_to_copy = this->byte_size(from_end - from_begin); + memcpy(this->c_end, reinterpret_cast(&*from_begin), bytes_to_copy); + this->c_end += bytes_to_copy; + } + else + { + for (auto it = from_begin; it != from_end; ++it) + push_back(*it); + } } template @@ -577,7 +609,7 @@ public: template void assign(size_t n, const T & x, TAllocatorParams &&... allocator_params) { - this->resize(n, std::forward(allocator_params)...); + this->resize_exact(n, std::forward(allocator_params)...); std::fill(begin(), end(), x); } @@ -586,10 +618,19 @@ public: { size_t required_capacity = from_end - from_begin; if (required_capacity > this->capacity()) - this->reserve(roundUpToPowerOfTwoOrZero(required_capacity), std::forward(allocator_params)...); + this->reserve_exact(required_capacity, std::forward(allocator_params)...); size_t bytes_to_copy = this->byte_size(required_capacity); - memcpy(this->c_start, reinterpret_cast(&*from_begin), bytes_to_copy); + if constexpr (sizeof(*from_begin) == sizeof(T)) + { + memcpy(this->c_start, reinterpret_cast(&*from_begin), bytes_to_copy); + } + else + { + auto it = begin(); + for (auto from_it = from_begin; from_it != from_end; ++from_it) + *it = *from_it; + } this->c_end = this->c_start + bytes_to_copy; } diff --git a/src/Common/tests/gtest_pod_array.cpp b/src/Common/tests/gtest_pod_array.cpp index b77bc56b59a..8c14811ddd2 100644 --- a/src/Common/tests/gtest_pod_array.cpp +++ b/src/Common/tests/gtest_pod_array.cpp @@ -33,6 +33,34 @@ TEST(Common, PODArrayInsert) EXPECT_EQ(str, std::string(chars.data(), chars.size())); } +TEST(Common, PODInsertIteratorRange) +{ + size_t size = 1 << 20; + char value = 123; + + PODArray big; + PODArray small(size, value); + + EXPECT_EQ(big.size(), 0); + EXPECT_EQ(small.size(), size); + + big.insert(small.begin(), small.end()); + + EXPECT_EQ(big.size(), size); + EXPECT_EQ(big.back(), value); + + big.assign(small.begin(), small.end()); + + EXPECT_EQ(big.size(), size); + EXPECT_EQ(big.back(), value); + + big.insert(big.begin(), small.begin(), small.end()); + + EXPECT_EQ(big.size(), size * 2); + EXPECT_EQ(big.front(), value); + EXPECT_EQ(big.back(), value); +} + TEST(Common, PODPushBackRawMany) { PODArray chars; From 671c544b7d72d4357e68d8f92b042294ca6a71b7 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Wed, 8 Jul 2020 04:24:38 +0300 Subject: [PATCH 02/17] Improvements in PODArray --- src/Common/PODArray.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Common/PODArray.h b/src/Common/PODArray.h index 28f3de53950..e8289c6c07d 100644 --- a/src/Common/PODArray.h +++ b/src/Common/PODArray.h @@ -460,7 +460,7 @@ public: if (unlikely(bytes_to_move)) memcpy(this->c_end + bytes_to_copy - bytes_to_move, this->c_end - bytes_to_move, bytes_to_move); - if constexpr (sizeof(*from_begin) == sizeof(T)) + if constexpr (std::is_same_v) { memcpy(this->c_end - bytes_to_move, reinterpret_cast(&*from_begin), bytes_to_copy); } @@ -476,7 +476,7 @@ public: template void insert_assume_reserved(It1 from_begin, It2 from_end) { - if constexpr (sizeof(*from_begin) == sizeof(T)) + if constexpr (std::is_same_v) { size_t bytes_to_copy = this->byte_size(from_end - from_begin); memcpy(this->c_end, reinterpret_cast(&*from_begin), bytes_to_copy); @@ -621,7 +621,7 @@ public: this->reserve_exact(required_capacity, std::forward(allocator_params)...); size_t bytes_to_copy = this->byte_size(required_capacity); - if constexpr (sizeof(*from_begin) == sizeof(T)) + if constexpr (std::is_same_v) { memcpy(this->c_start, reinterpret_cast(&*from_begin), bytes_to_copy); } From 3471e526ffe583312ea55727a7fbd1666081ef2d Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Wed, 8 Jul 2020 04:26:44 +0300 Subject: [PATCH 03/17] Improvements in PODArray --- src/Common/PODArray.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Common/PODArray.h b/src/Common/PODArray.h index e8289c6c07d..1f97759f314 100644 --- a/src/Common/PODArray.h +++ b/src/Common/PODArray.h @@ -460,7 +460,7 @@ public: if (unlikely(bytes_to_move)) memcpy(this->c_end + bytes_to_copy - bytes_to_move, this->c_end - bytes_to_move, bytes_to_move); - if constexpr (std::is_same_v) + if constexpr (std::is_same_v>) { memcpy(this->c_end - bytes_to_move, reinterpret_cast(&*from_begin), bytes_to_copy); } @@ -476,7 +476,7 @@ public: template void insert_assume_reserved(It1 from_begin, It2 from_end) { - if constexpr (std::is_same_v) + if constexpr (std::is_same_v>) { size_t bytes_to_copy = this->byte_size(from_end - from_begin); memcpy(this->c_end, reinterpret_cast(&*from_begin), bytes_to_copy); @@ -621,7 +621,7 @@ public: this->reserve_exact(required_capacity, std::forward(allocator_params)...); size_t bytes_to_copy = this->byte_size(required_capacity); - if constexpr (std::is_same_v) + if constexpr (std::is_same_v>) { memcpy(this->c_start, reinterpret_cast(&*from_begin), bytes_to_copy); } From 491bd449555195608763e03779dbbdf899b938a2 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Wed, 8 Jul 2020 04:41:09 +0300 Subject: [PATCH 04/17] Improve performance of clang build --- src/Common/PODArray.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Common/PODArray.h b/src/Common/PODArray.h index 1f97759f314..3e17205b44d 100644 --- a/src/Common/PODArray.h +++ b/src/Common/PODArray.h @@ -204,8 +204,12 @@ public: void clear() { c_end = c_start; } + /// Always inline is for clang. GCC works fine. + /// It makes sense when "resize" called in a loop, so we want "reserve" to be inlined into "resize". + /// It improves performance of SQL queries with "materialize" of String by 23%. + /// And we don't need always inline for "reserve_exact" because calling it in a loop is already terribly slow. template - void reserve(size_t n, TAllocatorParams &&... allocator_params) + ALWAYS_INLINE void reserve(size_t n, TAllocatorParams &&... allocator_params) { if (n > capacity()) realloc(roundUpToPowerOfTwoOrZero(minimum_memory_for_elements(n)), std::forward(allocator_params)...); From a6dbe421ac494c17cac6258d5db636d72119be6b Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Wed, 8 Jul 2020 05:31:29 +0300 Subject: [PATCH 05/17] Fix build --- src/Common/PODArray.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Common/PODArray.h b/src/Common/PODArray.h index 3e17205b44d..3b5a1ccd2b2 100644 --- a/src/Common/PODArray.h +++ b/src/Common/PODArray.h @@ -471,7 +471,7 @@ public: else { for (auto from_it = from_begin; from_it != from_end; ++from_it, ++it) - *it = *from_it; + new (&*it) T{*from_it}; } this->c_end += bytes_to_copy; @@ -633,7 +633,7 @@ public: { auto it = begin(); for (auto from_it = from_begin; from_it != from_end; ++from_it) - *it = *from_it; + new (&*it) T{*from_it}; } this->c_end = this->c_start + bytes_to_copy; } From c4e925434a90bc6fbb0f48b0cc9b95219030999b Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Wed, 8 Jul 2020 06:29:54 +0300 Subject: [PATCH 06/17] Fix build --- src/Common/PODArray.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Common/PODArray.h b/src/Common/PODArray.h index 3b5a1ccd2b2..cc2e80e1e46 100644 --- a/src/Common/PODArray.h +++ b/src/Common/PODArray.h @@ -471,7 +471,7 @@ public: else { for (auto from_it = from_begin; from_it != from_end; ++from_it, ++it) - new (&*it) T{*from_it}; + new (&*it) T(*from_it); } this->c_end += bytes_to_copy; @@ -633,7 +633,7 @@ public: { auto it = begin(); for (auto from_it = from_begin; from_it != from_end; ++from_it) - new (&*it) T{*from_it}; + new (&*it) T(*from_it); } this->c_end = this->c_start + bytes_to_copy; } From d6c87af267521ec6301848537b13226993cd85c2 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Thu, 9 Jul 2020 02:04:42 +0300 Subject: [PATCH 07/17] Fix strange code CC @Enmk. Prove: g++ -xc++ -include vector - <<< 'int main() { return std::vector{1, 2, 3}.size(); }' --- src/Storages/tests/gtest_SplitTokenExtractor.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Storages/tests/gtest_SplitTokenExtractor.cpp b/src/Storages/tests/gtest_SplitTokenExtractor.cpp index 9255e5ca817..cb801cff808 100644 --- a/src/Storages/tests/gtest_SplitTokenExtractor.cpp +++ b/src/Storages/tests/gtest_SplitTokenExtractor.cpp @@ -35,7 +35,7 @@ public: { const auto & param = GetParam(); const auto & source = param.source; - data = std::make_unique>(source.data(), source.data() + source.size()); + data = std::make_unique>(source.data(), source.data() + source.size()); // add predefined padding that forms tokens to ensure no reads past end of buffer. const char extra_padding[] = "this is the end \xd1\x8d\xd1\x82\xd0\xbe\xd0\xba\xd0\xbe \xd0\xbd\xd0\xb5\xd1\x86"; @@ -44,7 +44,7 @@ public: data->resize(data->size() - sizeof(extra_padding)); } - std::unique_ptr> data; + std::unique_ptr> data; }; TEST_P(SplitTokenExtractorTest, next) From 70aae6eb447ae5ab7caf4191adae7046a1776197 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Thu, 9 Jul 2020 04:21:25 +0300 Subject: [PATCH 08/17] Fix error --- src/Common/PODArray.h | 3 +++ src/Common/tests/gtest_pod_array.cpp | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/src/Common/PODArray.h b/src/Common/PODArray.h index cc2e80e1e46..af0b6280a9b 100644 --- a/src/Common/PODArray.h +++ b/src/Common/PODArray.h @@ -453,9 +453,11 @@ public: this->c_end += bytes_to_copy; } + /// Do not insert into the array a piece of itself. Because with the resize, the iterators on themselves can be invalidated. template void insert(iterator it, It1 from_begin, It2 from_end) { + size_t position = it - begin(); size_t bytes_to_copy = this->byte_size(from_end - from_begin); size_t bytes_to_move = this->byte_size(end() - it); @@ -470,6 +472,7 @@ public: } else { + it = begin() + position; for (auto from_it = from_begin; from_it != from_end; ++from_it, ++it) new (&*it) T(*from_it); } diff --git a/src/Common/tests/gtest_pod_array.cpp b/src/Common/tests/gtest_pod_array.cpp index 305d09b1047..3bd2307452b 100644 --- a/src/Common/tests/gtest_pod_array.cpp +++ b/src/Common/tests/gtest_pod_array.cpp @@ -59,6 +59,13 @@ TEST(Common, PODInsertIteratorRange) EXPECT_EQ(big.size(), size * 2); EXPECT_EQ(big.front(), value); EXPECT_EQ(big.back(), value); + + PODArray arr1{1, 2, 3, 4, 5}; + PODArray arr2{11, 12}; + + arr1.insert(arr1.begin() + 3, arr2.begin(), arr2.end()); + + EXPECT_EQ(arr1, (PODArray{1, 2, 3, 11, 12, 4, 5})); } TEST(Common, PODPushBackRawMany) From edb68b7f6b07f77411c033a16bfbde4f8f9018b4 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Thu, 9 Jul 2020 04:22:52 +0300 Subject: [PATCH 09/17] Add test --- src/Common/tests/gtest_pod_array.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Common/tests/gtest_pod_array.cpp b/src/Common/tests/gtest_pod_array.cpp index 3bd2307452b..e0c0834644b 100644 --- a/src/Common/tests/gtest_pod_array.cpp +++ b/src/Common/tests/gtest_pod_array.cpp @@ -66,6 +66,10 @@ TEST(Common, PODInsertIteratorRange) arr1.insert(arr1.begin() + 3, arr2.begin(), arr2.end()); EXPECT_EQ(arr1, (PODArray{1, 2, 3, 11, 12, 4, 5})); + + arr2.insert(arr2.begin() + 1, arr1.begin(), arr1.end()); + + EXPECT_EQ(arr2, (PODArray{11, 1, 2, 3, 11, 12, 4, 5, 12})); } TEST(Common, PODPushBackRawMany) From befbae585b9608a529e9a4ef76337c679ecea0d6 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sun, 12 Jul 2020 21:31:17 +0300 Subject: [PATCH 10/17] Revert the only change that should affect performance in gcc --- src/Common/PODArray.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Common/PODArray.h b/src/Common/PODArray.h index af0b6280a9b..a29d7759513 100644 --- a/src/Common/PODArray.h +++ b/src/Common/PODArray.h @@ -102,7 +102,7 @@ protected: void alloc_for_num_elements(size_t num_elements) { - alloc(minimum_memory_for_elements(num_elements)); + alloc(roundUpToPowerOfTwoOrZero(minimum_memory_for_elements(num_elements))); } template From 0c237de00dd5c145435876a24e7aaed96541a4bd Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Wed, 15 Jul 2020 00:31:50 +0300 Subject: [PATCH 11/17] Fix build --- src/Common/PODArray.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Common/PODArray.h b/src/Common/PODArray.h index a29d7759513..655e6d804f6 100644 --- a/src/Common/PODArray.h +++ b/src/Common/PODArray.h @@ -457,7 +457,7 @@ public: template void insert(iterator it, It1 from_begin, It2 from_end) { - size_t position = it - begin(); + size_t position [[maybe_unused]] = it - begin(); size_t bytes_to_copy = this->byte_size(from_end - from_begin); size_t bytes_to_move = this->byte_size(end() - it); From 52146218302e7ab2ee6bd7c1172fd7e9fa32f5bc Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Thu, 30 Jul 2020 18:20:47 +0300 Subject: [PATCH 12/17] [draft] only fix int16->int32 insert in PODArray --- src/Common/PODArray.h | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/src/Common/PODArray.h b/src/Common/PODArray.h index 655e6d804f6..22d81145ae2 100644 --- a/src/Common/PODArray.h +++ b/src/Common/PODArray.h @@ -215,13 +215,6 @@ public: realloc(roundUpToPowerOfTwoOrZero(minimum_memory_for_elements(n)), std::forward(allocator_params)...); } - template - void reserve_exact(size_t n, TAllocatorParams &&... allocator_params) - { - if (n > capacity()) - realloc(minimum_memory_for_elements(n), std::forward(allocator_params)...); - } - template void resize(size_t n, TAllocatorParams &&... allocator_params) { @@ -229,13 +222,6 @@ public: resize_assume_reserved(n); } - template - void resize_exact(size_t n, TAllocatorParams &&... allocator_params) - { - reserve_exact(n, std::forward(allocator_params)...); - resize_assume_reserved(n); - } - void resize_assume_reserved(const size_t n) { c_end = c_start + byte_size(n); @@ -616,7 +602,7 @@ public: template void assign(size_t n, const T & x, TAllocatorParams &&... allocator_params) { - this->resize_exact(n, std::forward(allocator_params)...); + this->resize(n, std::forward(allocator_params)...); std::fill(begin(), end(), x); } @@ -625,7 +611,7 @@ public: { size_t required_capacity = from_end - from_begin; if (required_capacity > this->capacity()) - this->reserve_exact(required_capacity, std::forward(allocator_params)...); + this->reserve(roundUpToPowerOfTwoOrZero(required_capacity), std::forward(allocator_params)...); size_t bytes_to_copy = this->byte_size(required_capacity); if constexpr (std::is_same_v>) From 59ed586b691c1764aea8a875110b021542e185b7 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Mon, 3 Aug 2020 20:15:08 +0300 Subject: [PATCH 13/17] fixup --- src/Common/PODArray.h | 67 ++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 32 deletions(-) diff --git a/src/Common/PODArray.h b/src/Common/PODArray.h index 22d81145ae2..e7a1447a986 100644 --- a/src/Common/PODArray.h +++ b/src/Common/PODArray.h @@ -21,6 +21,20 @@ #include +template +constexpr bool allow_memcpy = std::is_same_v; + +template +constexpr bool allow_memcpy = std::is_same_v + || std::is_same_v || std::is_same_v; + +template +constexpr bool allow_memcpy = std::is_same_v + || std::is_same_v || std::is_same_v; + +template +constexpr bool allow_memcpy = std::is_same_v + || std::is_same_v || std::is_same_v; namespace DB { @@ -317,7 +331,15 @@ public: insert(from_begin, from_end); } - PODArray(std::initializer_list il) : PODArray(std::begin(il), std::end(il)) {} + PODArray(std::initializer_list il) + { + reserve(std::size(il)); + + for (const auto & x : il) + { + push_back(x); + } + } PODArray(PODArray && other) { @@ -443,6 +465,8 @@ public: template void insert(iterator it, It1 from_begin, It2 from_end) { + static_assert(allow_memcpy, std::decay_t>); + size_t position [[maybe_unused]] = it - begin(); size_t bytes_to_copy = this->byte_size(from_end - from_begin); size_t bytes_to_move = this->byte_size(end() - it); @@ -452,16 +476,7 @@ public: if (unlikely(bytes_to_move)) memcpy(this->c_end + bytes_to_copy - bytes_to_move, this->c_end - bytes_to_move, bytes_to_move); - if constexpr (std::is_same_v>) - { - memcpy(this->c_end - bytes_to_move, reinterpret_cast(&*from_begin), bytes_to_copy); - } - else - { - it = begin() + position; - for (auto from_it = from_begin; from_it != from_end; ++from_it, ++it) - new (&*it) T(*from_it); - } + memcpy(this->c_end - bytes_to_move, reinterpret_cast(&*from_begin), bytes_to_copy); this->c_end += bytes_to_copy; } @@ -469,17 +484,11 @@ public: template void insert_assume_reserved(It1 from_begin, It2 from_end) { - if constexpr (std::is_same_v>) - { - size_t bytes_to_copy = this->byte_size(from_end - from_begin); - memcpy(this->c_end, reinterpret_cast(&*from_begin), bytes_to_copy); - this->c_end += bytes_to_copy; - } - else - { - for (auto it = from_begin; it != from_end; ++it) - push_back(*it); - } + static_assert(allow_memcpy, std::decay_t>); + + size_t bytes_to_copy = this->byte_size(from_end - from_begin); + memcpy(this->c_end, reinterpret_cast(&*from_begin), bytes_to_copy); + this->c_end += bytes_to_copy; } template @@ -609,21 +618,15 @@ public: template void assign(It1 from_begin, It2 from_end, TAllocatorParams &&... allocator_params) { + static_assert(allow_memcpy, std::decay_t>); + size_t required_capacity = from_end - from_begin; if (required_capacity > this->capacity()) this->reserve(roundUpToPowerOfTwoOrZero(required_capacity), std::forward(allocator_params)...); size_t bytes_to_copy = this->byte_size(required_capacity); - if constexpr (std::is_same_v>) - { - memcpy(this->c_start, reinterpret_cast(&*from_begin), bytes_to_copy); - } - else - { - auto it = begin(); - for (auto from_it = from_begin; from_it != from_end; ++from_it) - new (&*it) T(*from_it); - } + memcpy(this->c_start, reinterpret_cast(&*from_begin), bytes_to_copy); + this->c_end = this->c_start + bytes_to_copy; } From 0d4b8bc22677fb2e0e818c5e3f28d45ced5452db Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Mon, 3 Aug 2020 20:22:59 +0300 Subject: [PATCH 14/17] fixup --- src/Common/PODArray.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Common/PODArray.h b/src/Common/PODArray.h index e7a1447a986..36436f7667e 100644 --- a/src/Common/PODArray.h +++ b/src/Common/PODArray.h @@ -333,11 +333,11 @@ public: PODArray(std::initializer_list il) { - reserve(std::size(il)); + this->reserve(std::size(il)); for (const auto & x : il) { - push_back(x); + this->push_back(x); } } From 0d947b64bdccf69854cbfd7b939a89ae65ec0792 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Wed, 5 Aug 2020 02:23:31 +0300 Subject: [PATCH 15/17] fixup --- src/Common/tests/gtest_pod_array.cpp | 39 ---------------------------- 1 file changed, 39 deletions(-) diff --git a/src/Common/tests/gtest_pod_array.cpp b/src/Common/tests/gtest_pod_array.cpp index e0c0834644b..988a3e649ba 100644 --- a/src/Common/tests/gtest_pod_array.cpp +++ b/src/Common/tests/gtest_pod_array.cpp @@ -33,45 +33,6 @@ TEST(Common, PODArrayInsert) EXPECT_EQ(str, std::string(chars.data(), chars.size())); } -TEST(Common, PODInsertIteratorRange) -{ - size_t size = 1 << 20; - char value = 123; - - PODArray big; - PODArray small(size, value); - - EXPECT_EQ(big.size(), 0); - EXPECT_EQ(small.size(), size); - - big.insert(small.begin(), small.end()); - - EXPECT_EQ(big.size(), size); - EXPECT_EQ(big.back(), value); - - big.assign(small.begin(), small.end()); - - EXPECT_EQ(big.size(), size); - EXPECT_EQ(big.back(), value); - - big.insert(big.begin(), small.begin(), small.end()); - - EXPECT_EQ(big.size(), size * 2); - EXPECT_EQ(big.front(), value); - EXPECT_EQ(big.back(), value); - - PODArray arr1{1, 2, 3, 4, 5}; - PODArray arr2{11, 12}; - - arr1.insert(arr1.begin() + 3, arr2.begin(), arr2.end()); - - EXPECT_EQ(arr1, (PODArray{1, 2, 3, 11, 12, 4, 5})); - - arr2.insert(arr2.begin() + 1, arr1.begin(), arr1.end()); - - EXPECT_EQ(arr2, (PODArray{11, 1, 2, 3, 11, 12, 4, 5, 12})); -} - TEST(Common, PODPushBackRawMany) { PODArray chars; From ead62bc9d7c0572054b20f54f3a3a47ac0399f6b Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Wed, 5 Aug 2020 17:12:41 +0300 Subject: [PATCH 16/17] Remove everything except static_assert for same type --- src/Common/PODArray.h | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/Common/PODArray.h b/src/Common/PODArray.h index 36436f7667e..6de85ea7556 100644 --- a/src/Common/PODArray.h +++ b/src/Common/PODArray.h @@ -218,12 +218,8 @@ public: void clear() { c_end = c_start; } - /// Always inline is for clang. GCC works fine. - /// It makes sense when "resize" called in a loop, so we want "reserve" to be inlined into "resize". - /// It improves performance of SQL queries with "materialize" of String by 23%. - /// And we don't need always inline for "reserve_exact" because calling it in a loop is already terribly slow. template - ALWAYS_INLINE void reserve(size_t n, TAllocatorParams &&... allocator_params) + void reserve(size_t n, TAllocatorParams &&... allocator_params) { if (n > capacity()) realloc(roundUpToPowerOfTwoOrZero(minimum_memory_for_elements(n)), std::forward(allocator_params)...); @@ -467,7 +463,6 @@ public: { static_assert(allow_memcpy, std::decay_t>); - size_t position [[maybe_unused]] = it - begin(); size_t bytes_to_copy = this->byte_size(from_end - from_begin); size_t bytes_to_move = this->byte_size(end() - it); From e7e71b861543aedef6178dd263d7114a8d2718c0 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Thu, 6 Aug 2020 16:34:23 +0300 Subject: [PATCH 17/17] Use the memcpy predicate from 6f8a274ba04fdec65f168ebe63f1acd715bdbd72 --- src/Common/PODArray.h | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/src/Common/PODArray.h b/src/Common/PODArray.h index 6de85ea7556..1fddea4507b 100644 --- a/src/Common/PODArray.h +++ b/src/Common/PODArray.h @@ -21,20 +21,15 @@ #include -template -constexpr bool allow_memcpy = std::is_same_v; - -template -constexpr bool allow_memcpy = std::is_same_v - || std::is_same_v || std::is_same_v; - -template -constexpr bool allow_memcpy = std::is_same_v - || std::is_same_v || std::is_same_v; - -template -constexpr bool allow_memcpy = std::is_same_v - || std::is_same_v || std::is_same_v; +/** Whether we can use memcpy instead of a loop with assignment to T from U. + * It is Ok if types are the same. And if types are integral and of the same size, + * example: char, signed char, unsigned char. + * It's not Ok for int and float. + * Don't forget to apply std::decay when using this constexpr. + */ +template +constexpr bool memcpy_can_be_used_for_assignment = std::is_same_v + || (std::is_integral_v && std::is_integral_v && sizeof(T) == sizeof(U)); namespace DB { @@ -461,7 +456,7 @@ public: template void insert(iterator it, It1 from_begin, It2 from_end) { - static_assert(allow_memcpy, std::decay_t>); + static_assert(memcpy_can_be_used_for_assignment, std::decay_t>); size_t bytes_to_copy = this->byte_size(from_end - from_begin); size_t bytes_to_move = this->byte_size(end() - it); @@ -479,7 +474,7 @@ public: template void insert_assume_reserved(It1 from_begin, It2 from_end) { - static_assert(allow_memcpy, std::decay_t>); + static_assert(memcpy_can_be_used_for_assignment, std::decay_t>); size_t bytes_to_copy = this->byte_size(from_end - from_begin); memcpy(this->c_end, reinterpret_cast(&*from_begin), bytes_to_copy); @@ -613,7 +608,7 @@ public: template void assign(It1 from_begin, It2 from_end, TAllocatorParams &&... allocator_params) { - static_assert(allow_memcpy, std::decay_t>); + static_assert(memcpy_can_be_used_for_assignment, std::decay_t>); size_t required_capacity = from_end - from_begin; if (required_capacity > this->capacity())