From cbec41695989a2af9e4f3b714493e29c658bee72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Wilken=20D=C3=B6rrie?= Date: Sun, 26 Apr 2020 19:45:52 +0000 Subject: [PATCH] [base] Specialize __unwrap_iter for CheckedContiguousIterator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change specializes libc++'s __unwrap_iter for base's CheckedContiguousIterator. This results in speeding up STL algorithms like std::copy, which now dispatches to a simple memmove if possible. Bug: 994174 Change-Id: I4970a7cb420a875a6f323f218448d305ab81d44b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1875734 Reviewed-by: Adam Langley Reviewed-by: Chris Palmer Reviewed-by: Daniel Cheng Commit-Queue: Jan Wilken Dörrie Cr-Commit-Position: refs/heads/master@{#762698} --- base/containers/checked_iterators.h | 38 +++++++++ base/containers/checked_iterators_unittest.cc | 79 +++++++++++++++++++ device/fido/cable/v2_handshake.cc | 5 +- 3 files changed, 119 insertions(+), 3 deletions(-) diff --git a/base/containers/checked_iterators.h b/base/containers/checked_iterators.h index 66d927d24654f4..a4517dd94c2649 100644 --- a/base/containers/checked_iterators.h +++ b/base/containers/checked_iterators.h @@ -14,6 +14,18 @@ namespace base { +#if defined(_LIBCPP_VERSION) +// SFINAE friendly type trait that only enables an overload if the passed in +// type is trivially copy assignable, so that it would be safe to implement +// std::copy as a memmove, as libc++ does for some iterator types. +namespace internal { +template +using PointerIfIsTriviallyCopyAssignable = std::enable_if_t< + std::is_trivially_copy_assignable>::value, + T*>; +} // namespace internal +#endif + template class CheckedContiguousIterator { public: @@ -28,6 +40,21 @@ class CheckedContiguousIterator { friend class CheckedContiguousIterator; constexpr CheckedContiguousIterator() = default; + +#if defined(_LIBCPP_VERSION) + // This constructor is required to be able to use a CheckedContiguousIterator + // as the third argument for the optimized pointer based std::copy version in + // libc++. Do not use this constructor otherwise. + constexpr CheckedContiguousIterator(T* ptr) + : start_(ptr), current_(ptr), end_(ptr) {} + + // Friend __unwrap_iter so that it can get unchecked access to the underlying + // pointer. + template + friend constexpr internal::PointerIfIsTriviallyCopyAssignable + __unwrap_iter(CheckedContiguousIterator iter); +#endif + constexpr CheckedContiguousIterator(T* start, const T* end) : CheckedContiguousIterator(start, start, end) {} constexpr CheckedContiguousIterator(const T* start, T* current, const T* end) @@ -204,6 +231,17 @@ class CheckedContiguousIterator { template using CheckedContiguousConstIterator = CheckedContiguousIterator; +#if defined(_LIBCPP_VERSION) +// This is a non-portable overload of libc++'s __unwrap_iter. This allows +// treating a CheckedContiguousIterator as a raw pointer within STL algorithms +// enabling performance optimizations that wouldn't be possible otherwise. +template +constexpr internal::PointerIfIsTriviallyCopyAssignable __unwrap_iter( + CheckedContiguousIterator iter) { + return iter.current_; +} +#endif + } // namespace base #endif // BASE_CONTAINERS_CHECKED_ITERATORS_H_ diff --git a/base/containers/checked_iterators_unittest.cc b/base/containers/checked_iterators_unittest.cc index 9004a020b52cd3..b4a44322d3d61a 100644 --- a/base/containers/checked_iterators_unittest.cc +++ b/base/containers/checked_iterators_unittest.cc @@ -4,6 +4,9 @@ #include "base/containers/checked_iterators.h" +#include +#include + #include "testing/gtest/include/gtest/gtest.h" namespace base { @@ -77,4 +80,80 @@ TEST(CheckedContiguousIterator, ConvertingComparisonOperators) { EXPECT_GE(cbegin, begin); } +#if defined(_LIBCPP_VERSION) +namespace { + +// Helper template that wraps an iterator and disables its dereference and +// increment operations. +template +struct DisableDerefAndIncr : Iterator { + using Iterator::Iterator; + + void operator*() = delete; + void operator++() = delete; + void operator++(int) = delete; +}; + +template +auto __unwrap_iter(DisableDerefAndIncr iter) { + return __unwrap_iter(static_cast(iter)); +} + +} // namespace + +// Tests that using std::copy with CheckedContiguousIterator results in an +// optimized code-path that does not invoke the iterator's dereference and +// increment operations. This would fail to compile if std::copy was not +// optimized. +TEST(CheckedContiguousIterator, OptimizedCopy) { + using Iter = DisableDerefAndIncr>; + static_assert(std::is_same::value, + "Error: Iter should unwrap to int*"); + + int arr_in[5] = {1, 2, 3, 4, 5}; + int arr_out[5]; + + Iter begin(std::begin(arr_in), std::end(arr_in)); + Iter end(std::begin(arr_in), std::end(arr_in), std::end(arr_in)); + std::copy(begin, end, arr_out); + + EXPECT_TRUE(std::equal(std::begin(arr_in), std::end(arr_in), + std::begin(arr_out), std::end(arr_out))); +} + +TEST(CheckedContiguousIterator, UnwrapIter) { + static_assert( + std::is_same()))>::value, + "Error: CCI should unwrap to int*"); + + static_assert( + std::is_same, + decltype(__unwrap_iter( + CheckedContiguousIterator()))>::value, + "Error: CCI should unwrap to CCI"); +} + +// While the result of std::copying into a range via a CCI can't be +// compared to other iterators, it should be possible to re-use it in another +// std::copy expresson. +TEST(CheckedContiguousIterator, ReuseCopyIter) { + using Iter = CheckedContiguousIterator; + + int arr_in[5] = {1, 2, 3, 4, 5}; + int arr_out[5]; + + Iter begin(std::begin(arr_in), std::end(arr_in)); + Iter end(std::begin(arr_in), std::end(arr_in), std::end(arr_in)); + Iter out_begin(std::begin(arr_out), std::end(arr_out)); + + auto out_middle = std::copy_n(begin, 3, out_begin); + std::copy(begin + 3, end, out_middle); + + EXPECT_TRUE(std::equal(std::begin(arr_in), std::end(arr_in), + std::begin(arr_out), std::end(arr_out))); +} + +#endif + } // namespace base diff --git a/device/fido/cable/v2_handshake.cc b/device/fido/cable/v2_handshake.cc index 72178943677816..98351ac37cbf1b 100644 --- a/device/fido/cable/v2_handshake.cc +++ b/device/fido/cable/v2_handshake.cc @@ -38,9 +38,8 @@ bool ConstructNonce(uint32_t counter, base::span out_nonce) { // Nonce is just a little-endian counter. std::array counter_bytes; memcpy(counter_bytes.data(), &counter, sizeof(counter)); - auto remaining = - std::copy(counter_bytes.begin(), counter_bytes.end(), out_nonce.begin()); - std::fill(remaining, out_nonce.end(), 0); + std::copy(counter_bytes.begin(), counter_bytes.end(), out_nonce.begin()); + std::fill(out_nonce.begin() + counter_bytes.size(), out_nonce.end(), 0); return true; }