Export of internal Abseil changes
--
60b8e77be4bab1bbd3b4c3b70054879229634511 by Derek Mauro <dmauro@google.com>:
Use _MSVC_LANG for some C++ dialect checks since MSVC doesn't
set __cplusplus accurately by default.
https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/
See GitHub #722.
PiperOrigin-RevId: 371362181
--
5d736accdff04db0e722f377c0d79f2d3ed53263 by Martijn Vels <mvels@google.com>:
Fix the estimated memory size for CordRepExternal
PiperOrigin-RevId: 371350380
--
eaaa1d8a167aeca67a2aa3a098a2b61a9d72172f by Martijn Vels <mvels@google.com>:
Remove flakes by not enforcing re-allocated pointers do never match original
Tests that do multiple updates could end up with the original allocated pointer on a 2nd resize, so the 'EqIfPrivate' should not assume that if we do 'not' have the capacity that all following relocations will never match the original. We only care about 'pointer unchanged if private and there is capacity', trying to establish 'pointer changed at some point due to re-allocation; is pointless.
PiperOrigin-RevId: 371338965
--
d1837bee6bade1902b095c1cbf64231668bb84c5 by Martijn Vels <mvels@google.com>:
Undo inline of small data copy in cord
This leads to a performance regression as the code is not inlined (absent hard FDO inputs), and there are no suitable tail call options.
PiperOrigin-RevId: 371332332
--
06dc64b833069efc7d18b11df607c8c22be690da by Martijn Vels <mvels@google.com>:
Add final instrumentation for Cordz and remove 'old' cordz logic.
This change instruments the last cord function for cordz. It removes the 'old' functions: set_tree, replace_tree, UpdateCordzStatistics and RecordMetrics.
PiperOrigin-RevId: 371219909
--
a5e0be538579c603052feec03e6d9910c43ea787 by Martijn Vels <mvels@google.com>:
Extend the life of CordRep* if inside a snapshot
If a snapshot (potentially) includes the current CordzInfo, we need to extent the lifetime of the CordRep*, as the snapshot 'point in time' observation of the cord should ideally be preserved.
PiperOrigin-RevId: 371146151
--
74d77a89774cd6c8ecdeebee0193b294a39383d6 by Martijn Vels <mvels@google.com>:
Instrument std::string consuming methods: ctor, operator=, Append and Prepend
This change moves the 'steal into CordRep' logic into a separate function so we can use it directly in the ctor, operator assign and append and prepend, allowing Cordz instrumentation with the proper method attributes.
The assign operator is implemented in AssignLargeString leaving the dispatch inlined in cord.h (which as a side effects also allows clean tail calls in the AssignLargeString method)
PiperOrigin-RevId: 371094756
--
b39effc45266b7ce2e7f96caa3b16cb6e3acc2dd by Martijn Vels <mvels@google.com>:
Add Cordz instrumentation to CordReader
PiperOrigin-RevId: 370990181
GitOrigin-RevId: 60b8e77be4bab1bbd3b4c3b70054879229634511
Change-Id: I96af62e6f1a643e8b1228ae01e6c84e33706bb05
diff --git a/absl/memory/memory.h b/absl/memory/memory.h
index 2b5ff62..d633260 100644
--- a/absl/memory/memory.h
+++ b/absl/memory/memory.h
@@ -420,7 +420,7 @@
//
// 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 b5427a4..e7c1239 100644
--- a/absl/meta/type_traits.h
+++ b/absl/meta/type_traits.h
@@ -634,7 +634,7 @@
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 ae88f7b..15277de 100644
--- a/absl/strings/BUILD.bazel
+++ b/absl/strings/BUILD.bazel
@@ -529,6 +529,7 @@
":cord",
":cord_internal",
":strings",
+ "//absl/base:config",
],
)
diff --git a/absl/strings/CMakeLists.txt b/absl/strings/CMakeLists.txt
index 8ebe91e..1750f7a 100644
--- a/absl/strings/CMakeLists.txt
+++ b/absl/strings/CMakeLists.txt
@@ -830,6 +830,7 @@
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 9262aee..1c2ff9f 100644
--- a/absl/strings/cord.cc
+++ b/absl/strings/cord.cc
@@ -56,6 +56,7 @@
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 @@
}
}
+// 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 @@
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 @@
// --------------------------------------------------------------------
// 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 @@
}
}
+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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
}
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 @@
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 @@
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 @@
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 @@
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 @@
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 d7b4324..8abc474 100644
--- a/absl/strings/cord.h
+++ b/absl/strings/cord.h
@@ -678,6 +678,10 @@
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 @@
// 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 @@
// 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 @@
// 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 @@
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 @@
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::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 @@
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 @@
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 @@
}
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 2943cf3..cc8fbaf 100644
--- a/absl/strings/cord_ring_test.cc
+++ b/absl/strings/cord_ring_test.cc
@@ -98,15 +98,22 @@
// 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 74a9086..14eca15 100644
--- a/absl/strings/cord_test.cc
+++ b/absl/strings/cord_test.cc
@@ -190,14 +190,15 @@
}
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 6ccccc5..31a1dc8 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 @@
// 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 @@
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 84947cf..aeb3d13 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 @@
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 @@
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 @@
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 @@
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 5297ec8..a73fefe 100644
--- a/absl/strings/internal/cordz_handle.cc
+++ b/absl/strings/internal/cordz_handle.cc
@@ -68,11 +68,16 @@
}
}
+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 93076cf..5df53c7 100644
--- a/absl/strings/internal/cordz_handle.h
+++ b/absl/strings/internal/cordz_handle.h
@@ -40,9 +40,20 @@
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 c04240e..4eefd72 100644
--- a/absl/strings/internal/cordz_handle_test.cc
+++ b/absl/strings/internal/cordz_handle_test.cc
@@ -52,6 +52,7 @@
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, 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 @@
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 @@
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 @@
// 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 0461ec4..0f657aa 100644
--- a/absl/strings/internal/cordz_info.cc
+++ b/absl/strings/internal/cordz_info.cc
@@ -126,13 +126,6 @@
}
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 @@
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 f7682cb..1fe046e 100644
--- a/absl/strings/internal/cordz_info.h
+++ b/absl/strings/internal/cordz_info.h
@@ -110,7 +110,7 @@
// 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 @@
// 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 @@
}
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 @@
: 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 @@
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::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 5eaf4b9..5bb23e4 100644
--- a/absl/strings/internal/cordz_info_test.cc
+++ b/absl/strings/internal/cordz_info_test.cc
@@ -38,6 +38,7 @@
using ::testing::Eq;
using ::testing::HasSubstr;
using ::testing::Ne;
+using ::testing::SizeIs;
// Used test values
auto constexpr kUnknownMethod = CordzUpdateTracker::kUnknown;
@@ -78,10 +79,19 @@
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 @@
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 741950b..d5b1406 100644
--- a/absl/strings/internal/cordz_update_tracker.h
+++ b/absl/strings/internal/cordz_update_tracker.h
@@ -47,8 +47,10 @@
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 eda662f..fcd17df 100644
--- a/absl/strings/internal/cordz_update_tracker_test.cc
+++ b/absl/strings/internal/cordz_update_tracker_test.cc
@@ -45,8 +45,10 @@
Method::kClear,
Method::kConstructorCord,
Method::kConstructorString,
+ Method::kCordReader,
Method::kFlatten,
Method::kGetAppendRegion,
+ Method::kMakeCordFromExternal,
Method::kMoveAppendCord,
Method::kMoveAssignCord,
Method::kMovePrependCord,
