diff --git a/absl/base/attributes.h b/absl/base/attributes.h index d710f287..52139556 100644 --- a/absl/base/attributes.h +++ b/absl/base/attributes.h @@ -599,31 +599,24 @@ // case 42: // ... // -// Notes: when compiled with clang in C++11 mode, the ABSL_FALLTHROUGH_INTENDED -// macro is expanded to the [[clang::fallthrough]] attribute, which is analysed -// when performing switch labels fall-through diagnostic -// (`-Wimplicit-fallthrough`). See clang documentation on language extensions -// for details: +// Notes: When supported, GCC and Clang can issue a warning on switch labels +// with unannotated fallthrough using the warning `-Wimplicit-fallthrough`. See +// clang documentation on language extensions for details: // https://clang.llvm.org/docs/AttributeReference.html#fallthrough-clang-fallthrough // -// When used with unsupported compilers, the ABSL_FALLTHROUGH_INTENDED macro -// has no effect on diagnostics. In any case this macro has no effect on runtime +// When used with unsupported compilers, the ABSL_FALLTHROUGH_INTENDED macro has +// no effect on diagnostics. In any case this macro has no effect on runtime // behavior and performance of code. #ifdef ABSL_FALLTHROUGH_INTENDED #error "ABSL_FALLTHROUGH_INTENDED should not be defined." -#endif - -// TODO(zhangxy): Use c++17 standard [[fallthrough]] macro, when supported. -#if defined(__clang__) && defined(__has_warning) -#if __has_feature(cxx_attributes) && __has_warning("-Wimplicit-fallthrough") +#elif ABSL_HAVE_CPP_ATTRIBUTE(fallthrough) +#define ABSL_FALLTHROUGH_INTENDED [[fallthrough]] +#elif ABSL_HAVE_CPP_ATTRIBUTE(clang::fallthrough) #define ABSL_FALLTHROUGH_INTENDED [[clang::fallthrough]] -#endif -#elif ABSL_INTERNAL_HAVE_MIN_GNUC_VERSION(7, 0) +#elif ABSL_HAVE_CPP_ATTRIBUTE(gnu::fallthrough) #define ABSL_FALLTHROUGH_INTENDED [[gnu::fallthrough]] -#endif - -#ifndef ABSL_FALLTHROUGH_INTENDED +#else #define ABSL_FALLTHROUGH_INTENDED \ do { \ } while (0) diff --git a/absl/strings/BUILD.bazel b/absl/strings/BUILD.bazel index 68c71f0a..ae88f7bb 100644 --- a/absl/strings/BUILD.bazel +++ b/absl/strings/BUILD.bazel @@ -544,6 +544,7 @@ cc_library( ":cordz_sample_token", ":cordz_statistics", ":cordz_update_tracker", + ":strings", "//absl/base:config", "//absl/base:core_headers", "@com_google_googletest//:gtest", diff --git a/absl/strings/CMakeLists.txt b/absl/strings/CMakeLists.txt index 80ae2a60..8ebe91e6 100644 --- a/absl/strings/CMakeLists.txt +++ b/absl/strings/CMakeLists.txt @@ -852,6 +852,7 @@ absl_cc_library( absl::cordz_statistics absl::cordz_update_tracker absl::core_headers + absl::strings TESTONLY ) diff --git a/absl/strings/cord.cc b/absl/strings/cord.cc index 15d17337..9262aee6 100644 --- a/absl/strings/cord.cc +++ b/absl/strings/cord.cc @@ -584,26 +584,35 @@ void Cord::Clear() { } Cord& Cord::operator=(absl::string_view src) { + auto constexpr method = CordzUpdateTracker::kAssignString; const char* data = src.data(); size_t length = src.size(); CordRep* tree = contents_.tree(); if (length <= InlineRep::kMaxInline) { - // Embed into this->contents_ - if (tree) CordzInfo::MaybeUntrackCord(contents_.cordz_info()); + // Embed into this->contents_, which is somewhat subtle: + // - MaybeUntrackCord must be called before Unref(tree). + // - MaybeUntrackCord must be called before set_data() clobbers cordz_info. + // - set_data() must be called before Unref(tree) as it may reference tree. + if (tree != nullptr) CordzInfo::MaybeUntrackCord(contents_.cordz_info()); contents_.set_data(data, length, true); - if (tree) CordRep::Unref(tree); + if (tree != nullptr) CordRep::Unref(tree); return *this; } - if (tree != nullptr && tree->tag >= FLAT && - tree->flat()->Capacity() >= length && tree->refcount.IsOne()) { - // Copy in place if the existing FLAT node is reusable. - memmove(tree->flat()->Data(), data, length); - tree->length = length; - VerifyTree(tree); - return *this; + if (tree != nullptr) { + CordzUpdateScope scope(contents_.cordz_info(), method); + if (tree->tag >= FLAT && tree->flat()->Capacity() >= length && + tree->refcount.IsOne()) { + // Copy in place if the existing FLAT node is reusable. + memmove(tree->flat()->Data(), data, length); + tree->length = length; + VerifyTree(tree); + return *this; + } + contents_.SetTree(NewTree(data, length, 0), scope); + CordRep::Unref(tree); + } else { + contents_.EmplaceTree(NewTree(data, length, 0), method); } - contents_.set_tree(NewTree(data, length, 0)); - if (tree) CordRep::Unref(tree); return *this; } @@ -893,12 +902,17 @@ void Cord::RemovePrefix(size_t n) { CordRep* tree = contents_.tree(); if (tree == nullptr) { contents_.remove_prefix(n); - } else if (tree->tag == RING) { - contents_.replace_tree(CordRepRing::RemovePrefix(tree->ring(), n)); } else { - CordRep* newrep = RemovePrefixFrom(tree, n); - CordRep::Unref(tree); - contents_.replace_tree(VerifyTree(newrep)); + auto constexpr method = CordzUpdateTracker::kRemovePrefix; + CordzUpdateScope scope(contents_.cordz_info(), method); + if (tree->tag == RING) { + tree = CordRepRing::RemovePrefix(tree->ring(), n); + } else { + CordRep* newrep = RemovePrefixFrom(tree, n); + CordRep::Unref(tree); + tree = VerifyTree(newrep); + } + contents_.SetTreeOrEmpty(tree, scope); } } @@ -909,12 +923,17 @@ void Cord::RemoveSuffix(size_t n) { CordRep* tree = contents_.tree(); if (tree == nullptr) { contents_.reduce_size(n); - } else if (tree->tag == RING) { - contents_.replace_tree(CordRepRing::RemoveSuffix(tree->ring(), n)); } else { - CordRep* newrep = RemoveSuffixFrom(tree, n); - CordRep::Unref(tree); - contents_.replace_tree(VerifyTree(newrep)); + auto constexpr method = CordzUpdateTracker::kRemoveSuffix; + CordzUpdateScope scope(contents_.cordz_info(), method); + if (tree->tag == RING) { + tree = CordRepRing::RemoveSuffix(tree->ring(), n); + } else { + CordRep* newrep = RemoveSuffixFrom(tree, n); + CordRep::Unref(tree); + tree = VerifyTree(newrep); + } + contents_.SetTreeOrEmpty(tree, scope); } } @@ -969,37 +988,51 @@ 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(); if (pos > length) pos = length; if (new_size > length - pos) new_size = length - pos; + if (new_size == 0) return sub_cord; + CordRep* tree = contents_.tree(); if (tree == nullptr) { // sub_cord is newly constructed, no need to re-zero-out the tail of // contents_ memory. sub_cord.contents_.set_data(contents_.data() + pos, new_size, false); - } else if (new_size == 0) { - // We want to return empty subcord, so nothing to do. - } else if (new_size <= InlineRep::kMaxInline) { - Cord::ChunkIterator it = chunk_begin(); - it.AdvanceBytes(pos); - char* dest = sub_cord.contents_.data_.as_chars(); - 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); + return sub_cord; + } + + if (new_size <= InlineRep::kMaxInline) { + CopyDataAtPosition(pos, new_size, sub_cord.contents_.data_.as_chars()); sub_cord.contents_.set_inline_size(new_size); - } else if (tree->tag == RING) { - tree = CordRepRing::SubRing(CordRep::Ref(tree)->ring(), pos, new_size); - sub_cord.contents_.set_tree(tree); + return sub_cord; + } + + if (tree->tag == RING) { + CordRepRing* ring = CordRep::Ref(tree)->ring(); + tree = CordRepRing::SubRing(ring, pos, new_size); } else { - sub_cord.contents_.set_tree(NewSubRange(tree, pos, new_size)); + tree = NewSubRange(tree, pos, new_size); } + sub_cord.contents_.EmplaceTree(tree, contents_.data_, + CordzUpdateTracker::kSubCord); return sub_cord; } @@ -1676,6 +1709,7 @@ char Cord::operator[](size_t i) const { } absl::string_view Cord::FlattenSlowPath() { + assert(contents_.is_tree()); size_t total_size = size(); CordRep* new_rep; char* new_buffer; @@ -1696,10 +1730,9 @@ absl::string_view Cord::FlattenSlowPath() { s.size()); }); } - if (CordRep* tree = contents_.tree()) { - CordRep::Unref(tree); - } - contents_.set_tree(new_rep); + CordzUpdateScope scope(contents_.cordz_info(), CordzUpdateTracker::kFlatten); + CordRep::Unref(contents_.as_tree()); + contents_.SetTree(new_rep, scope); return absl::string_view(new_buffer, total_size); } diff --git a/absl/strings/cord.h b/absl/strings/cord.h index d5a13b34..d7b43247 100644 --- a/absl/strings/cord.h +++ b/absl/strings/cord.h @@ -692,6 +692,10 @@ 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. @@ -745,12 +749,21 @@ class Cord { // the CordzInfo instance is updated to reference the new `rep` value. void SetTree(CordRep* rep, const CordzUpdateScope& scope); + // Identical to SetTree(), except that `rep` is allowed to be null, in + // which case the current instance is reset to an empty value. + void SetTreeOrEmpty(CordRep* rep, const CordzUpdateScope& scope); + // Sets the tree value for this instance, and randomly samples this cord. // This function disregards existing contents in `data_`, and should be // called when a Cord is 'promoted' from an 'uninitialized' or 'inlined' // value to a non-inlined (tree / ring) value. void EmplaceTree(CordRep* rep, MethodIdentifier method); + // Identical to EmplaceTree, except that it copies the parent stack from + // the provided `parent` data if the parent is sampled. + void EmplaceTree(CordRep* rep, const InlineData& parent, + MethodIdentifier method); + // Commits the change of a newly created, or updated `rep` root value into // this cord. `old_rep` indicates the old (inlined or tree) value of the // cord, and determines if the commit invokes SetTree() or EmplaceTree(). @@ -1071,6 +1084,12 @@ inline void Cord::InlineRep::EmplaceTree(CordRep* rep, CordzInfo::MaybeTrackCord(data_, method); } +inline void Cord::InlineRep::EmplaceTree(CordRep* rep, const InlineData& parent, + MethodIdentifier method) { + data_.make_tree(rep); + CordzInfo::MaybeTrackCord(data_, parent, method); +} + inline void Cord::InlineRep::SetTree(CordRep* rep, const CordzUpdateScope& scope) { assert(rep); @@ -1079,6 +1098,17 @@ inline void Cord::InlineRep::SetTree(CordRep* rep, scope.SetCordRep(rep); } +inline void Cord::InlineRep::SetTreeOrEmpty(CordRep* rep, + const CordzUpdateScope& scope) { + assert(data_.is_tree()); + if (rep) { + data_.set_tree(rep); + } else { + data_ = {}; + } + scope.SetCordRep(rep); +} + inline void Cord::InlineRep::CommitTree(const CordRep* old_rep, CordRep* rep, const CordzUpdateScope& scope, MethodIdentifier method) { diff --git a/absl/strings/cordz_test.cc b/absl/strings/cordz_test.cc index b16e9686..84947cfd 100644 --- a/absl/strings/cordz_test.cc +++ b/absl/strings/cordz_test.cc @@ -52,6 +52,8 @@ inline void PrintTo(const Cord& cord, std::ostream* s) { namespace { +auto constexpr kMaxInline = cord_internal::kMaxInline; + // Returns a string_view value of the specified length // We do this to avoid 'consuming' large strings in Cord by default. absl::string_view MakeString(size_t size) { @@ -139,6 +141,16 @@ TEST(CordzTest, MoveAssignCord) { EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString)); } +TEST_P(CordzUpdateTest, AssignSmallArray) { + cord() = MakeString(TestCordSize::kSmall); + EXPECT_THAT(cord(), HasValidCordzInfoOf(Method::kAssignString)); +} + +TEST_P(CordzUpdateTest, AssignInlinedArray) { + cord() = MakeString(TestCordSize::kInlined); + EXPECT_THAT(GetCordzInfoForTesting(cord()), Eq(nullptr)); +} + TEST_P(CordzUpdateTest, AppendCord) { Cord src = UnsampledCord(MakeString(TestCordSize::kLarge)); cord().Append(src); @@ -166,6 +178,56 @@ TEST_P(CordzUpdateTest, AppendLargeArray) { EXPECT_THAT(cord(), HasValidCordzInfoOf(InitialOr(Method::kAppendString))); } +TEST(CordzTest, RemovePrefix) { + CordzSamplingIntervalHelper sample_every(1); + Cord cord(MakeString(TestCordSize::kLarge)); + + // Half the cord + cord.RemovePrefix(cord.size() / 2); + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString)); + EXPECT_THAT(cord, CordzMethodCountEq(Method::kRemovePrefix, 1)); + + // TODO(mvels): RemovePrefix does not reset to inlined, except if empty? + cord.RemovePrefix(cord.size() - kMaxInline); + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString)); + EXPECT_THAT(cord, CordzMethodCountEq(Method::kRemovePrefix, 2)); + + cord.RemovePrefix(cord.size()); + EXPECT_THAT(GetCordzInfoForTesting(cord), Eq(nullptr)); +} + +TEST(CordzTest, RemoveSuffix) { + CordzSamplingIntervalHelper sample_every(1); + Cord cord(MakeString(TestCordSize::kLarge)); + + // Half the cord + cord.RemoveSuffix(cord.size() / 2); + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString)); + EXPECT_THAT(cord, CordzMethodCountEq(Method::kRemoveSuffix, 1)); + + // TODO(mvels): RemoveSuffix does not reset to inlined, except if empty? + cord.RemoveSuffix(cord.size() - kMaxInline); + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString)); + EXPECT_THAT(cord, CordzMethodCountEq(Method::kRemoveSuffix, 2)); + + cord.RemoveSuffix(cord.size()); + EXPECT_THAT(GetCordzInfoForTesting(cord), Eq(nullptr)); +} + +TEST(CordzTest, SubCord) { + CordzSamplingIntervalHelper sample_every{1}; + Cord src(MakeString(TestCordSize::kLarge)); + + Cord cord1 = src.Subcord(10, src.size() / 2); + EXPECT_THAT(cord1, HasValidCordzInfoOf(Method::kSubCord)); + + Cord cord2 = src.Subcord(10, kMaxInline + 1); + EXPECT_THAT(cord2, HasValidCordzInfoOf(Method::kSubCord)); + + Cord cord3 = src.Subcord(10, kMaxInline); + EXPECT_THAT(GetCordzInfoForTesting(cord3), Eq(nullptr)); +} + } // namespace ABSL_NAMESPACE_END diff --git a/absl/strings/cordz_test_helpers.h b/absl/strings/cordz_test_helpers.h index d9573c97..e410eecf 100644 --- a/absl/strings/cordz_test_helpers.h +++ b/absl/strings/cordz_test_helpers.h @@ -27,6 +27,7 @@ #include "absl/strings/internal/cordz_sample_token.h" #include "absl/strings/internal/cordz_statistics.h" #include "absl/strings/internal/cordz_update_tracker.h" +#include "absl/strings/str_cat.h" namespace absl { ABSL_NAMESPACE_BEGIN @@ -34,7 +35,7 @@ ABSL_NAMESPACE_BEGIN // Returns the CordzInfo for the cord, or nullptr if the cord is not sampled. inline const cord_internal::CordzInfo* GetCordzInfoForTesting( const Cord& cord) { - if (cord.size() <= cord_internal::kMaxInline) return nullptr; + if (!cord.contents_.is_tree()) return nullptr; return cord.contents_.cordz_info(); } @@ -47,13 +48,11 @@ inline bool CordzInfoIsListed(const cord_internal::CordzInfo* cordz_info, return false; } -// Matcher on Cord* that verifies all of: +// Matcher on Cord that verifies all of: // - the cord is sampled // - the CordzInfo of the cord is listed / discoverable. // - the reported CordzStatistics match the cord's actual properties // - the cord has an (initial) UpdateTracker count of 1 for `method` -// This matcher accepts a const Cord* to avoid having the matcher dump -// copious amounts of cord data on failures. MATCHER_P(HasValidCordzInfoOf, method, "CordzInfo matches cord") { const cord_internal::CordzInfo* cord_info = GetCordzInfoForTesting(arg); if (cord_info == nullptr) { @@ -78,6 +77,24 @@ MATCHER_P(HasValidCordzInfoOf, method, "CordzInfo matches cord") { return true; } +// Matcher on Cord that verifies that the cord is sampled and that the CordzInfo +// update tracker has 'method' with a call count of 'n' +MATCHER_P2(CordzMethodCountEq, method, n, + absl::StrCat("CordzInfo method count equals ", n)) { + const cord_internal::CordzInfo* cord_info = GetCordzInfoForTesting(arg); + if (cord_info == nullptr) { + *result_listener << "cord is not sampled"; + return false; + } + cord_internal::CordzStatistics stat = cord_info->GetCordzStatistics(); + if (stat.update_tracker.Value(method) != n) { + *result_listener << "Expected method count " << n << " for " << method + << ", found " << stat.update_tracker.Value(method); + return false; + } + return true; +} + // Cordz will only update with a new rate once the previously scheduled event // has fired. When we disable Cordz, a long delay takes place where we won't // consider profiling new Cords. CordzSampleIntervalHelper will burn through