diff options
Diffstat (limited to 'absl')
-rw-r--r-- | absl/memory/memory.h | 2 | ||||
-rw-r--r-- | absl/meta/type_traits.h | 2 | ||||
-rw-r--r-- | absl/strings/BUILD.bazel | 1 | ||||
-rw-r--r-- | absl/strings/CMakeLists.txt | 1 | ||||
-rw-r--r-- | absl/strings/cord.cc | 154 | ||||
-rw-r--r-- | absl/strings/cord.h | 77 | ||||
-rw-r--r-- | absl/strings/cord_ring_test.cc | 47 | ||||
-rw-r--r-- | absl/strings/cord_test.cc | 9 | ||||
-rw-r--r-- | absl/strings/cord_test_helpers.h | 23 | ||||
-rw-r--r-- | absl/strings/cordz_test.cc | 133 | ||||
-rw-r--r-- | absl/strings/internal/cordz_handle.cc | 7 | ||||
-rw-r--r-- | absl/strings/internal/cordz_handle.h | 13 | ||||
-rw-r--r-- | absl/strings/internal/cordz_handle_test.cc | 13 | ||||
-rw-r--r-- | absl/strings/internal/cordz_info.cc | 21 | ||||
-rw-r--r-- | absl/strings/internal/cordz_info.h | 22 | ||||
-rw-r--r-- | absl/strings/internal/cordz_info_test.cc | 24 | ||||
-rw-r--r-- | absl/strings/internal/cordz_update_tracker.h | 2 | ||||
-rw-r--r-- | absl/strings/internal/cordz_update_tracker_test.cc | 2 |
18 files changed, 394 insertions, 159 deletions
diff --git a/absl/memory/memory.h b/absl/memory/memory.h index 2b5ff623..d6332606 100644 --- a/absl/memory/memory.h +++ b/absl/memory/memory.h @@ -420,7 +420,7 @@ struct pointer_traits<T*> { // // A C++11 compatible implementation of C++17's std::allocator_traits. // -#if __cplusplus >= 201703L +#if __cplusplus >= 201703L || (defined(_MSVC_LANG) && _MSVC_LANG >= 201703L) using std::allocator_traits; #else // __cplusplus >= 201703L template <typename Alloc> diff --git a/absl/meta/type_traits.h b/absl/meta/type_traits.h index b5427a43..e7c12393 100644 --- a/absl/meta/type_traits.h +++ b/absl/meta/type_traits.h @@ -634,7 +634,7 @@ using underlying_type_t = typename std::underlying_type<T>::type; namespace type_traits_internal { -#if __cplusplus >= 201703L +#if __cplusplus >= 201703L || (defined(_MSVC_LANG) && _MSVC_LANG >= 201703L) // std::result_of is deprecated (C++17) or removed (C++20) template<typename> struct result_of; template<typename F, typename... Args> diff --git a/absl/strings/BUILD.bazel b/absl/strings/BUILD.bazel index ae88f7bb..15277de3 100644 --- a/absl/strings/BUILD.bazel +++ b/absl/strings/BUILD.bazel @@ -529,6 +529,7 @@ cc_library( ":cord", ":cord_internal", ":strings", + "//absl/base:config", ], ) diff --git a/absl/strings/CMakeLists.txt b/absl/strings/CMakeLists.txt index 8ebe91e6..1750f7af 100644 --- a/absl/strings/CMakeLists.txt +++ b/absl/strings/CMakeLists.txt @@ -830,6 +830,7 @@ absl_cc_library( COPTS ${ABSL_TEST_COPTS} DEPS + absl::config absl::cord absl::cord_internal absl::strings diff --git a/absl/strings/cord.cc b/absl/strings/cord.cc index 9262aee6..1c2ff9f2 100644 --- a/absl/strings/cord.cc +++ b/absl/strings/cord.cc @@ -56,6 +56,7 @@ using ::absl::cord_internal::CordRepExternal; using ::absl::cord_internal::CordRepFlat; using ::absl::cord_internal::CordRepRing; using ::absl::cord_internal::CordRepSubstring; +using ::absl::cord_internal::CordzUpdateTracker; using ::absl::cord_internal::InlineData; using ::absl::cord_internal::kMaxFlatLength; using ::absl::cord_internal::kMinFlatLength; @@ -281,6 +282,35 @@ static CordRep* NewSubstring(CordRep* child, size_t offset, size_t length) { } } +// Creates a CordRep from the provided string. If the string is large enough, +// and not wasteful, we move the string into an external cord rep, preserving +// the already allocated string contents. +// Requires the provided string length to be larger than `kMaxInline`. +static CordRep* CordRepFromString(std::string&& src) { + assert(src.length() > cord_internal::kMaxInline); + if ( + // String is short: copy data to avoid external block overhead. + src.size() <= kMaxBytesToCopy || + // String is wasteful: copy data to avoid pinning too much unused memory. + src.size() < src.capacity() / 2 + ) { + return NewTree(src.data(), src.size(), 0); + } + + struct StringReleaser { + void operator()(absl::string_view /* data */) {} + std::string data; + }; + const absl::string_view original_data = src; + auto* rep = + static_cast<::absl::cord_internal::CordRepExternalImpl<StringReleaser>*>( + absl::cord_internal::NewExternalRep(original_data, + StringReleaser{std::move(src)})); + // Moving src may have invalidated its data pointer, so adjust it. + rep->base = rep->template get<0>().data.data(); + return rep; +} + // -------------------------------------------------------------------- // Cord::InlineRep functions @@ -486,17 +516,17 @@ static bool RepMemoryUsageLeaf(const CordRep* rep, size_t* total_mem_usage) { return true; } if (rep->tag == EXTERNAL) { - *total_mem_usage += sizeof(CordRepConcat) + rep->length; + // We don't know anything about the embedded / bound data, but we can safely + // assume it is 'at least' a word / pointer to data. In the future we may + // choose to use the 'data' byte as a tag to identify the types of some + // well-known externals, such as a std::string instance. + *total_mem_usage += + sizeof(cord_internal::CordRepExternalImpl<intptr_t>) + rep->length; return true; } return false; } -void Cord::InlineRep::UpdateCordzStatisticsSlow() { - CordRep* tree = as_tree(); - data_.cordz_info()->RecordMetrics(tree->length); -} - void Cord::InlineRep::AssignSlow(const Cord::InlineRep& src) { assert(&src != this); assert(is_tree() || src.is_tree()); @@ -525,42 +555,24 @@ void Cord::InlineRep::UnrefTree() { // -------------------------------------------------------------------- // Constructors and destructors -Cord::Cord(absl::string_view src) : contents_(InlineData::kDefaultInit) { +Cord::Cord(absl::string_view src, MethodIdentifier method) + : contents_(InlineData::kDefaultInit) { const size_t n = src.size(); if (n <= InlineRep::kMaxInline) { contents_.set_data(src.data(), n, true); } else { CordRep* rep = NewTree(src.data(), n, 0); - contents_.EmplaceTree(rep, CordzUpdateTracker::kConstructorString); + contents_.EmplaceTree(rep, method); } } template <typename T, Cord::EnableIfString<T>> -Cord::Cord(T&& src) { - if ( - // String is short: copy data to avoid external block overhead. - src.size() <= kMaxBytesToCopy || - // String is wasteful: copy data to avoid pinning too much unused memory. - src.size() < src.capacity() / 2 - ) { - if (src.size() <= InlineRep::kMaxInline) { - contents_.set_data(src.data(), src.size(), false); - } else { - contents_.set_tree(NewTree(src.data(), src.size(), 0)); - } +Cord::Cord(T&& src) : contents_(InlineData::kDefaultInit) { + if (src.size() <= InlineRep::kMaxInline) { + contents_.set_data(src.data(), src.size(), true); } else { - struct StringReleaser { - void operator()(absl::string_view /* data */) {} - std::string data; - }; - const absl::string_view original_data = src; - auto* rep = static_cast< - ::absl::cord_internal::CordRepExternalImpl<StringReleaser>*>( - absl::cord_internal::NewExternalRep( - original_data, StringReleaser{std::forward<T>(src)})); - // Moving src may have invalidated its data pointer, so adjust it. - rep->base = rep->template get<0>().data.data(); - contents_.set_tree(rep); + CordRep* rep = CordRepFromString(std::forward<T>(src)); + contents_.EmplaceTree(rep, CordzUpdateTracker::kConstructorString); } } @@ -583,6 +595,20 @@ void Cord::Clear() { } } +Cord& Cord::AssignLargeString(std::string&& src) { + auto constexpr method = CordzUpdateTracker::kAssignString; + assert(src.size() > kMaxBytesToCopy); + CordRep* rep = CordRepFromString(std::move(src)); + if (CordRep* tree = contents_.tree()) { + CordzUpdateScope scope(contents_.cordz_info(), method); + contents_.SetTree(rep, scope); + CordRep::Unref(tree); + } else { + contents_.EmplaceTree(rep, method); + } + return *this; +} + Cord& Cord::operator=(absl::string_view src) { auto constexpr method = CordzUpdateTracker::kAssignString; const char* data = src.data(); @@ -616,18 +642,6 @@ Cord& Cord::operator=(absl::string_view src) { return *this; } -template <typename T, Cord::EnableIfString<T>> -Cord& Cord::operator=(T&& src) { - if (src.size() <= kMaxBytesToCopy) { - *this = absl::string_view(src); - } else { - *this = Cord(std::forward<T>(src)); - } - return *this; -} - -template Cord& Cord::operator=(std::string&& src); - // TODO(sanjay): Move to Cord::InlineRep section of file. For now, // we keep it here to make diffs easier. void Cord::InlineRep::AppendArray(absl::string_view src, @@ -653,10 +667,8 @@ void Cord::InlineRep::AppendArray(absl::string_view src, return; } - // It is possible that src.data() == data_, but when we transition from an - // InlineRep to a tree we need to assign data_ = root via set_tree. To - // avoid corrupting the source data before we copy it, delay calling - // set_tree until after we've copied data. + // Note: we don't concern ourselves if src aliases data stored in the + // inlined data of 'this', as we update the InlineData only at the end. // We are going from an inline size to beyond inline size. Make the new size // either double the inlined size, or the added size + 10%. const size_t size1 = inline_length * 2 + src.size(); @@ -762,7 +774,8 @@ void Cord::Append(T&& src) { if (src.size() <= kMaxBytesToCopy) { Append(absl::string_view(src)); } else { - Append(Cord(std::forward<T>(src))); + CordRep* rep = CordRepFromString(std::forward<T>(src)); + contents_.AppendTree(rep, CordzUpdateTracker::kAppendString); } } @@ -804,7 +817,8 @@ inline void Cord::Prepend(T&& src) { if (src.size() <= kMaxBytesToCopy) { Prepend(absl::string_view(src)); } else { - Prepend(Cord(std::forward<T>(src))); + CordRep* rep = CordRepFromString(std::forward<T>(src)); + contents_.PrependTree(rep, CordzUpdateTracker::kPrependString); } } @@ -988,22 +1002,6 @@ static CordRep* NewSubRange(CordRep* node, size_t pos, size_t n) { return results[0]; } -void Cord::CopyDataAtPosition(size_t pos, size_t new_size, char* dest) const { - assert(new_size <= cord_internal::kMaxInline); - assert(pos <= size()); - assert(new_size <= size() - pos); - Cord::ChunkIterator it = chunk_begin(); - it.AdvanceBytes(pos); - size_t remaining_size = new_size; - while (remaining_size > it->size()) { - cord_internal::SmallMemmove(dest, it->data(), it->size()); - remaining_size -= it->size(); - dest += it->size(); - ++it; - } - cord_internal::SmallMemmove(dest, it->data(), remaining_size); -} - Cord Cord::Subcord(size_t pos, size_t new_size) const { Cord sub_cord; size_t length = size(); @@ -1020,7 +1018,17 @@ Cord Cord::Subcord(size_t pos, size_t new_size) const { } if (new_size <= InlineRep::kMaxInline) { - CopyDataAtPosition(pos, new_size, sub_cord.contents_.data_.as_chars()); + char* dest = sub_cord.contents_.data_.as_chars(); + Cord::ChunkIterator it = chunk_begin(); + it.AdvanceBytes(pos); + size_t remaining_size = new_size; + while (remaining_size > it->size()) { + cord_internal::SmallMemmove(dest, it->data(), it->size()); + remaining_size -= it->size(); + dest += it->size(); + ++it; + } + cord_internal::SmallMemmove(dest, it->data(), remaining_size); sub_cord.contents_.set_inline_size(new_size); return sub_cord; } @@ -1474,6 +1482,7 @@ Cord Cord::ChunkIterator::AdvanceAndReadBytes(size_t n) { ABSL_HARDENING_ASSERT(bytes_remaining_ >= n && "Attempted to iterate past `end()`"); Cord subcord; + auto constexpr method = CordzUpdateTracker::kCordReader; if (n <= InlineRep::kMaxInline) { // Range to read fits in inline data. Flatten it. @@ -1496,11 +1505,12 @@ Cord Cord::ChunkIterator::AdvanceAndReadBytes(size_t n) { if (ring_reader_) { size_t chunk_size = current_chunk_.size(); if (n <= chunk_size && n <= kMaxBytesToCopy) { - subcord = Cord(current_chunk_.substr(0, n)); + subcord = Cord(current_chunk_.substr(0, n), method); } else { auto* ring = CordRep::Ref(ring_reader_.ring())->ring(); size_t offset = ring_reader_.length() - bytes_remaining_; - subcord.contents_.set_tree(CordRepRing::SubRing(ring, offset, n)); + CordRep* rep = CordRepRing::SubRing(ring, offset, n); + subcord.contents_.EmplaceTree(rep, method); } if (n < chunk_size) { bytes_remaining_ -= n; @@ -1519,7 +1529,7 @@ Cord Cord::ChunkIterator::AdvanceAndReadBytes(size_t n) { const char* data = subnode->tag == EXTERNAL ? subnode->external()->base : subnode->flat()->Data(); subnode = NewSubstring(subnode, current_chunk_.data() - data, n); - subcord.contents_.set_tree(VerifyTree(subnode)); + subcord.contents_.EmplaceTree(VerifyTree(subnode), method); RemoveChunkPrefix(n); return subcord; } @@ -1562,7 +1572,7 @@ Cord Cord::ChunkIterator::AdvanceAndReadBytes(size_t n) { if (node == nullptr) { // We have reached the end of the Cord. assert(bytes_remaining_ == 0); - subcord.contents_.set_tree(VerifyTree(subnode)); + subcord.contents_.EmplaceTree(VerifyTree(subnode), method); return subcord; } @@ -1602,7 +1612,7 @@ Cord Cord::ChunkIterator::AdvanceAndReadBytes(size_t n) { current_chunk_ = absl::string_view(data + offset + n, length - n); current_leaf_ = node; bytes_remaining_ -= n; - subcord.contents_.set_tree(VerifyTree(subnode)); + subcord.contents_.EmplaceTree(VerifyTree(subnode), method); return subcord; } diff --git a/absl/strings/cord.h b/absl/strings/cord.h index d7b43247..8abc4742 100644 --- a/absl/strings/cord.h +++ b/absl/strings/cord.h @@ -678,6 +678,10 @@ class Cord { using InlineData = cord_internal::InlineData; using MethodIdentifier = CordzUpdateTracker::MethodIdentifier; + // Creates a cord instance with `method` representing the originating + // public API call causing the cord to be created. + explicit Cord(absl::string_view src, MethodIdentifier method); + friend class CordTestPeer; friend bool operator==(const Cord& lhs, const Cord& rhs); friend bool operator==(const Cord& lhs, absl::string_view rhs); @@ -692,10 +696,6 @@ class Cord { // called by Flatten() when the cord was not already flat. absl::string_view FlattenSlowPath(); - // Copies `new_size` bytes starting at `pos` into `dest`. Requires at least - // `new_size` bytes to be available, and `new_size` to be <= kMaxInline. - void CopyDataAtPosition(size_t pos, size_t new_size, char* dest) const; - // Actual cord contents are hidden inside the following simple // class so that we can isolate the bulk of cord.cc from changes // to the representation. @@ -725,11 +725,6 @@ class Cord { // Returns nullptr if holding bytes absl::cord_internal::CordRep* tree() const; absl::cord_internal::CordRep* as_tree() const; - // Discards old pointer, if any - void set_tree(absl::cord_internal::CordRep* rep); - // Replaces a tree with a new root. This is faster than set_tree, but it - // should only be used when it's clear that the old rep was a tree. - void replace_tree(absl::cord_internal::CordRep* rep); // Returns non-null iff was holding a pointer absl::cord_internal::CordRep* clear(); // Converts to pointer if necessary. @@ -831,11 +826,6 @@ class Cord { // Resets the current cordz_info to null / empty. void clear_cordz_info() { data_.clear_cordz_info(); } - // Updates the cordz statistics. info may be nullptr if the CordzInfo object - // is unknown. - void UpdateCordzStatistics(); - void UpdateCordzStatisticsSlow(); - private: friend class Cord; @@ -892,6 +882,10 @@ class Cord { template <typename C> void AppendImpl(C&& src); + // Assigns the value in 'src' to this instance, 'stealing' its contents. + // Requires src.length() > kMaxBytesToCopy. + Cord& AssignLargeString(std::string&& src); + // Helper for AbslHashValue(). template <typename H> H HashFragmented(H hash_state) const { @@ -994,8 +988,11 @@ inline CordRep* NewExternalRep(absl::string_view data, template <typename Releaser> Cord MakeCordFromExternal(absl::string_view data, Releaser&& releaser) { Cord cord; - cord.contents_.set_tree(::absl::cord_internal::NewExternalRep( - data, std::forward<Releaser>(releaser))); + if (auto* rep = ::absl::cord_internal::NewExternalRep( + data, std::forward<Releaser>(releaser))) { + cord.contents_.EmplaceTree(rep, + Cord::MethodIdentifier::kMakeCordFromExternal); + } return cord; } @@ -1119,36 +1116,6 @@ inline void Cord::InlineRep::CommitTree(const CordRep* old_rep, CordRep* rep, } } -inline void Cord::InlineRep::set_tree(absl::cord_internal::CordRep* rep) { - if (rep == nullptr) { - if (data_.is_tree()) { - CordzInfo::MaybeUntrackCord(data_.cordz_info()); - } - ResetToEmpty(); - } else { - if (data_.is_tree()) { - // `data_` already holds a 'tree' value and an optional cordz_info value. - // Replace the tree value only, leaving the cordz_info value unchanged. - data_.set_tree(rep); - } else { - // `data_` contains inlined data: initialize data_ to tree value `rep`. - data_.make_tree(rep); - CordzInfo::MaybeTrackCord(data_, CordzUpdateTracker::kUnknown); - } - UpdateCordzStatistics(); - } -} - -inline void Cord::InlineRep::replace_tree(absl::cord_internal::CordRep* rep) { - ABSL_ASSERT(is_tree()); - if (ABSL_PREDICT_FALSE(rep == nullptr)) { - set_tree(rep); - return; - } - data_.set_tree(rep); - UpdateCordzStatistics(); -} - inline absl::cord_internal::CordRep* Cord::InlineRep::clear() { if (is_tree()) { CordzInfo::MaybeUntrackCord(cordz_info()); @@ -1165,13 +1132,11 @@ inline void Cord::InlineRep::CopyToArray(char* dst) const { cord_internal::SmallMemmove(dst, data_.as_chars(), n); } -inline void Cord::InlineRep::UpdateCordzStatistics() { - if (ABSL_PREDICT_TRUE(!is_profiled())) return; - UpdateCordzStatisticsSlow(); -} - constexpr inline Cord::Cord() noexcept {} +inline Cord::Cord(absl::string_view src) + : Cord(src, CordzUpdateTracker::kConstructorString) {} + template <typename T> constexpr Cord::Cord(strings_internal::StringConstant<T>) : contents_(strings_internal::StringConstant<T>::value.size() <= @@ -1187,6 +1152,15 @@ inline Cord& Cord::operator=(const Cord& x) { return *this; } +template <typename T, Cord::EnableIfString<T>> +Cord& Cord::operator=(T&& src) { + if (src.size() <= cord_internal::kMaxBytesToCopy) { + return operator=(absl::string_view(src)); + } else { + return AssignLargeString(std::forward<T>(src)); + } +} + inline Cord::Cord(const Cord& src) : contents_(src.contents_) {} inline Cord::Cord(Cord&& src) noexcept : contents_(std::move(src.contents_)) {} @@ -1201,7 +1175,6 @@ inline Cord& Cord::operator=(Cord&& x) noexcept { } extern template Cord::Cord(std::string&& src); -extern template Cord& Cord::operator=(std::string&& src); inline size_t Cord::size() const { // Length is 1st field in str.rep_ diff --git a/absl/strings/cord_ring_test.cc b/absl/strings/cord_ring_test.cc index 2943cf38..cc8fbaf9 100644 --- a/absl/strings/cord_ring_test.cc +++ b/absl/strings/cord_ring_test.cc @@ -98,15 +98,22 @@ using TestParams = std::vector<TestParam>; // Matcher validating when mutable copies are required / performed. MATCHER_P2(EqIfPrivate, param, rep, absl::StrCat("Equal 0x", absl::Hex(rep), " if private")) { - return param.refcount_is_one ? arg == rep : arg != rep; + return param.refcount_is_one ? arg == rep : true; } // Matcher validating when mutable copies are required / performed. MATCHER_P2(EqIfPrivateAndCapacity, param, rep, absl::StrCat("Equal 0x", absl::Hex(rep), " if private and capacity")) { - return (param.refcount_is_one && param.with_capacity) ? arg == rep - : arg != rep; + return (param.refcount_is_one && param.with_capacity) ? arg == rep : true; +} + +// Matcher validating a shared ring was re-allocated. Should only be used for +// tests doing exactly one update as subsequent updates could return the +// original (freed and re-used) pointer. +MATCHER_P2(NeIfShared, param, rep, + absl::StrCat("Not equal 0x", absl::Hex(rep), " if shared")) { + return param.refcount_is_one ? true : arg != rep; } MATCHER_P2(EqIfInputPrivate, param, rep, "Equal if input is private") { @@ -518,6 +525,7 @@ TEST_P(CordRingCreateTest, CreateFromRing) { CordRepRing* result = NeedsUnref(CordRepRing::Create(ring)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivate(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAreArray(kFoxFlats)); } @@ -655,6 +663,7 @@ TEST_P(CordRingBuildTest, AppendFlat) { CordRepRing* result = NeedsUnref(CordRepRing::Append(ring, MakeFlat(str2))); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(result->length, Eq(str1.size() + str2.size())); EXPECT_THAT(ToFlats(result), ElementsAre(str1, str2)); } @@ -666,6 +675,7 @@ TEST_P(CordRingBuildTest, PrependFlat) { CordRepRing* result = NeedsUnref(CordRepRing::Prepend(ring, MakeFlat(str2))); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(result->length, Eq(str1.size() + str2.size())); EXPECT_THAT(ToFlats(result), ElementsAre(str2, str1)); } @@ -677,6 +687,7 @@ TEST_P(CordRingBuildTest, AppendString) { CordRepRing* result = NeedsUnref(CordRepRing::Append(ring, str2)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(result->length, Eq(str1.size() + str2.size())); EXPECT_THAT(ToFlats(result), ElementsAre(str1, str2)); } @@ -689,6 +700,7 @@ TEST_P(CordRingBuildTest, AppendStringHavingExtra) { ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result->length, Eq(str1.size() + str2.size())); EXPECT_THAT(result, EqIfPrivate(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); } TEST_P(CordRingBuildTest, AppendStringHavingPartialExtra) { @@ -710,6 +722,7 @@ TEST_P(CordRingBuildTest, AppendStringHavingPartialExtra) { ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result->length, Eq(str1.size() + str2.size())); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); if (GetParam().refcount_is_one) { EXPECT_THAT(ToFlats(result), ElementsAre(StrCat(str1, str1a), str2a)); } else { @@ -725,6 +738,7 @@ TEST_P(CordRingBuildTest, AppendStringHavingExtraInSubstring) { CordRepRing* result = NeedsUnref(CordRepRing::Append(ring, str2)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivate(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(result->length, Eq(4 + str2.size())); if (GetParam().refcount_is_one) { EXPECT_THAT(ToFlats(result), ElementsAre(StrCat("1234", str2))); @@ -758,6 +772,7 @@ TEST_P(CordRingBuildTest, AppendStringHavingSharedExtra) { CordRepRing* result = NeedsUnref(CordRepRing::Append(ring, str2)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(result->length, Eq(4 + str2.size())); EXPECT_THAT(ToFlats(result), ElementsAre("1234", str2)); @@ -802,6 +817,7 @@ TEST_P(CordRingBuildTest, PrependStringHavingExtra) { CordRepRing* result = NeedsUnref(CordRepRing::Prepend(ring, str2)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivate(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(result->length, Eq(4 + str2.size())); if (GetParam().refcount_is_one) { EXPECT_THAT(ToFlats(result), ElementsAre(StrCat(str2, "1234"))); @@ -833,6 +849,7 @@ TEST_P(CordRingBuildTest, PrependStringHavingSharedExtra) { ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result->length, Eq(str1a.size() + str2.size())); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAre(str2, str1a)); CordRep::Unref(shared_type == 1 ? flat1 : flat); } @@ -920,6 +937,7 @@ TEST_P(CordRingSubTest, SubRing) { result = NeedsUnref(CordRepRing::SubRing(ring, offset, len)); ASSERT_THAT(result, IsValidRingBuffer()); ASSERT_THAT(result, EqIfPrivate(GetParam(), ring)); + ASSERT_THAT(result, NeIfShared(GetParam(), ring)); ASSERT_THAT(ToString(result), Eq(all.substr(offset, len))); } } @@ -945,6 +963,7 @@ TEST_P(CordRingSubTest, SubRingFromLargeExternal) { result = NeedsUnref(CordRepRing::SubRing(ring, offset, len)); ASSERT_THAT(result, IsValidRingBuffer()); ASSERT_THAT(result, EqIfPrivate(GetParam(), ring)); + ASSERT_THAT(result, NeIfShared(GetParam(), ring)); auto str = ToString(result); ASSERT_THAT(str, SizeIs(len)); ASSERT_THAT(str, Eq(all.substr(offset, len))); @@ -966,6 +985,7 @@ TEST_P(CordRingSubTest, RemovePrefix) { result = NeedsUnref(CordRepRing::RemovePrefix(ring, len)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivate(GetParam(), ring)); + ASSERT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToString(result), Eq(all.substr(len))); } } @@ -996,8 +1016,9 @@ TEST_P(CordRingSubTest, RemoveSuffix) { ring = RefIfShared(FromFlats(flats, composition)); result = NeedsUnref(CordRepRing::RemoveSuffix(ring, len)); ASSERT_THAT(result, IsValidRingBuffer()); - EXPECT_THAT(result, EqIfPrivate(GetParam(), ring)); - EXPECT_THAT(ToString(result), Eq(all.substr(0, all.size() - len))); + ASSERT_THAT(result, EqIfPrivate(GetParam(), ring)); + ASSERT_THAT(result, NeIfShared(GetParam(), ring)); + ASSERT_THAT(ToString(result), Eq(all.substr(0, all.size() - len))); } } @@ -1010,6 +1031,7 @@ TEST_P(CordRingSubTest, AppendRing) { CordRepRing* result = NeedsUnref(CordRepRing::Append(ring, child)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivate(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAreArray(kFoxFlats)); } @@ -1023,6 +1045,7 @@ TEST_P(CordRingBuildInputTest, AppendRingWithFlatOffset) { CordRepRing* result = NeedsUnref(CordRepRing::Append(ring, stripped)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAre("Head", "brown ", "fox ", "jumps ", "over ", "the ", "lazy ", "dog")); } @@ -1037,6 +1060,7 @@ TEST_P(CordRingBuildInputTest, AppendRingWithBrokenOffset) { CordRepRing* result = NeedsUnref(CordRepRing::Append(ring, stripped)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAre("Head", "umps ", "over ", "the ", "lazy ", "dog")); } @@ -1051,6 +1075,7 @@ TEST_P(CordRingBuildInputTest, AppendRingWithFlatLength) { CordRepRing* result = NeedsUnref(CordRepRing::Append(ring, stripped)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAre("Head", "The ", "quick ", "brown ", "fox ", "jumps ", "over ", "the ")); } @@ -1065,6 +1090,7 @@ TEST_P(CordRingBuildTest, AppendRingWithBrokenFlatLength) { CordRepRing* result = NeedsUnref(CordRepRing::Append(ring, stripped)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAre("Head", "The ", "quick ", "brown ", "fox ", "jumps ", "ov")); } @@ -1079,6 +1105,7 @@ TEST_P(CordRingBuildTest, AppendRingMiddlePiece) { CordRepRing* result = NeedsUnref(CordRepRing::Append(ring, stripped)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAre("Head", "ck ", "brown ", "fox ", "jum")); } @@ -1093,6 +1120,7 @@ TEST_P(CordRingBuildTest, AppendRingSinglePiece) { CordRepRing* result = NeedsUnref(CordRepRing::Append(ring, stripped)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAre("Head", "row")); } @@ -1110,6 +1138,7 @@ TEST_P(CordRingBuildInputTest, AppendRingSinglePieceWithPrefix) { CordRepRing* result = NeedsUnref(CordRepRing::Append(ring, stripped)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAre("Prepend", "Head", "row")); } @@ -1123,6 +1152,7 @@ TEST_P(CordRingBuildInputTest, PrependRing) { CordRepRing* result = NeedsUnref(CordRepRing::Prepend(ring, child)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAreArray(kFoxFlats)); } @@ -1136,6 +1166,7 @@ TEST_P(CordRingBuildInputTest, PrependRingWithFlatOffset) { CordRepRing* result = NeedsUnref(CordRepRing::Prepend(ring, stripped)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAre("brown ", "fox ", "jumps ", "over ", "the ", "lazy ", "dog", "Tail")); } @@ -1149,6 +1180,7 @@ TEST_P(CordRingBuildInputTest, PrependRingWithBrokenOffset) { CordRep* stripped = RefIfInputSharedIndirect(RemovePrefix(21, child)); CordRepRing* result = NeedsUnref(CordRepRing::Prepend(ring, stripped)); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAre("umps ", "over ", "the ", "lazy ", "dog", "Tail")); } @@ -1163,6 +1195,7 @@ TEST_P(CordRingBuildInputTest, PrependRingWithFlatLength) { CordRepRing* result = NeedsUnref(CordRepRing::Prepend(ring, stripped)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAre("The ", "quick ", "brown ", "fox ", "jumps ", "over ", "the ", "Tail")); } @@ -1177,6 +1210,7 @@ TEST_P(CordRingBuildInputTest, PrependRingWithBrokenFlatLength) { CordRepRing* result = NeedsUnref(CordRepRing::Prepend(ring, stripped)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAre("The ", "quick ", "brown ", "fox ", "jumps ", "ov", "Tail")); } @@ -1192,6 +1226,7 @@ TEST_P(CordRingBuildInputTest, PrependRingMiddlePiece) { CordRepRing* result = NeedsUnref(CordRepRing::Prepend(ring, stripped)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAre("ck ", "brown ", "fox ", "jum", "Tail")); } @@ -1206,6 +1241,7 @@ TEST_P(CordRingBuildInputTest, PrependRingSinglePiece) { CordRepRing* result = NeedsUnref(CordRepRing::Prepend(ring, stripped)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAre("row", "Tail")); } @@ -1222,6 +1258,7 @@ TEST_P(CordRingBuildInputTest, PrependRingSinglePieceWithPrefix) { CordRepRing* result = NeedsUnref(CordRepRing::Prepend(ring, stripped)); ASSERT_THAT(result, IsValidRingBuffer()); EXPECT_THAT(result, EqIfPrivateAndCapacity(GetParam(), ring)); + EXPECT_THAT(result, NeIfShared(GetParam(), ring)); EXPECT_THAT(ToFlats(result), ElementsAre("row", "Prepend", "Tail")); } diff --git a/absl/strings/cord_test.cc b/absl/strings/cord_test.cc index 74a90863..14eca155 100644 --- a/absl/strings/cord_test.cc +++ b/absl/strings/cord_test.cc @@ -190,14 +190,15 @@ class CordTestPeer { } static Cord MakeSubstring(Cord src, size_t offset, size_t length) { - Cord cord = src; - ABSL_RAW_CHECK(cord.contents_.is_tree(), "Can not be inlined"); + ABSL_RAW_CHECK(src.contents_.is_tree(), "Can not be inlined"); + Cord cord; auto* rep = new cord_internal::CordRepSubstring; rep->tag = cord_internal::SUBSTRING; - rep->child = cord.contents_.tree(); + rep->child = cord_internal::CordRep::Ref(src.contents_.tree()); rep->start = offset; rep->length = length; - cord.contents_.replace_tree(rep); + cord.contents_.EmplaceTree(rep, + cord_internal::CordzUpdateTracker::kSubCord); return cord; } }; diff --git a/absl/strings/cord_test_helpers.h b/absl/strings/cord_test_helpers.h index 6ccccc51..31a1dc89 100644 --- a/absl/strings/cord_test_helpers.h +++ b/absl/strings/cord_test_helpers.h @@ -19,7 +19,9 @@ #include <cstdint> #include <iostream> +#include <string> +#include "absl/base/config.h" #include "absl/strings/cord.h" #include "absl/strings/internal/cord_internal.h" #include "absl/strings/string_view.h" @@ -29,10 +31,27 @@ ABSL_NAMESPACE_BEGIN // Cord sizes relevant for testing enum class TestCordSize { + // An empty value kEmpty = 0, + + // An inlined string value kInlined = cord_internal::kMaxInline / 2 + 1, + + // 'Well known' SSO lengths (excluding terminating zero). + // libstdcxx has a maximum SSO of 15, libc++ has a maximum SSO of 22. + kStringSso1 = 15, + kStringSso2 = 22, + + // A string value which is too large to fit in inlined data, but small enough + // such that Cord prefers copying the value if possible, i.e.: not stealing + // std::string inputs, or referencing existing CordReps on Append, etc. kSmall = cord_internal::kMaxBytesToCopy / 2 + 1, + + // A string value large enough that Cord prefers to reference or steal from + // existing inputs rather than copying contents of the input. kMedium = cord_internal::kMaxFlatLength / 2 + 1, + + // A string value large enough to cause it to be stored in mutliple flats. kLarge = cord_internal::kMaxFlatLength * 4 }; @@ -45,6 +64,10 @@ inline absl::string_view ToString(TestCordSize size) { return "Inlined"; case TestCordSize::kSmall: return "Small"; + case TestCordSize::kStringSso1: + return "StringSso1"; + case TestCordSize::kStringSso2: + return "StringSso2"; case TestCordSize::kMedium: return "Medium"; case TestCordSize::kLarge: diff --git a/absl/strings/cordz_test.cc b/absl/strings/cordz_test.cc index 84947cfd..aeb3d13f 100644 --- a/absl/strings/cordz_test.cc +++ b/absl/strings/cordz_test.cc @@ -34,6 +34,7 @@ #ifdef ABSL_INTERNAL_CORDZ_ENABLED using testing::Eq; +using testing::AnyOf; namespace absl { ABSL_NAMESPACE_BEGIN @@ -84,24 +85,50 @@ class CordzUpdateTest : public testing::TestWithParam<TestCordSize> { Cord cord_{MakeString(GetParam())}; }; +template <typename T> +std::string ParamToString(::testing::TestParamInfo<T> param) { + return std::string(ToString(param.param)); +} + INSTANTIATE_TEST_SUITE_P(WithParam, CordzUpdateTest, testing::Values(TestCordSize::kEmpty, TestCordSize::kInlined, TestCordSize::kLarge), TestParamToString); -TEST(CordzTest, ConstructSmallString) { +class CordzStringTest : public testing::TestWithParam<TestCordSize> { + private: + CordzSamplingIntervalHelper sample_every_{1}; +}; + +INSTANTIATE_TEST_SUITE_P(WithParam, CordzStringTest, + testing::Values(TestCordSize::kInlined, + TestCordSize::kStringSso1, + TestCordSize::kStringSso2, + TestCordSize::kSmall, + TestCordSize::kLarge), + ParamToString<TestCordSize>); + +TEST(CordzTest, ConstructSmallArray) { CordzSamplingIntervalHelper sample_every{1}; Cord cord(MakeString(TestCordSize::kSmall)); EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString)); } -TEST(CordzTest, ConstructLargeString) { +TEST(CordzTest, ConstructLargeArray) { CordzSamplingIntervalHelper sample_every{1}; Cord cord(MakeString(TestCordSize::kLarge)); EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString)); } +TEST_P(CordzStringTest, ConstructString) { + CordzSamplingIntervalHelper sample_every{1}; + Cord cord(std::string(Length(GetParam()), '.')); + if (Length(GetParam()) > kMaxInline) { + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString)); + } +} + TEST(CordzTest, CopyConstruct) { CordzSamplingIntervalHelper sample_every{1}; Cord src = UnsampledCord(MakeString(TestCordSize::kLarge)); @@ -151,6 +178,28 @@ TEST_P(CordzUpdateTest, AssignInlinedArray) { EXPECT_THAT(GetCordzInfoForTesting(cord()), Eq(nullptr)); } +TEST_P(CordzStringTest, AssignStringToInlined) { + Cord cord; + cord = std::string(Length(GetParam()), '.'); + if (Length(GetParam()) > kMaxInline) { + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kAssignString)); + } +} + +TEST_P(CordzStringTest, AssignStringToCord) { + Cord cord(MakeString(TestCordSize::kLarge)); + cord = std::string(Length(GetParam()), '.'); + if (Length(GetParam()) > kMaxInline) { + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString)); + EXPECT_THAT(cord, CordzMethodCountEq(Method::kAssignString, 1)); + } +} + +TEST_P(CordzUpdateTest, AssignInlinedString) { + cord() = std::string(Length(TestCordSize::kInlined), '.'); + EXPECT_THAT(GetCordzInfoForTesting(cord()), Eq(nullptr)); +} + TEST_P(CordzUpdateTest, AppendCord) { Cord src = UnsampledCord(MakeString(TestCordSize::kLarge)); cord().Append(src); @@ -162,12 +211,6 @@ TEST_P(CordzUpdateTest, MoveAppendCord) { EXPECT_THAT(cord(), HasValidCordzInfoOf(InitialOr(Method::kAppendCord))); } -TEST_P(CordzUpdateTest, PrependCord) { - Cord src = UnsampledCord(MakeString(TestCordSize::kLarge)); - cord().Prepend(src); - EXPECT_THAT(cord(), HasValidCordzInfoOf(InitialOr(Method::kPrependCord))); -} - TEST_P(CordzUpdateTest, AppendSmallArray) { cord().Append(MakeString(TestCordSize::kSmall)); EXPECT_THAT(cord(), HasValidCordzInfoOf(InitialOr(Method::kAppendString))); @@ -178,6 +221,80 @@ TEST_P(CordzUpdateTest, AppendLargeArray) { EXPECT_THAT(cord(), HasValidCordzInfoOf(InitialOr(Method::kAppendString))); } +TEST_P(CordzStringTest, AppendStringToEmpty) { + Cord cord; + cord.Append(std::string(Length(GetParam()), '.')); + if (Length(GetParam()) > kMaxInline) { + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kAppendString)); + } +} + +TEST_P(CordzStringTest, AppendStringToInlined) { + Cord cord(MakeString(TestCordSize::kInlined)); + cord.Append(std::string(Length(GetParam()), '.')); + if (Length(TestCordSize::kInlined) + Length(GetParam()) > kMaxInline) { + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kAppendString)); + } +} + +TEST_P(CordzStringTest, AppendStringToCord) { + Cord cord(MakeString(TestCordSize::kLarge)); + cord.Append(std::string(Length(GetParam()), '.')); + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString)); + EXPECT_THAT(cord, CordzMethodCountEq(Method::kAppendString, 1)); +} + +TEST(CordzTest, MakeCordFromExternal) { + CordzSamplingIntervalHelper sample_every{1}; + Cord cord = MakeCordFromExternal("Hello world", [](absl::string_view) {}); + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kMakeCordFromExternal)); +} + +TEST(CordzTest, MakeCordFromEmptyExternal) { + CordzSamplingIntervalHelper sample_every{1}; + Cord cord = MakeCordFromExternal({}, [](absl::string_view) {}); + EXPECT_THAT(GetCordzInfoForTesting(cord), Eq(nullptr)); +} + +TEST_P(CordzUpdateTest, PrependCord) { + Cord src = UnsampledCord(MakeString(TestCordSize::kLarge)); + cord().Prepend(src); + EXPECT_THAT(cord(), HasValidCordzInfoOf(InitialOr(Method::kPrependCord))); +} + +TEST_P(CordzUpdateTest, PrependSmallArray) { + cord().Prepend(MakeString(TestCordSize::kSmall)); + EXPECT_THAT(cord(), HasValidCordzInfoOf(InitialOr(Method::kPrependString))); +} + +TEST_P(CordzUpdateTest, PrependLargeArray) { + cord().Prepend(MakeString(TestCordSize::kLarge)); + EXPECT_THAT(cord(), HasValidCordzInfoOf(InitialOr(Method::kPrependString))); +} + +TEST_P(CordzStringTest, PrependStringToEmpty) { + Cord cord; + cord.Prepend(std::string(Length(GetParam()), '.')); + if (Length(GetParam()) > kMaxInline) { + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kPrependString)); + } +} + +TEST_P(CordzStringTest, PrependStringToInlined) { + Cord cord(MakeString(TestCordSize::kInlined)); + cord.Prepend(std::string(Length(GetParam()), '.')); + if (Length(TestCordSize::kInlined) + Length(GetParam()) > kMaxInline) { + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kPrependString)); + } +} + +TEST_P(CordzStringTest, PrependStringToCord) { + Cord cord(MakeString(TestCordSize::kLarge)); + cord.Prepend(std::string(Length(GetParam()), '.')); + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString)); + EXPECT_THAT(cord, CordzMethodCountEq(Method::kPrependString, 1)); +} + TEST(CordzTest, RemovePrefix) { CordzSamplingIntervalHelper sample_every(1); Cord cord(MakeString(TestCordSize::kLarge)); diff --git a/absl/strings/internal/cordz_handle.cc b/absl/strings/internal/cordz_handle.cc index 5297ec8e..a73fefed 100644 --- a/absl/strings/internal/cordz_handle.cc +++ b/absl/strings/internal/cordz_handle.cc @@ -68,11 +68,16 @@ CordzHandle::~CordzHandle() { } } +bool CordzHandle::SafeToDelete() const { + return is_snapshot_ || queue_->IsEmpty(); +} + void CordzHandle::Delete(CordzHandle* handle) { + assert(handle); if (handle) { handle->ODRCheck(); Queue* const queue = handle->queue_; - if (!handle->is_snapshot_ && !queue->IsEmpty()) { + if (!handle->SafeToDelete()) { SpinLockHolder lock(&queue->mutex); CordzHandle* dq_tail = queue->dq_tail.load(std::memory_order_acquire); if (dq_tail != nullptr) { diff --git a/absl/strings/internal/cordz_handle.h b/absl/strings/internal/cordz_handle.h index 93076cfd..5df53c78 100644 --- a/absl/strings/internal/cordz_handle.h +++ b/absl/strings/internal/cordz_handle.h @@ -40,9 +40,20 @@ class CordzHandle { bool is_snapshot() const { return is_snapshot_; } + // Returns true if this instance is safe to be deleted because it is either a + // snapshot, which is always safe to delete, or not included in the global + // delete queue and thus not included in any snapshot. + // Callers are responsible for making sure this instance can not be newly + // discovered by other threads. For example, CordzInfo instances first de-list + // themselves from the global CordzInfo list before determining if they are + // safe to be deleted directly. + // If SafeToDelete returns false, callers MUST use the Delete() method to + // safely queue CordzHandle instances for deletion. + bool SafeToDelete() const; + // Deletes the provided instance, or puts it on the delete queue to be deleted // once there are no more sample tokens (snapshot) instances potentially - // referencing the instance. `handle` may be null. + // referencing the instance. `handle` should not be null. static void Delete(CordzHandle* handle); // Returns the current entries in the delete queue in LIFO order. diff --git a/absl/strings/internal/cordz_handle_test.cc b/absl/strings/internal/cordz_handle_test.cc index c04240e8..4eefd72c 100644 --- a/absl/strings/internal/cordz_handle_test.cc +++ b/absl/strings/internal/cordz_handle_test.cc @@ -52,6 +52,7 @@ TEST(CordzHandleTest, CordzHandleCreateDelete) { bool deleted = false; auto* handle = new CordzHandleDeleteTracker(&deleted); EXPECT_FALSE(handle->is_snapshot()); + EXPECT_TRUE(handle->SafeToDelete()); EXPECT_THAT(DeleteQueue(), SizeIs(0)); CordzHandle::Delete(handle); @@ -62,6 +63,7 @@ TEST(CordzHandleTest, CordzHandleCreateDelete) { TEST(CordzHandleTest, CordzSnapshotCreateDelete) { auto* snapshot = new CordzSnapshot(); EXPECT_TRUE(snapshot->is_snapshot()); + EXPECT_TRUE(snapshot->SafeToDelete()); EXPECT_THAT(DeleteQueue(), ElementsAre(snapshot)); delete snapshot; EXPECT_THAT(DeleteQueue(), SizeIs(0)); @@ -71,10 +73,12 @@ TEST(CordzHandleTest, CordzHandleCreateDeleteWithSnapshot) { bool deleted = false; auto* snapshot = new CordzSnapshot(); auto* handle = new CordzHandleDeleteTracker(&deleted); + EXPECT_FALSE(handle->SafeToDelete()); CordzHandle::Delete(handle); EXPECT_THAT(DeleteQueue(), ElementsAre(handle, snapshot)); EXPECT_FALSE(deleted); + EXPECT_FALSE(handle->SafeToDelete()); delete snapshot; EXPECT_THAT(DeleteQueue(), SizeIs(0)); @@ -219,8 +223,8 @@ TEST(CordzHandleTest, MultiThreaded) { if (safe_to_inspect.size() > max_safe_to_inspect) { max_safe_to_inspect = safe_to_inspect.size(); } + CordzHandle::Delete(old_handle); } - CordzHandle::Delete(old_handle); } // Confirm that the test did *something*. This check will be satisfied as @@ -234,9 +238,10 @@ TEST(CordzHandleTest, MultiThreaded) { // Have each thread attempt to clean up everything. Some thread will be // the last to reach this cleanup code, and it will be guaranteed to clean // up everything because nothing remains to create new handles. - for (size_t i = 0; i < handles.size(); i++) { - CordzHandle* handle = handles[i].exchange(nullptr); - CordzHandle::Delete(handle); + for (auto& h : handles) { + if (CordzHandle* handle = h.exchange(nullptr)) { + CordzHandle::Delete(handle); + } } }); } diff --git a/absl/strings/internal/cordz_info.cc b/absl/strings/internal/cordz_info.cc index 0461ec46..0f657aaf 100644 --- a/absl/strings/internal/cordz_info.cc +++ b/absl/strings/internal/cordz_info.cc @@ -126,13 +126,6 @@ void CordzInfo::Track() { } void CordzInfo::Untrack() { - { - // TODO(b/117940323): change this to assuming ownership instead once all - // Cord logic is properly keeping `rep_` in sync with the Cord root rep. - absl::MutexLock lock(&mutex_); - rep_ = nullptr; - } - ODRCheck(); { SpinLockHolder l(&list_->mutex); @@ -154,6 +147,20 @@ void CordzInfo::Untrack() { list_->head.store(next, std::memory_order_release); } } + + // We can no longer be discovered: perform a fast path check if we are not + // listed on any delete queue, so we can directly delete this instance. + if (SafeToDelete()) { + UnsafeSetCordRep(nullptr); + delete this; + return; + } + + // We are likely part of a snapshot, extend the life of the CordRep + { + absl::MutexLock lock(&mutex_); + if (rep_) CordRep::Ref(rep_); + } CordzHandle::Delete(this); } diff --git a/absl/strings/internal/cordz_info.h b/absl/strings/internal/cordz_info.h index f7682cb3..1fe046ec 100644 --- a/absl/strings/internal/cordz_info.h +++ b/absl/strings/internal/cordz_info.h @@ -110,7 +110,7 @@ class ABSL_LOCKABLE CordzInfo : public CordzHandle { // Asserts that this CordzInfo instance is locked. void AssertHeld() ABSL_ASSERT_EXCLUSIVE_LOCK(mutex_); - // Updates the `rep' property of this instance. This methods is invoked by + // Updates the `rep` property of this instance. This methods is invoked by // Cord logic each time the root node of a sampled Cord changes, and before // the old root reference count is deleted. This guarantees that collection // code can always safely take a reference on the tracked cord. @@ -119,6 +119,11 @@ class ABSL_LOCKABLE CordzInfo : public CordzHandle { // Cord code is in a state where this can be proven true by the compiler. void SetCordRep(CordRep* rep); + // Returns the current `rep` property of this instance with a reference + // added, or null if this instance represents a cord that has since been + // deleted or untracked. + CordRep* RefCordRep() const ABSL_LOCKS_EXCLUDED(mutex_); + // Returns the current value of `rep_` for testing purposes only. CordRep* GetCordRepForTesting() const ABSL_NO_THREAD_SAFETY_ANALYSIS { return rep_; @@ -148,6 +153,9 @@ class ABSL_LOCKABLE CordzInfo : public CordzHandle { } private: + using SpinLock = absl::base_internal::SpinLock; + using SpinLockHolder = ::absl::base_internal::SpinLockHolder; + // Global cordz info list. CordzInfo stores a pointer to the global list // instance to harden against ODR violations. struct List { @@ -155,7 +163,7 @@ class ABSL_LOCKABLE CordzInfo : public CordzHandle { : mutex(absl::kConstInit, absl::base_internal::SCHEDULE_COOPERATIVE_AND_KERNEL) {} - absl::base_internal::SpinLock mutex; + SpinLock mutex; std::atomic<CordzInfo*> head ABSL_GUARDED_BY(mutex){nullptr}; }; @@ -165,6 +173,9 @@ class ABSL_LOCKABLE CordzInfo : public CordzHandle { MethodIdentifier method); ~CordzInfo() override; + // Sets `rep_` without holding a lock. + void UnsafeSetCordRep(CordRep* rep) ABSL_NO_THREAD_SAFETY_ANALYSIS; + void Track(); // Returns the parent method from `src`, which is either `parent_method_` or @@ -244,6 +255,13 @@ inline void CordzInfo::SetCordRep(CordRep* rep) { } } +inline void CordzInfo::UnsafeSetCordRep(CordRep* rep) { rep_ = rep; } + +inline CordRep* CordzInfo::RefCordRep() const ABSL_LOCKS_EXCLUDED(mutex_) { + MutexLock lock(&mutex_); + return rep_ ? CordRep::Ref(rep_) : nullptr; +} + } // namespace cord_internal ABSL_NAMESPACE_END } // namespace absl diff --git a/absl/strings/internal/cordz_info_test.cc b/absl/strings/internal/cordz_info_test.cc index 5eaf4b9c..5bb23e47 100644 --- a/absl/strings/internal/cordz_info_test.cc +++ b/absl/strings/internal/cordz_info_test.cc @@ -38,6 +38,7 @@ using ::testing::ElementsAre; using ::testing::Eq; using ::testing::HasSubstr; using ::testing::Ne; +using ::testing::SizeIs; // Used test values auto constexpr kUnknownMethod = CordzUpdateTracker::kUnknown; @@ -78,10 +79,19 @@ TEST(CordzInfoTest, UntrackCord) { CordzInfo::TrackCord(data.data, kTrackCordMethod); CordzInfo* info = data.data.cordz_info(); + info->Untrack(); + EXPECT_THAT(DeleteQueue(), SizeIs(0)); +} + +TEST(CordzInfoTest, UntrackCordWithSnapshot) { + TestCordData data; + CordzInfo::TrackCord(data.data, kTrackCordMethod); + CordzInfo* info = data.data.cordz_info(); + CordzSnapshot snapshot; info->Untrack(); EXPECT_THAT(CordzInfo::Head(CordzSnapshot()), Eq(nullptr)); - EXPECT_THAT(info->GetCordRepForTesting(), Eq(nullptr)); + EXPECT_THAT(info->GetCordRepForTesting(), Eq(data.rep.rep)); EXPECT_THAT(DeleteQueue(), ElementsAre(info, &snapshot)); } @@ -113,6 +123,18 @@ TEST(CordzInfoTest, SetCordRepNullUntracksCordOnUnlock) { EXPECT_THAT(CordzInfo::Head(CordzSnapshot()), Eq(nullptr)); } +TEST(CordzInfoTest, RefCordRep) { + TestCordData data; + CordzInfo::TrackCord(data.data, kTrackCordMethod); + CordzInfo* info = data.data.cordz_info(); + + size_t refcount = data.rep.rep->refcount.Get(); + EXPECT_THAT(info->RefCordRep(), Eq(data.rep.rep)); + EXPECT_THAT(data.rep.rep->refcount.Get(), Eq(refcount + 1)); + CordRep::Unref(data.rep.rep); + info->Untrack(); +} + #if GTEST_HAS_DEATH_TEST TEST(CordzInfoTest, SetCordRepRequiresMutex) { diff --git a/absl/strings/internal/cordz_update_tracker.h b/absl/strings/internal/cordz_update_tracker.h index 741950b2..d5b14067 100644 --- a/absl/strings/internal/cordz_update_tracker.h +++ b/absl/strings/internal/cordz_update_tracker.h @@ -47,8 +47,10 @@ class CordzUpdateTracker { kClear, kConstructorCord, kConstructorString, + kCordReader, kFlatten, kGetAppendRegion, + kMakeCordFromExternal, kMoveAppendCord, kMoveAssignCord, kMovePrependCord, diff --git a/absl/strings/internal/cordz_update_tracker_test.cc b/absl/strings/internal/cordz_update_tracker_test.cc index eda662f2..fcd17df7 100644 --- a/absl/strings/internal/cordz_update_tracker_test.cc +++ b/absl/strings/internal/cordz_update_tracker_test.cc @@ -45,8 +45,10 @@ Methods AllMethods() { Method::kClear, Method::kConstructorCord, Method::kConstructorString, + Method::kCordReader, Method::kFlatten, Method::kGetAppendRegion, + Method::kMakeCordFromExternal, Method::kMoveAppendCord, Method::kMoveAssignCord, Method::kMovePrependCord, |