From e20fe888fabc1fc995dc61180e8a31b5f809a95f Mon Sep 17 00:00:00 2001 From: Abseil Team Date: Thu, 15 Apr 2021 19:42:51 -0700 Subject: Export of internal Abseil changes -- 341670bce317dd6af8d3c066970230591a47e80c by Martijn Vels : Change GetStack() and GetParentStack() to return absl::Span PiperOrigin-RevId: 368765721 -- 6aaab9536d6957303c7aba100c3afaa6fb0ea2c8 by Martijn Vels : Remove locking from parent stack. This change removes the need to lock all access to `parent_stack' by making the 'copy constructor' logic specify the 'copied from' CordzInfo (where available) to the TrackCord function, after which parent_stack is immutable. PiperOrigin-RevId: 368760630 -- b19e2059cada35a8ede994833018edac94de6ddc by Martijn Vels : Add cordz instrumentation to Cord PiperOrigin-RevId: 368746225 -- 67b8bbf980f0f4e1db79aa32968e9a715a09b51a by Martijn Vels : Create ABSL_INTERNAL_CORDZ_ENABLED define controlling when Cordz code is enabled There are specific builds and condtions under which we don't support cordz sampling, which is per this change represented by ABSL_INTERNAL_CORDZ_ENABLED being defined. PiperOrigin-RevId: 368731603 -- 8cbfe0e3169637a620f4b66ad2bc2ce340879cb0 by Martijn Vels : Add a `rep` property to CordzInfo to be managed by Cord logic. This change adds a `rep` property to CordzInfo, which is intended to be used by collection logic. Mini design: Cord invokes TrackCord() providing the active 'root' cordrep of the newly sampled Cord, returning a CordzInfo with a weak (uncounted) reference to this root. Cord invokes `SetCordRep()` each time the root cordrep of the sampled Cord is updated while holding `mutex()`. Cord must also obtain `mutex()` _before_ removing a reference on the old root. i.e.: Cord must guarantee that the (weak) reference held in CordzInfo is at all times valid. CordzInfo collection code can then safely obtain a (reference counted) rep pointer by adding a reference to `rep_` while holding `mutex()`. This requires only a very brief critical section inside CordzInfo logic, minimizing contention with concurrent Cord updates. Cord code should typically obtain and hold `mutex()` for the entirety of each mutating Cord operation on a sampled cord. As Cord is thread compatible, it never competes on the lock with any other thread. The only possible concurrent access is from Cordz collection code, which should be a relatively rare event. PiperOrigin-RevId: 368673758 -- 1255120dce2bdd6b4205a34a0e555e0b74b6152f by Martijn Vels : Remove 'depth' from active recorded metrics. Going forward we do not 'live' record depth (and size), but will observe these at collection time only. PiperOrigin-RevId: 368636572 -- 83e5146e35f221736b49e9f0a8805f8c159a51db by Martijn Vels : Make cordz targets visible in OSS PiperOrigin-RevId: 368615010 -- dcb16a4f1239151f0a8c70a8cfeb29dabbd113b8 by Martijn Vels : Internal cleanup PiperOrigin-RevId: 368514666 GitOrigin-RevId: 341670bce317dd6af8d3c066970230591a47e80c Change-Id: I94cecfbbd441eb386f99fc5186c468a7a5538862 --- absl/strings/internal/cordz_handle.h | 97 ++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 absl/strings/internal/cordz_handle.h (limited to 'absl/strings/internal/cordz_handle.h') diff --git a/absl/strings/internal/cordz_handle.h b/absl/strings/internal/cordz_handle.h new file mode 100644 index 00000000..0235d4bd --- /dev/null +++ b/absl/strings/internal/cordz_handle.h @@ -0,0 +1,97 @@ +// Copyright 2019 The Abseil Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef ABSL_STRINGS_CORDZ_HANDLE_H_ +#define ABSL_STRINGS_CORDZ_HANDLE_H_ + +#include +#include + +#include "absl/base/config.h" +#include "absl/synchronization/mutex.h" + +namespace absl { +ABSL_NAMESPACE_BEGIN +namespace cord_internal { + +// This base class allows multiple types of object (CordzInfo and +// CordzSampleToken) to exist simultaneously on the delete queue (pointed to by +// global_dq_tail and traversed using dq_prev_ and dq_next_). The +// delete queue guarantees that once a profiler creates a CordzSampleToken and +// has gained visibility into a CordzInfo object, that CordzInfo object will not +// be deleted prematurely. This allows the profiler to inspect all CordzInfo +// objects that are alive without needing to hold a global lock. +class CordzHandle { + public: + CordzHandle() : CordzHandle(false) {} + + bool is_snapshot() const { return is_snapshot_; } + + // 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. + static void Delete(CordzHandle* handle); + + // Returns the current entries in the delete queue in LIFO order. + static std::vector DiagnosticsGetDeleteQueue(); + + // Returns true if the provided handle is nullptr or guarded by this handle. + // Since the CordzSnapshot token is itself a CordzHandle, this method will + // allow tests to check if that token is keeping an arbitrary CordzHandle + // alive. + bool DiagnosticsHandleIsSafeToInspect(const CordzHandle* handle) const; + + // Returns the current entries in the delete queue, in LIFO order, that are + // protected by this. CordzHandle objects are only placed on the delete queue + // after CordzHandle::Delete is called with them as an argument. Only + // CordzHandle objects that are not also CordzSnapshot objects will be + // included in the return vector. For each of the handles in the return + // vector, the earliest that their memory can be freed is when this + // CordzSnapshot object is deleted. + std::vector DiagnosticsGetSafeToInspectDeletedHandles(); + + protected: + explicit CordzHandle(bool is_snapshot); + virtual ~CordzHandle(); + + private: + // Returns true if the delete queue is empty. This method does not acquire the + // lock, but does a 'load acquire' observation on the delete queue tail. It + // is used inside Delete() to check for the presence of a delete queue without + // holding the lock. The assumption is that the caller is in the state of + // 'being deleted', and can not be newly discovered by a concurrent 'being + // constructed' snapshot instance. Practically, this means that any such + // discovery (`find`, 'first' or 'next', etc) must have proper 'happens before + // / after' semantics and atomic fences. + static bool UnsafeDeleteQueueEmpty() ABSL_NO_THREAD_SAFETY_ANALYSIS { + return dq_tail_.load(std::memory_order_acquire) == nullptr; + } + + const bool is_snapshot_; + static absl::Mutex mutex_; + static std::atomic dq_tail_ ABSL_GUARDED_BY(mutex_); + CordzHandle* dq_prev_ ABSL_GUARDED_BY(mutex_) = nullptr; + CordzHandle* dq_next_ ABSL_GUARDED_BY(mutex_) = nullptr; +}; + +class CordzSnapshot : public CordzHandle { + public: + CordzSnapshot() : CordzHandle(true) {} +}; + +} // namespace cord_internal +ABSL_NAMESPACE_END +} // namespace absl + +#endif // ABSL_STRINGS_CORDZ_HANDLE_H_ -- cgit v1.2.3 From 732c6540c19610d2653ce73c09eb6cb66da15f42 Mon Sep 17 00:00:00 2001 From: Abseil Team Date: Fri, 16 Apr 2021 11:25:33 -0700 Subject: Export of internal Abseil changes -- c7ce91501834b225bc9cf2d3fa2a319dd0b7f864 by Martijn Vels : Implement global data for CordzHandle in an ODR hardened way This change puts the global data into a global delete queue structure, and stores a reference to the global data in the handle itself. This hardens the implementation against ODR violations where handles are crossing dynamic library boundaries which are privately loaded. PiperOrigin-RevId: 368885636 GitOrigin-RevId: c7ce91501834b225bc9cf2d3fa2a319dd0b7f864 Change-Id: I9775151a760b30989dec9517e4bcd2183e8c1651 --- absl/strings/CMakeLists.txt | 1 + absl/strings/internal/cordz_handle.cc | 36 ++++++++++++++------------ absl/strings/internal/cordz_handle.h | 48 +++++++++++++++++++++++++---------- 3 files changed, 55 insertions(+), 30 deletions(-) (limited to 'absl/strings/internal/cordz_handle.h') diff --git a/absl/strings/CMakeLists.txt b/absl/strings/CMakeLists.txt index 044e38b3..8f41f064 100644 --- a/absl/strings/CMakeLists.txt +++ b/absl/strings/CMakeLists.txt @@ -655,6 +655,7 @@ absl_cc_library( ${ABSL_DEFAULT_COPTS} DEPS absl::config + absl::raw_logging_internal absl::synchronization ) diff --git a/absl/strings/internal/cordz_handle.cc b/absl/strings/internal/cordz_handle.cc index 7e82f85b..22d07978 100644 --- a/absl/strings/internal/cordz_handle.cc +++ b/absl/strings/internal/cordz_handle.cc @@ -21,26 +21,26 @@ namespace absl { ABSL_NAMESPACE_BEGIN namespace cord_internal { -ABSL_CONST_INIT absl::Mutex CordzHandle::mutex_(absl::kConstInit); -ABSL_CONST_INIT std::atomic CordzHandle::dq_tail_{nullptr}; +ABSL_CONST_INIT CordzHandle::Queue CordzHandle::global_queue_(absl::kConstInit); CordzHandle::CordzHandle(bool is_snapshot) : is_snapshot_(is_snapshot) { if (is_snapshot) { - MutexLock lock(&mutex_); - CordzHandle* dq_tail = dq_tail_.load(std::memory_order_acquire); + MutexLock lock(&queue_->mutex); + CordzHandle* dq_tail = queue_->dq_tail.load(std::memory_order_acquire); if (dq_tail != nullptr) { dq_prev_ = dq_tail; dq_tail->dq_next_ = this; } - dq_tail_.store(this, std::memory_order_release); + queue_->dq_tail.store(this, std::memory_order_release); } } CordzHandle::~CordzHandle() { + ODRCheck(); if (is_snapshot_) { std::vector to_delete; { - absl::MutexLock lock(&mutex_); + absl::MutexLock lock(&queue_->mutex); CordzHandle* next = dq_next_; if (dq_prev_ == nullptr) { // We were head of the queue, delete every CordzHandle until we reach @@ -56,7 +56,7 @@ CordzHandle::~CordzHandle() { if (next) { next->dq_prev_ = dq_prev_; } else { - dq_tail_.store(dq_prev_, std::memory_order_release); + queue_->dq_tail.store(dq_prev_, std::memory_order_release); } } for (CordzHandle* handle : to_delete) { @@ -67,13 +67,15 @@ CordzHandle::~CordzHandle() { void CordzHandle::Delete(CordzHandle* handle) { if (handle) { - if (!handle->is_snapshot_ && !UnsafeDeleteQueueEmpty()) { - MutexLock lock(&mutex_); - CordzHandle* dq_tail = dq_tail_.load(std::memory_order_acquire); + handle->ODRCheck(); + Queue* const queue = handle->queue_; + if (!handle->is_snapshot_ && !queue->IsEmpty()) { + MutexLock lock(&queue->mutex); + CordzHandle* dq_tail = queue->dq_tail.load(std::memory_order_acquire); if (dq_tail != nullptr) { handle->dq_prev_ = dq_tail; dq_tail->dq_next_ = handle; - dq_tail_.store(handle, std::memory_order_release); + queue->dq_tail.store(handle, std::memory_order_release); return; } } @@ -83,8 +85,8 @@ void CordzHandle::Delete(CordzHandle* handle) { std::vector CordzHandle::DiagnosticsGetDeleteQueue() { std::vector handles; - MutexLock lock(&mutex_); - CordzHandle* dq_tail = dq_tail_.load(std::memory_order_acquire); + MutexLock lock(&global_queue_.mutex); + CordzHandle* dq_tail = global_queue_.dq_tail.load(std::memory_order_acquire); for (const CordzHandle* p = dq_tail; p; p = p->dq_prev_) { handles.push_back(p); } @@ -93,12 +95,13 @@ std::vector CordzHandle::DiagnosticsGetDeleteQueue() { bool CordzHandle::DiagnosticsHandleIsSafeToInspect( const CordzHandle* handle) const { + ODRCheck(); if (!is_snapshot_) return false; if (handle == nullptr) return true; if (handle->is_snapshot_) return false; bool snapshot_found = false; - MutexLock lock(&mutex_); - for (const CordzHandle* p = dq_tail_; p; p = p->dq_prev_) { + MutexLock lock(&queue_->mutex); + for (const CordzHandle* p = queue_->dq_tail; p; p = p->dq_prev_) { if (p == handle) return !snapshot_found; if (p == this) snapshot_found = true; } @@ -108,12 +111,13 @@ bool CordzHandle::DiagnosticsHandleIsSafeToInspect( std::vector CordzHandle::DiagnosticsGetSafeToInspectDeletedHandles() { + ODRCheck(); std::vector handles; if (!is_snapshot()) { return handles; } - MutexLock lock(&mutex_); + MutexLock lock(&queue_->mutex); for (const CordzHandle* p = dq_next_; p != nullptr; p = p->dq_next_) { if (!p->is_snapshot()) { handles.push_back(p); diff --git a/absl/strings/internal/cordz_handle.h b/absl/strings/internal/cordz_handle.h index 0235d4bd..71563c8b 100644 --- a/absl/strings/internal/cordz_handle.h +++ b/absl/strings/internal/cordz_handle.h @@ -19,6 +19,7 @@ #include #include "absl/base/config.h" +#include "absl/base/internal/raw_logging.h" #include "absl/synchronization/mutex.h" namespace absl { @@ -66,23 +67,42 @@ class CordzHandle { virtual ~CordzHandle(); private: - // Returns true if the delete queue is empty. This method does not acquire the - // lock, but does a 'load acquire' observation on the delete queue tail. It - // is used inside Delete() to check for the presence of a delete queue without - // holding the lock. The assumption is that the caller is in the state of - // 'being deleted', and can not be newly discovered by a concurrent 'being - // constructed' snapshot instance. Practically, this means that any such - // discovery (`find`, 'first' or 'next', etc) must have proper 'happens before - // / after' semantics and atomic fences. - static bool UnsafeDeleteQueueEmpty() ABSL_NO_THREAD_SAFETY_ANALYSIS { - return dq_tail_.load(std::memory_order_acquire) == nullptr; + // Global queue data. CordzHandle stores a pointer to the global queue + // instance to harden against ODR violations. + struct Queue { + constexpr explicit Queue(absl::ConstInitType) : mutex(absl::kConstInit) {} + + absl::Mutex mutex; + std::atomic dq_tail ABSL_GUARDED_BY(mutex){nullptr}; + + // Returns true if this delete queue is empty. This method does not acquire + // the lock, but does a 'load acquire' observation on the delete queue tail. + // It is used inside Delete() to check for the presence of a delete queue + // without holding the lock. The assumption is that the caller is in the + // state of 'being deleted', and can not be newly discovered by a concurrent + // 'being constructed' snapshot instance. Practically, this means that any + // such discovery (`find`, 'first' or 'next', etc) must have proper 'happens + // before / after' semantics and atomic fences. + bool IsEmpty() const ABSL_NO_THREAD_SAFETY_ANALYSIS { + return dq_tail.load(std::memory_order_acquire) == nullptr; + } + }; + + void ODRCheck() const { +#ifndef NDEBUG + ABSL_RAW_CHECK(queue_ == &global_queue_, "ODR violation in Cord"); +#endif } + ABSL_CONST_INIT static Queue global_queue_; + Queue* const queue_ = &global_queue_; const bool is_snapshot_; - static absl::Mutex mutex_; - static std::atomic dq_tail_ ABSL_GUARDED_BY(mutex_); - CordzHandle* dq_prev_ ABSL_GUARDED_BY(mutex_) = nullptr; - CordzHandle* dq_next_ ABSL_GUARDED_BY(mutex_) = nullptr; + + // dq_prev_ and dq_next_ require the global queue mutex to be held. + // Unfortunately we can't use thread annotations such that the thread safety + // analysis understands that queue_ and global_queue_ are one and the same. + CordzHandle* dq_prev_ = nullptr; + CordzHandle* dq_next_ = nullptr; }; class CordzSnapshot : public CordzHandle { -- cgit v1.2.3 From a5167f986b82e9a7535ee710e87e53cf4c8ef2a8 Mon Sep 17 00:00:00 2001 From: Abseil Team Date: Tue, 27 Apr 2021 09:11:54 -0700 Subject: Export of internal Abseil changes -- f6fbb03bff276e72123e8590519079e87732ae62 by Abseil Team : Replace static absl::Mutex with SpinLock in absl::Cords to avoid static initializers by absl::Mutex destructors PiperOrigin-RevId: 370694199 -- 654b7d9edfdc24f226990b2b46cbf91451a1d92a by Martijn Vels : Implement global data for CordzInfo in an ODR hardened way This change puts the global data into a global list structure, and stores a reference to the global list in the handle itself. This hardens the implementation against ODR violations where info pointers are crossing dynamic library boundaries which are privately loaded. PiperOrigin-RevId: 370673045 -- 712dba768e66ee2ba85d6010829c617cd2af6ba7 by Martijn Vels : Intrument Cord::operator= for Cordz PiperOrigin-RevId: 370659149 -- c0b347a2289e151b72680269332e264b8fa989c0 by Matt Kulukundis : Fix test guards for ABSL_ATTRIBUTE_RETURNS_NONNULL PiperOrigin-RevId: 370594807 -- c2bedaa3472ef223f907de2604f9b9b58852ec5f by Martijn Vels : Add new Cordz instrumentation on GetAppendRegion. PiperOrigin-RevId: 370587761 -- 84fbfcc852697d509f6094482b86e84743a6b331 by Martijn Vels : Add instrumentation on Cord::Apppend(string_view) PiperOrigin-RevId: 370576590 -- 9e077390b8ca2239e1cb7bfbe1d5a04f2fc11d30 by Abseil Team : Google-internal changes only. PiperOrigin-RevId: 370558424 -- fb53c149eb2364ea34e3a67235f873866618b8ac by Matt Kulukundis : Update config.h macros with a few useful helpers to simplify version checking PiperOrigin-RevId: 370557684 -- abf8142e99b9ff7e15f6528a357f1005461950b0 by Martijn Vels : clang-format cord PiperOrigin-RevId: 370549371 -- e555985eabe63fcf0e980e9c433dd84caffec191 by Martijn Vels : Add MaybeUntrackCord() function This function is near identical to the old UntrackCord() but allows info to be null, moving the cord.is_profiled() branch into CordzInfo. PiperOrigin-RevId: 370528447 -- 3883538efe4601f7864bda70a50d868bb383c63b by Derek Mauro : Internal change PiperOrigin-RevId: 370503186 -- a9514b65542fde1bc73584e6f3c1c4b3a05f215f by Derek Mauro : Add -Winvalid-constexpr to warning options for LLVM PiperOrigin-RevId: 370455171 -- d8a3966de2cf15a2dc28e17e49a3d27d205eca92 by Martijn Vels : Add naive UniqueGenerator to avoid flakes from dup random values. PiperOrigin-RevId: 370179772 -- 46d0caa1a12b68a5998d4f919e20f0f83b9286f8 by Martijn Vels : Add new Cordz instrumentation on PrependTree. PiperOrigin-RevId: 370138969 GitOrigin-RevId: f6fbb03bff276e72123e8590519079e87732ae62 Change-Id: Ifa4c00a5c7b01198ee367a3253bea6b66612135e --- absl/base/attributes.h | 7 +- absl/base/config.h | 46 ++-- absl/container/internal/hash_generator_testing.h | 21 ++ .../internal/unordered_map_constructor_test.h | 38 +-- absl/copts/GENERATED_AbseilCopts.cmake | 1 + absl/copts/GENERATED_copts.bzl | 1 + absl/copts/copts.py | 1 + absl/copts/generate_copts.py | 2 +- absl/strings/BUILD.bazel | 5 + absl/strings/CMakeLists.txt | 5 + absl/strings/cord.cc | 295 +++++++++------------ absl/strings/cord.h | 54 +++- absl/strings/cord_test_helpers.h | 38 +++ absl/strings/cordz_test.cc | 130 +++++++-- absl/strings/cordz_test_helpers.h | 7 +- absl/strings/internal/cordz_handle.cc | 15 +- absl/strings/internal/cordz_handle.h | 7 +- absl/strings/internal/cordz_info.cc | 78 +++--- absl/strings/internal/cordz_info.h | 59 +++-- absl/strings/internal/cordz_info_test.cc | 30 +-- absl/strings/internal/cordz_sample_token_test.cc | 18 +- absl/strings/internal/cordz_update_scope.h | 8 +- absl/synchronization/internal/waiter.cc | 2 +- 23 files changed, 538 insertions(+), 330 deletions(-) (limited to 'absl/strings/internal/cordz_handle.h') diff --git a/absl/base/attributes.h b/absl/base/attributes.h index 294e5d0c..d710f287 100644 --- a/absl/base/attributes.h +++ b/absl/base/attributes.h @@ -281,10 +281,7 @@ // ABSL_ATTRIBUTE_RETURNS_NONNULL // // Tells the compiler that a particular function never returns a null pointer. -#if ABSL_HAVE_ATTRIBUTE(returns_nonnull) || \ - (defined(__GNUC__) && \ - (__GNUC__ > 5 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 9)) && \ - !defined(__clang__)) +#if ABSL_HAVE_ATTRIBUTE(returns_nonnull) #define ABSL_ATTRIBUTE_RETURNS_NONNULL __attribute__((returns_nonnull)) #else #define ABSL_ATTRIBUTE_RETURNS_NONNULL @@ -622,7 +619,7 @@ #if __has_feature(cxx_attributes) && __has_warning("-Wimplicit-fallthrough") #define ABSL_FALLTHROUGH_INTENDED [[clang::fallthrough]] #endif -#elif defined(__GNUC__) && __GNUC__ >= 7 +#elif ABSL_INTERNAL_HAVE_MIN_GNUC_VERSION(7, 0) #define ABSL_FALLTHROUGH_INTENDED [[gnu::fallthrough]] #endif diff --git a/absl/base/config.h b/absl/base/config.h index 7abc75a6..0524196d 100644 --- a/absl/base/config.h +++ b/absl/base/config.h @@ -166,6 +166,22 @@ static_assert(ABSL_INTERNAL_INLINE_NAMESPACE_STR[0] != 'h' || #define ABSL_HAVE_FEATURE(f) 0 #endif +// Portable check for GCC minimum version: +// https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html +#if defined(__GNUC__) && defined(__GNUC_MINOR__) +#define ABSL_INTERNAL_HAVE_MIN_GNUC_VERSION(x, y) \ + (__GNUC__ > (x) || __GNUC__ == (x) && __GNUC_MINOR__ >= (y)) +#else +#define ABSL_INTERNAL_HAVE_MIN_GNUC_VERSION(x, y) 0 +#endif + +#if defined(__clang__) && defined(__clang_major__) && defined(__clang_minor__) +#define ABSL_INTERNAL_HAVE_MIN_CLANG_VERSION(x, y) \ + (__clang_major__ > (x) || __clang_major__ == (x) && __clang_minor__ >= (y)) +#else +#define ABSL_INTERNAL_HAVE_MIN_CLANG_VERSION(x, y) 0 +#endif + // ABSL_HAVE_TLS is defined to 1 when __thread should be supported. // We assume __thread is supported on Linux when compiled with Clang or compiled // against libstdc++ with _GLIBCXX_HAVE_TLS defined. @@ -183,10 +199,9 @@ static_assert(ABSL_INTERNAL_INLINE_NAMESPACE_STR[0] != 'h' || // gcc >= 4.8.1 using libstdc++, and Visual Studio. #ifdef ABSL_HAVE_STD_IS_TRIVIALLY_DESTRUCTIBLE #error ABSL_HAVE_STD_IS_TRIVIALLY_DESTRUCTIBLE cannot be directly set -#elif defined(_LIBCPP_VERSION) || \ - (!defined(__clang__) && defined(__GNUC__) && defined(__GLIBCXX__) && \ - (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 8))) || \ - defined(_MSC_VER) +#elif defined(_LIBCPP_VERSION) || defined(_MSC_VER) || \ + (!defined(__clang__) && defined(__GLIBCXX__) && \ + ABSL_INTERNAL_HAVE_MIN_GNUC_VERSION(4, 8)) #define ABSL_HAVE_STD_IS_TRIVIALLY_DESTRUCTIBLE 1 #endif @@ -205,10 +220,9 @@ static_assert(ABSL_INTERNAL_INLINE_NAMESPACE_STR[0] != 'h' || #error ABSL_HAVE_STD_IS_TRIVIALLY_CONSTRUCTIBLE cannot be directly set #elif defined(ABSL_HAVE_STD_IS_TRIVIALLY_ASSIGNABLE) #error ABSL_HAVE_STD_IS_TRIVIALLY_ASSIGNABLE cannot directly set -#elif (defined(__clang__) && defined(_LIBCPP_VERSION)) || \ - (!defined(__clang__) && defined(__GNUC__) && \ - (__GNUC__ > 7 || (__GNUC__ == 7 && __GNUC_MINOR__ >= 4)) && \ - (defined(_LIBCPP_VERSION) || defined(__GLIBCXX__))) || \ +#elif (defined(__clang__) && defined(_LIBCPP_VERSION)) || \ + (!defined(__clang__) && ABSL_INTERNAL_HAVE_MIN_GNUC_VERSION(7, 4) && \ + (defined(_LIBCPP_VERSION) || defined(__GLIBCXX__))) || \ (defined(_MSC_VER) && !defined(__NVCC__)) #define ABSL_HAVE_STD_IS_TRIVIALLY_CONSTRUCTIBLE 1 #define ABSL_HAVE_STD_IS_TRIVIALLY_ASSIGNABLE 1 @@ -222,7 +236,7 @@ static_assert(ABSL_INTERNAL_INLINE_NAMESPACE_STR[0] != 'h' || #if ABSL_INTERNAL_HAS_KEYWORD(__builtin_LINE) && \ ABSL_INTERNAL_HAS_KEYWORD(__builtin_FILE) #define ABSL_HAVE_SOURCE_LOCATION_CURRENT 1 -#elif defined(__GNUC__) && __GNUC__ >= 5 +#elif ABSL_INTERNAL_HAVE_MIN_GNUC_VERSION(5, 0) #define ABSL_HAVE_SOURCE_LOCATION_CURRENT 1 #endif #endif @@ -319,25 +333,21 @@ static_assert(ABSL_INTERNAL_INLINE_NAMESPACE_STR[0] != 'h' || // For further details, consult the compiler's documentation. #ifdef ABSL_HAVE_EXCEPTIONS #error ABSL_HAVE_EXCEPTIONS cannot be directly set. - -#elif defined(__clang__) - -#if __clang_major__ > 3 || (__clang_major__ == 3 && __clang_minor__ >= 6) +#elif ABSL_INTERNAL_HAVE_MIN_CLANG_VERSION(3, 6) // Clang >= 3.6 #if ABSL_HAVE_FEATURE(cxx_exceptions) #define ABSL_HAVE_EXCEPTIONS 1 #endif // ABSL_HAVE_FEATURE(cxx_exceptions) -#else +#elif defined(__clang__) // Clang < 3.6 // http://releases.llvm.org/3.6.0/tools/clang/docs/ReleaseNotes.html#the-exceptions-macro #if defined(__EXCEPTIONS) && ABSL_HAVE_FEATURE(cxx_exceptions) #define ABSL_HAVE_EXCEPTIONS 1 #endif // defined(__EXCEPTIONS) && ABSL_HAVE_FEATURE(cxx_exceptions) -#endif // __clang_major__ > 3 || (__clang_major__ == 3 && __clang_minor__ >= 6) - // Handle remaining special cases and default to exceptions being supported. -#elif !(defined(__GNUC__) && (__GNUC__ < 5) && !defined(__EXCEPTIONS)) && \ - !(defined(__GNUC__) && (__GNUC__ >= 5) && !defined(__cpp_exceptions)) && \ +#elif !(defined(__GNUC__) && (__GNUC__ < 5) && !defined(__EXCEPTIONS)) && \ + !(ABSL_INTERNAL_HAVE_MIN_GNUC_VERSION(5, 0) && \ + !defined(__cpp_exceptions)) && \ !(defined(_MSC_VER) && !defined(_CPPUNWIND)) #define ABSL_HAVE_EXCEPTIONS 1 #endif diff --git a/absl/container/internal/hash_generator_testing.h b/absl/container/internal/hash_generator_testing.h index 6869fe45..f1f555a5 100644 --- a/absl/container/internal/hash_generator_testing.h +++ b/absl/container/internal/hash_generator_testing.h @@ -21,11 +21,13 @@ #include #include +#include #include #include #include #include #include +#include #include "absl/container/internal/hash_policy_testing.h" #include "absl/memory/memory.h" @@ -153,6 +155,25 @@ using GeneratedType = decltype( typename Container::value_type, typename Container::key_type>::type>&>()()); +// Naive wrapper that performs a linear search of previous values. +// Beware this is O(SQR), which is reasonable for smaller kMaxValues. +template +struct UniqueGenerator { + Generator gen; + std::vector values; + + T operator()() { + assert(values.size() < kMaxValues); + for (;;) { + T value = gen(); + if (std::find(values.begin(), values.end(), value) == values.end()) { + values.push_back(value); + return value; + } + } + } +}; + } // namespace hash_internal } // namespace container_internal ABSL_NAMESPACE_END diff --git a/absl/container/internal/unordered_map_constructor_test.h b/absl/container/internal/unordered_map_constructor_test.h index 3f90ad7c..c1d20f3c 100644 --- a/absl/container/internal/unordered_map_constructor_test.h +++ b/absl/container/internal/unordered_map_constructor_test.h @@ -179,7 +179,7 @@ TYPED_TEST_P(ConstructorTest, InputIteratorBucketHashEqualAlloc) { A alloc(0); std::vector values; std::generate_n(std::back_inserter(values), 10, - hash_internal::Generator()); + hash_internal::UniqueGenerator()); TypeParam m(values.begin(), values.end(), 123, hasher, equal, alloc); EXPECT_EQ(m.hash_function(), hasher); EXPECT_EQ(m.key_eq(), equal); @@ -198,7 +198,7 @@ void InputIteratorBucketAllocTest(std::true_type) { A alloc(0); std::vector values; std::generate_n(std::back_inserter(values), 10, - hash_internal::Generator()); + hash_internal::UniqueGenerator()); TypeParam m(values.begin(), values.end(), 123, alloc); EXPECT_EQ(m.get_allocator(), alloc); EXPECT_THAT(items(m), ::testing::UnorderedElementsAreArray(values)); @@ -221,7 +221,7 @@ void InputIteratorBucketHashAllocTest(std::true_type) { A alloc(0); std::vector values; std::generate_n(std::back_inserter(values), 10, - hash_internal::Generator()); + hash_internal::UniqueGenerator()); TypeParam m(values.begin(), values.end(), 123, hasher, alloc); EXPECT_EQ(m.hash_function(), hasher); EXPECT_EQ(m.get_allocator(), alloc); @@ -241,8 +241,9 @@ TYPED_TEST_P(ConstructorTest, CopyConstructor) { H hasher; E equal; A alloc(0); + hash_internal::UniqueGenerator gen; TypeParam m(123, hasher, equal, alloc); - for (size_t i = 0; i != 10; ++i) m.insert(hash_internal::Generator()()); + for (size_t i = 0; i != 10; ++i) m.insert(gen()); TypeParam n(m); EXPECT_EQ(m.hash_function(), n.hash_function()); EXPECT_EQ(m.key_eq(), n.key_eq()); @@ -262,8 +263,9 @@ void CopyConstructorAllocTest(std::true_type) { H hasher; E equal; A alloc(0); + hash_internal::UniqueGenerator gen; TypeParam m(123, hasher, equal, alloc); - for (size_t i = 0; i != 10; ++i) m.insert(hash_internal::Generator()()); + for (size_t i = 0; i != 10; ++i) m.insert(gen()); TypeParam n(m, A(11)); EXPECT_EQ(m.hash_function(), n.hash_function()); EXPECT_EQ(m.key_eq(), n.key_eq()); @@ -285,8 +287,9 @@ TYPED_TEST_P(ConstructorTest, MoveConstructor) { H hasher; E equal; A alloc(0); + hash_internal::UniqueGenerator gen; TypeParam m(123, hasher, equal, alloc); - for (size_t i = 0; i != 10; ++i) m.insert(hash_internal::Generator()()); + for (size_t i = 0; i != 10; ++i) m.insert(gen()); TypeParam t(m); TypeParam n(std::move(t)); EXPECT_EQ(m.hash_function(), n.hash_function()); @@ -307,8 +310,9 @@ void MoveConstructorAllocTest(std::true_type) { H hasher; E equal; A alloc(0); + hash_internal::UniqueGenerator gen; TypeParam m(123, hasher, equal, alloc); - for (size_t i = 0; i != 10; ++i) m.insert(hash_internal::Generator()()); + for (size_t i = 0; i != 10; ++i) m.insert(gen()); TypeParam t(m); TypeParam n(std::move(t), A(1)); EXPECT_EQ(m.hash_function(), n.hash_function()); @@ -325,7 +329,7 @@ TYPED_TEST_P(ConstructorTest, MoveConstructorAlloc) { TYPED_TEST_P(ConstructorTest, InitializerListBucketHashEqualAlloc) { using T = hash_internal::GeneratedType; - hash_internal::Generator gen; + hash_internal::UniqueGenerator gen; std::initializer_list values = {gen(), gen(), gen(), gen(), gen()}; using H = typename TypeParam::hasher; using E = typename TypeParam::key_equal; @@ -348,7 +352,7 @@ template void InitializerListBucketAllocTest(std::true_type) { using T = hash_internal::GeneratedType; using A = typename TypeParam::allocator_type; - hash_internal::Generator gen; + hash_internal::UniqueGenerator gen; std::initializer_list values = {gen(), gen(), gen(), gen(), gen()}; A alloc(0); TypeParam m(values, 123, alloc); @@ -371,7 +375,7 @@ void InitializerListBucketHashAllocTest(std::true_type) { using A = typename TypeParam::allocator_type; H hasher; A alloc(0); - hash_internal::Generator gen; + hash_internal::UniqueGenerator gen; std::initializer_list values = {gen(), gen(), gen(), gen(), gen()}; TypeParam m(values, 123, hasher, alloc); EXPECT_EQ(m.hash_function(), hasher); @@ -392,7 +396,7 @@ TYPED_TEST_P(ConstructorTest, Assignment) { H hasher; E equal; A alloc(0); - hash_internal::Generator gen; + hash_internal::UniqueGenerator gen; TypeParam m({gen(), gen(), gen()}, 123, hasher, equal, alloc); TypeParam n; n = m; @@ -412,7 +416,7 @@ TYPED_TEST_P(ConstructorTest, MoveAssignment) { H hasher; E equal; A alloc(0); - hash_internal::Generator gen; + hash_internal::UniqueGenerator gen; TypeParam m({gen(), gen(), gen()}, 123, hasher, equal, alloc); TypeParam t(m); TypeParam n; @@ -424,7 +428,7 @@ TYPED_TEST_P(ConstructorTest, MoveAssignment) { TYPED_TEST_P(ConstructorTest, AssignmentFromInitializerList) { using T = hash_internal::GeneratedType; - hash_internal::Generator gen; + hash_internal::UniqueGenerator gen; std::initializer_list values = {gen(), gen(), gen(), gen(), gen()}; TypeParam m; m = values; @@ -433,7 +437,7 @@ TYPED_TEST_P(ConstructorTest, AssignmentFromInitializerList) { TYPED_TEST_P(ConstructorTest, AssignmentOverwritesExisting) { using T = hash_internal::GeneratedType; - hash_internal::Generator gen; + hash_internal::UniqueGenerator gen; TypeParam m({gen(), gen(), gen()}); TypeParam n({gen()}); n = m; @@ -442,7 +446,7 @@ TYPED_TEST_P(ConstructorTest, AssignmentOverwritesExisting) { TYPED_TEST_P(ConstructorTest, MoveAssignmentOverwritesExisting) { using T = hash_internal::GeneratedType; - hash_internal::Generator gen; + hash_internal::UniqueGenerator gen; TypeParam m({gen(), gen(), gen()}); TypeParam t(m); TypeParam n({gen()}); @@ -452,7 +456,7 @@ TYPED_TEST_P(ConstructorTest, MoveAssignmentOverwritesExisting) { TYPED_TEST_P(ConstructorTest, AssignmentFromInitializerListOverwritesExisting) { using T = hash_internal::GeneratedType; - hash_internal::Generator gen; + hash_internal::UniqueGenerator gen; std::initializer_list values = {gen(), gen(), gen(), gen(), gen()}; TypeParam m; m = values; @@ -461,7 +465,7 @@ TYPED_TEST_P(ConstructorTest, AssignmentFromInitializerListOverwritesExisting) { TYPED_TEST_P(ConstructorTest, AssignmentOnSelf) { using T = hash_internal::GeneratedType; - hash_internal::Generator gen; + hash_internal::UniqueGenerator gen; std::initializer_list values = {gen(), gen(), gen(), gen(), gen()}; TypeParam m(values); m = *&m; // Avoid -Wself-assign diff --git a/absl/copts/GENERATED_AbseilCopts.cmake b/absl/copts/GENERATED_AbseilCopts.cmake index 51742c9b..18a1e5c3 100644 --- a/absl/copts/GENERATED_AbseilCopts.cmake +++ b/absl/copts/GENERATED_AbseilCopts.cmake @@ -71,6 +71,7 @@ list(APPEND ABSL_LLVM_FLAGS "-Wformat-security" "-Wgnu-redeclared-enum" "-Winfinite-recursion" + "-Winvalid-constexpr" "-Wliteral-conversion" "-Wmissing-declarations" "-Woverlength-strings" diff --git a/absl/copts/GENERATED_copts.bzl b/absl/copts/GENERATED_copts.bzl index 6707488f..d2bd5608 100644 --- a/absl/copts/GENERATED_copts.bzl +++ b/absl/copts/GENERATED_copts.bzl @@ -72,6 +72,7 @@ ABSL_LLVM_FLAGS = [ "-Wformat-security", "-Wgnu-redeclared-enum", "-Winfinite-recursion", + "-Winvalid-constexpr", "-Wliteral-conversion", "-Wmissing-declarations", "-Woverlength-strings", diff --git a/absl/copts/copts.py b/absl/copts/copts.py index cf52981c..ce30df89 100644 --- a/absl/copts/copts.py +++ b/absl/copts/copts.py @@ -87,6 +87,7 @@ COPT_VARS = { "-Wformat-security", "-Wgnu-redeclared-enum", "-Winfinite-recursion", + "-Winvalid-constexpr", "-Wliteral-conversion", "-Wmissing-declarations", "-Woverlength-strings", diff --git a/absl/copts/generate_copts.py b/absl/copts/generate_copts.py index 0e5dc9fa..34be2fc2 100755 --- a/absl/copts/generate_copts.py +++ b/absl/copts/generate_copts.py @@ -1,4 +1,4 @@ -#!/usr/bin/python +#!/usr/bin/env python3 """Generate Abseil compile compile option configs. Usage: /copts/generate_copts.py diff --git a/absl/strings/BUILD.bazel b/absl/strings/BUILD.bazel index 5d4ad7e3..68c71f0a 100644 --- a/absl/strings/BUILD.bazel +++ b/absl/strings/BUILD.bazel @@ -354,6 +354,7 @@ cc_library( hdrs = ["internal/cordz_handle.h"], copts = ABSL_DEFAULT_COPTS, deps = [ + "//absl/base", "//absl/base:config", "//absl/base:raw_logging_internal", "//absl/synchronization", @@ -371,8 +372,10 @@ cc_library( ":cordz_handle", ":cordz_statistics", ":cordz_update_tracker", + "//absl/base", "//absl/base:config", "//absl/base:core_headers", + "//absl/base:raw_logging_internal", "//absl/debugging:stacktrace", "//absl/synchronization", "//absl/types:span", @@ -525,6 +528,7 @@ cc_library( deps = [ ":cord", ":cord_internal", + ":strings", ], ) @@ -587,6 +591,7 @@ cc_test( visibility = ["//visibility:private"], deps = [ ":cord", + ":cord_test_helpers", ":cordz_functions", ":cordz_info", ":cordz_sample_token", diff --git a/absl/strings/CMakeLists.txt b/absl/strings/CMakeLists.txt index 2a8e3df2..80ae2a60 100644 --- a/absl/strings/CMakeLists.txt +++ b/absl/strings/CMakeLists.txt @@ -655,6 +655,7 @@ absl_cc_library( COPTS ${ABSL_DEFAULT_COPTS} DEPS + absl::base absl::config absl::raw_logging_internal absl::synchronization @@ -687,6 +688,7 @@ absl_cc_library( COPTS ${ABSL_DEFAULT_COPTS} DEPS + absl::base absl::config absl::cord_internal absl::cordz_functions @@ -695,6 +697,7 @@ absl_cc_library( absl::cordz_update_tracker absl::core_headers absl::span + absl::raw_logging_internal absl::stacktrace absl::synchronization ) @@ -829,6 +832,7 @@ absl_cc_library( DEPS absl::cord absl::cord_internal + absl::strings TESTONLY ) @@ -914,6 +918,7 @@ absl_cc_test( ${ABSL_TEST_COPTS} DEPS absl::cord + absl::cord_test_helpers absl::cordz_test_helpers absl::cordz_functions absl::cordz_info diff --git a/absl/strings/cord.cc b/absl/strings/cord.cc index 768106d4..15d17337 100644 --- a/absl/strings/cord.cc +++ b/absl/strings/cord.cc @@ -210,7 +210,7 @@ static CordRep* MakeBalancedTree(CordRep** reps, size_t n) { } static CordRepFlat* CreateFlat(const char* data, size_t length, - size_t alloc_hint) { + size_t alloc_hint) { CordRepFlat* flat = CordRepFlat::New(length + alloc_hint); flat->length = length; memcpy(flat->Data(), data, length); @@ -234,9 +234,7 @@ static CordRep* RingNewTree(const char* data, size_t length, // Create a new tree out of the specified array. // The returned node has a refcount of 1. -static CordRep* NewTree(const char* data, - size_t length, - size_t alloc_hint) { +static CordRep* NewTree(const char* data, size_t length, size_t alloc_hint) { if (length == 0) return nullptr; if (cord_ring_enabled()) { return RingNewTree(data, length, alloc_hint); @@ -303,24 +301,6 @@ inline char* Cord::InlineRep::set_data(size_t n) { return data_.as_chars(); } -static inline CordRepFlat* MakeFlat(const InlineData& data, size_t extra = 0) { - static_assert(kMinFlatLength >= sizeof(data), ""); - size_t len = data.inline_size(); - CordRepFlat* result = CordRepFlat::New(len + extra); - result->length = len; - memcpy(result->Data(), data.as_chars(), sizeof(data)); - return result; -} - -inline CordRep* Cord::InlineRep::force_tree(size_t extra_hint) { - if (data_.is_tree()) { - return data_.as_tree(); - } - CordRepFlat* result = MakeFlat(data_, extra_hint); - set_tree(result); - return result; -} - inline void Cord::InlineRep::reduce_size(size_t n) { size_t tag = inline_size(); assert(tag <= kMaxInline); @@ -346,7 +326,7 @@ void Cord::InlineRep::AppendTreeToInlined(CordRep* tree, MethodIdentifier method) { assert(!is_tree()); if (!data_.is_empty()) { - CordRepFlat* flat = MakeFlat(data_); + CordRepFlat* flat = MakeFlatWithExtraCapacity(0); if (cord_ring_enabled()) { tree = CordRepRing::Append(CordRepRing::Create(flat, 1), tree); } else { @@ -376,14 +356,38 @@ void Cord::InlineRep::AppendTree(CordRep* tree, MethodIdentifier method) { } } -void Cord::InlineRep::PrependTree(CordRep* tree) { +void Cord::InlineRep::PrependTreeToInlined(CordRep* tree, + MethodIdentifier method) { + assert(!is_tree()); + if (!data_.is_empty()) { + CordRepFlat* flat = MakeFlatWithExtraCapacity(0); + if (cord_ring_enabled()) { + tree = CordRepRing::Prepend(CordRepRing::Create(flat, 1), tree); + } else { + tree = Concat(tree, flat); + } + } + EmplaceTree(tree, method); +} + +void Cord::InlineRep::PrependTreeToTree(CordRep* tree, + MethodIdentifier method) { + assert(is_tree()); + const CordzUpdateScope scope(data_.cordz_info(), method); + if (cord_ring_enabled()) { + tree = CordRepRing::Prepend(ForceRing(data_.as_tree(), 1), tree); + } else { + tree = Concat(tree, data_.as_tree()); + } + SetTree(tree, scope); +} + +void Cord::InlineRep::PrependTree(CordRep* tree, MethodIdentifier method) { assert(tree != nullptr); - if (data_.is_empty()) { - set_tree(tree); - } else if (cord_ring_enabled()) { - set_tree(CordRepRing::Prepend(ForceRing(force_tree(0), 1), tree)); + if (data_.is_tree()) { + PrependTreeToTree(tree, method); } else { - set_tree(Concat(tree, force_tree(0))); + PrependTreeToInlined(tree, method); } } @@ -435,78 +439,43 @@ static inline bool PrepareAppendRegion(CordRep* root, char** region, return true; } +template void Cord::InlineRep::GetAppendRegion(char** region, size_t* size, - size_t max_length) { - if (max_length == 0) { - *region = nullptr; - *size = 0; - return; - } - - // Try to fit in the inline buffer if possible. - if (!is_tree()) { - size_t inline_length = inline_size(); - if (max_length <= kMaxInline - inline_length) { - *region = data_.as_chars() + inline_length; - *size = max_length; - set_inline_size(inline_length + max_length); + size_t length) { + auto constexpr method = CordzUpdateTracker::kGetAppendRegion; + + CordRep* root = tree(); + size_t sz = root ? root->length : inline_size(); + if (root == nullptr) { + size_t available = kMaxInline - sz; + if (available >= (has_length ? length : 1)) { + *region = data_.as_chars() + sz; + *size = has_length ? length : available; + set_inline_size(has_length ? sz + length : kMaxInline); return; } } - CordRep* root = force_tree(max_length); - - if (PrepareAppendRegion(root, region, size, max_length)) { - UpdateCordzStatistics(); + size_t extra = has_length ? length : (std::max)(sz, kMinFlatLength); + CordRep* rep = root ? root : MakeFlatWithExtraCapacity(extra); + CordzUpdateScope scope(root ? data_.cordz_info() : nullptr, method); + if (PrepareAppendRegion(rep, region, size, length)) { + CommitTree(root, rep, scope, method); return; } // Allocate new node. - CordRepFlat* new_node = - CordRepFlat::New(std::max(static_cast(root->length), max_length)); - new_node->length = std::min(new_node->Capacity(), max_length); + CordRepFlat* new_node = CordRepFlat::New(extra); + new_node->length = std::min(new_node->Capacity(), length); *region = new_node->Data(); *size = new_node->length; if (cord_ring_enabled()) { - replace_tree(CordRepRing::Append(ForceRing(root, 1), new_node)); - return; - } - replace_tree(Concat(root, new_node)); -} - -void Cord::InlineRep::GetAppendRegion(char** region, size_t* size) { - const size_t max_length = std::numeric_limits::max(); - - // Try to fit in the inline buffer if possible. - if (!data_.is_tree()) { - size_t inline_length = inline_size(); - if (inline_length < kMaxInline) { - *region = data_.as_chars() + inline_length; - *size = kMaxInline - inline_length; - set_inline_size(kMaxInline); - return; - } - } - - CordRep* root = force_tree(max_length); - - if (PrepareAppendRegion(root, region, size, max_length)) { - UpdateCordzStatistics(); - return; - } - - // Allocate new node. - CordRepFlat* new_node = CordRepFlat::New(root->length); - new_node->length = new_node->Capacity(); - *region = new_node->Data(); - *size = new_node->length; - - if (cord_ring_enabled()) { - replace_tree(CordRepRing::Append(ForceRing(root, 1), new_node)); - return; + rep = CordRepRing::Append(ForceRing(rep, 1), new_node); + } else { + rep = Concat(rep, new_node); } - replace_tree(Concat(root, new_node)); + CommitTree(root, rep, scope, method); } // If the rep is a leaf, this will increment the value at total_mem_usage and @@ -529,21 +498,26 @@ void Cord::InlineRep::UpdateCordzStatisticsSlow() { } void Cord::InlineRep::AssignSlow(const Cord::InlineRep& src) { - UnrefTree(); - - data_ = src.data_; - if (is_tree()) { - CordRep::Ref(tree()); - clear_cordz_info(); - CordzInfo::MaybeTrackCord(data_, CordzUpdateTracker::kAssignCord); + assert(&src != this); + assert(is_tree() || src.is_tree()); + auto constexpr method = CordzUpdateTracker::kAssignCord; + if (CordRep* tree = this->tree()) { + CordzUpdateScope scope(data_.cordz_info(), method); + CordRep::Unref(tree); + if (CordRep* src_tree = src.tree()) { + SetTree(CordRep::Ref(src_tree), scope); + } else { + scope.SetCordRep(nullptr); + data_ = src.data_; + } + } else { + EmplaceTree(CordRep::Ref(src.as_tree()), method); } } void Cord::InlineRep::UnrefTree() { if (is_tree()) { - if (ABSL_PREDICT_FALSE(is_profiled())) { - absl::cord_internal::CordzInfo::UntrackCord(cordz_info()); - } + CordzInfo::MaybeUntrackCord(data_.cordz_info()); CordRep::Unref(tree()); } } @@ -595,12 +569,9 @@ template Cord::Cord(std::string&& src); // The destruction code is separate so that the compiler can determine // that it does not need to call the destructor on a moved-from Cord. void Cord::DestroyCordSlow() { - if (ABSL_PREDICT_FALSE(contents_.is_profiled())) { - absl::cord_internal::CordzInfo::UntrackCord(contents_.cordz_info()); - } - if (CordRep* tree = contents_.tree()) { - CordRep::Unref(VerifyTree(tree)); - } + assert(contents_.is_tree()); + CordzInfo::MaybeUntrackCord(contents_.cordz_info()); + CordRep::Unref(VerifyTree(contents_.as_tree())); } // -------------------------------------------------------------------- @@ -613,23 +584,18 @@ void Cord::Clear() { } Cord& Cord::operator=(absl::string_view src) { - if (ABSL_PREDICT_FALSE(contents_.is_profiled())) { - absl::cord_internal::CordzInfo::UntrackCord(contents_.cordz_info()); - contents_.clear_cordz_info(); - } - 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()); contents_.set_data(data, length, true); if (tree) CordRep::Unref(tree); return *this; } if (tree != nullptr && tree->tag >= FLAT && - tree->flat()->Capacity() >= length && - tree->refcount.IsOne()) { + 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; @@ -655,71 +621,70 @@ 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(const char* src_data, size_t src_size) { - if (src_size == 0) return; // memcpy(_, nullptr, 0) is undefined. +void Cord::InlineRep::AppendArray(absl::string_view src, + MethodIdentifier method) { + if (src.empty()) return; // memcpy(_, nullptr, 0) is undefined. size_t appended = 0; - CordRep* root = nullptr; - if (is_tree()) { - root = data_.as_tree(); + CordRep* rep = tree(); + const CordRep* const root = rep; + CordzUpdateScope scope(root ? cordz_info() : nullptr, method); + if (root != nullptr) { char* region; - if (PrepareAppendRegion(root, ®ion, &appended, src_size)) { - memcpy(region, src_data, appended); - UpdateCordzStatistics(); + if (PrepareAppendRegion(rep, ®ion, &appended, src.size())) { + memcpy(region, src.data(), appended); } } else { // Try to fit in the inline buffer if possible. size_t inline_length = inline_size(); - if (src_size <= kMaxInline - inline_length) { + if (src.size() <= kMaxInline - inline_length) { // Append new data to embedded array - memcpy(data_.as_chars() + inline_length, src_data, src_size); - set_inline_size(inline_length + src_size); + memcpy(data_.as_chars() + inline_length, src.data(), src.size()); + set_inline_size(inline_length + src.size()); return; } - // It is possible that src_data == data_, but when we transition from an + // 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. // 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; - const size_t size2 = inline_length + src_size / 10; - root = CordRepFlat::New(std::max(size1, size2)); - appended = std::min( - src_size, root->flat()->Capacity() - inline_length); - memcpy(root->flat()->Data(), data_.as_chars(), inline_length); - memcpy(root->flat()->Data() + inline_length, src_data, appended); - root->length = inline_length + appended; - set_tree(root); - } - - src_data += appended; - src_size -= appended; - if (src_size == 0) { + const size_t size1 = inline_length * 2 + src.size(); + const size_t size2 = inline_length + src.size() / 10; + rep = CordRepFlat::New(std::max(size1, size2)); + appended = std::min(src.size(), rep->flat()->Capacity() - inline_length); + memcpy(rep->flat()->Data(), data_.as_chars(), inline_length); + memcpy(rep->flat()->Data() + inline_length, src.data(), appended); + rep->length = inline_length + appended; + } + + src.remove_prefix(appended); + if (src.empty()) { + CommitTree(root, rep, scope, method); return; } if (cord_ring_enabled()) { - absl::string_view data(src_data, src_size); - root = ForceRing(root, (data.size() - 1) / kMaxFlatLength + 1); - replace_tree(CordRepRing::Append(root->ring(), data)); - return; - } - - // Use new block(s) for any remaining bytes that were not handled above. - // Alloc extra memory only if the right child of the root of the new tree is - // going to be a FLAT node, which will permit further inplace appends. - size_t length = src_size; - if (src_size < kMaxFlatLength) { - // The new length is either - // - old size + 10% - // - old_size + src_size - // This will cause a reasonable conservative step-up in size that is still - // large enough to avoid excessive amounts of small fragments being added. - length = std::max(root->length / 10, src_size); + rep = ForceRing(rep, (src.size() - 1) / kMaxFlatLength + 1); + rep = CordRepRing::Append(rep->ring(), src); + } else { + // Use new block(s) for any remaining bytes that were not handled above. + // Alloc extra memory only if the right child of the root of the new tree + // is going to be a FLAT node, which will permit further inplace appends. + size_t length = src.size(); + if (src.size() < kMaxFlatLength) { + // The new length is either + // - old size + 10% + // - old_size + src.size() + // This will cause a reasonable conservative step-up in size that is + // still large enough to avoid excessive amounts of small fragments + // being added. + length = std::max(rep->length / 10, src.size()); + } + rep = Concat(rep, NewTree(src.data(), src.size(), length - src.size())); } - set_tree(Concat(root, NewTree(src_data, src_size, length - src_size))); + CommitTree(root, rep, scope, method); } inline CordRep* Cord::TakeRep() const& { @@ -734,12 +699,13 @@ inline CordRep* Cord::TakeRep() && { template inline void Cord::AppendImpl(C&& src) { + auto constexpr method = CordzUpdateTracker::kAppendCord; if (empty()) { // Since destination is empty, we can avoid allocating a node, if (src.contents_.is_tree()) { // by taking the tree directly CordRep* rep = std::forward(src).TakeRep(); - contents_.EmplaceTree(rep, CordzUpdateTracker::kAppendCord); + contents_.EmplaceTree(rep, method); } else { // or copying over inline data contents_.data_ = src.contents_.data_; @@ -753,12 +719,12 @@ inline void Cord::AppendImpl(C&& src) { CordRep* src_tree = src.contents_.tree(); if (src_tree == nullptr) { // src has embedded data. - contents_.AppendArray(src.contents_.data(), src_size); + contents_.AppendArray({src.contents_.data(), src_size}, method); return; } if (src_tree->tag >= FLAT) { // src tree just has one flat node. - contents_.AppendArray(src_tree->flat()->Data(), src_size); + contents_.AppendArray({src_tree->flat()->Data(), src_size}, method); return; } if (&src == this) { @@ -797,7 +763,7 @@ void Cord::Prepend(const Cord& src) { CordRep* src_tree = src.contents_.tree(); if (src_tree != nullptr) { CordRep::Ref(src_tree); - contents_.PrependTree(src_tree); + contents_.PrependTree(src_tree, CordzUpdateTracker::kPrependCord); return; } @@ -820,7 +786,8 @@ void Cord::Prepend(absl::string_view src) { return; } } - contents_.PrependTree(NewTree(src.data(), src.size(), 0)); + CordRep* rep = NewTree(src.data(), src.size(), 0); + contents_.PrependTree(rep, CordzUpdateTracker::kPrependString); } template > @@ -1847,8 +1814,7 @@ static void DumpNode(CordRep* rep, bool include_data, std::ostream* os, *os << absl::CEscape(std::string(rep->external()->base, rep->length)); *os << "]\n"; } else if (rep->tag >= FLAT) { - *os << "FLAT cap=" << rep->flat()->Capacity() - << " ["; + *os << "FLAT cap=" << rep->flat()->Capacity() << " ["; if (include_data) *os << absl::CEscape(std::string(rep->flat()->Data(), rep->length)); *os << "]\n"; @@ -1860,7 +1826,7 @@ static void DumpNode(CordRep* rep, bool include_data, std::ostream* os, do { DumpNode(ring->entry_child(head), include_data, os, indent + kIndentStep); - head = ring->advance(head);; + head = ring->advance(head); } while (head != ring->tail()); } if (stack.empty()) break; @@ -1906,9 +1872,8 @@ static bool VerifyNode(CordRep* root, CordRep* start_node, worklist.push_back(node->concat()->left); } } else if (node->tag >= FLAT) { - ABSL_INTERNAL_CHECK( - node->length <= node->flat()->Capacity(), - ReportError(root, node)); + ABSL_INTERNAL_CHECK(node->length <= node->flat()->Capacity(), + ReportError(root, node)); } else if (node->tag == EXTERNAL) { ABSL_INTERNAL_CHECK(node->external()->base != nullptr, ReportError(root, node)); diff --git a/absl/strings/cord.h b/absl/strings/cord.h index dd6c3bca..d5a13b34 100644 --- a/absl/strings/cord.h +++ b/absl/strings/cord.h @@ -671,6 +671,7 @@ class Cord { private: using CordRep = absl::cord_internal::CordRep; + using CordRepFlat = absl::cord_internal::CordRepFlat; using CordzInfo = cord_internal::CordzInfo; using CordzUpdateScope = cord_internal::CordzUpdateScope; using CordzUpdateTracker = cord_internal::CordzUpdateTracker; @@ -728,12 +729,15 @@ class Cord { // Returns non-null iff was holding a pointer absl::cord_internal::CordRep* clear(); // Converts to pointer if necessary. - absl::cord_internal::CordRep* force_tree(size_t extra_hint); void reduce_size(size_t n); // REQUIRES: holding data void remove_prefix(size_t n); // REQUIRES: holding data - void AppendArray(const char* src_data, size_t src_size); + void AppendArray(absl::string_view src, MethodIdentifier method); absl::string_view FindFlatStartPiece() const; + // Creates a CordRepFlat instance from the current inlined data with `extra' + // bytes of desired additional capacity. + CordRepFlat* MakeFlatWithExtraCapacity(size_t extra); + // Sets the tree value for this instance. `rep` must not be null. // Requires the current instance to hold a tree, and a lock to be held on // any CordzInfo referenced by this instance. The latter is enforced through @@ -747,12 +751,22 @@ class Cord { // value to a non-inlined (tree / ring) value. void EmplaceTree(CordRep* rep, 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(). + void CommitTree(const CordRep* old_rep, CordRep* rep, + const CordzUpdateScope& scope, MethodIdentifier method); + void AppendTreeToInlined(CordRep* tree, MethodIdentifier method); void AppendTreeToTree(CordRep* tree, MethodIdentifier method); void AppendTree(CordRep* tree, MethodIdentifier method); - void PrependTree(absl::cord_internal::CordRep* tree); - void GetAppendRegion(char** region, size_t* size, size_t max_length); - void GetAppendRegion(char** region, size_t* size); + void PrependTreeToInlined(CordRep* tree, MethodIdentifier method); + void PrependTreeToTree(CordRep* tree, MethodIdentifier method); + void PrependTree(CordRep* tree, MethodIdentifier method); + + template + void GetAppendRegion(char** region, size_t* size, size_t length); + bool IsSame(const InlineRep& other) const { return memcmp(&data_, &other.data_, sizeof(data_)) == 0; } @@ -1041,6 +1055,16 @@ inline size_t Cord::InlineRep::size() const { return is_tree() ? as_tree()->length : inline_size(); } +inline cord_internal::CordRepFlat* Cord::InlineRep::MakeFlatWithExtraCapacity( + size_t extra) { + static_assert(cord_internal::kMinFlatLength >= sizeof(data_), ""); + size_t len = data_.inline_size(); + auto* result = CordRepFlat::New(len + extra); + result->length = len; + memcpy(result->Data(), data_.as_chars(), sizeof(data_)); + return result; +} + inline void Cord::InlineRep::EmplaceTree(CordRep* rep, MethodIdentifier method) { data_.make_tree(rep); @@ -1055,10 +1079,20 @@ inline void Cord::InlineRep::SetTree(CordRep* rep, scope.SetCordRep(rep); } +inline void Cord::InlineRep::CommitTree(const CordRep* old_rep, CordRep* rep, + const CordzUpdateScope& scope, + MethodIdentifier method) { + if (old_rep) { + SetTree(rep, scope); + } else { + EmplaceTree(rep, method); + } +} + inline void Cord::InlineRep::set_tree(absl::cord_internal::CordRep* rep) { if (rep == nullptr) { - if (ABSL_PREDICT_FALSE(is_profiled())) { - absl::cord_internal::CordzInfo::UntrackCord(cordz_info()); + if (data_.is_tree()) { + CordzInfo::MaybeUntrackCord(data_.cordz_info()); } ResetToEmpty(); } else { @@ -1086,8 +1120,8 @@ inline void Cord::InlineRep::replace_tree(absl::cord_internal::CordRep* rep) { } inline absl::cord_internal::CordRep* Cord::InlineRep::clear() { - if (ABSL_PREDICT_FALSE(is_profiled())) { - absl::cord_internal::CordzInfo::UntrackCord(cordz_info()); + if (is_tree()) { + CordzInfo::MaybeUntrackCord(cordz_info()); } absl::cord_internal::CordRep* result = tree(); ResetToEmpty(); @@ -1180,7 +1214,7 @@ inline absl::string_view Cord::Flatten() { } inline void Cord::Append(absl::string_view src) { - contents_.AppendArray(src.data(), src.size()); + contents_.AppendArray(src, CordzUpdateTracker::kAppendString); } extern template void Cord::Append(std::string&& src); diff --git a/absl/strings/cord_test_helpers.h b/absl/strings/cord_test_helpers.h index 3815df81..6ccccc51 100644 --- a/absl/strings/cord_test_helpers.h +++ b/absl/strings/cord_test_helpers.h @@ -17,12 +17,50 @@ #ifndef ABSL_STRINGS_CORD_TEST_HELPERS_H_ #define ABSL_STRINGS_CORD_TEST_HELPERS_H_ +#include +#include + #include "absl/strings/cord.h" #include "absl/strings/internal/cord_internal.h" +#include "absl/strings/string_view.h" namespace absl { ABSL_NAMESPACE_BEGIN +// Cord sizes relevant for testing +enum class TestCordSize { + kEmpty = 0, + kInlined = cord_internal::kMaxInline / 2 + 1, + kSmall = cord_internal::kMaxBytesToCopy / 2 + 1, + kMedium = cord_internal::kMaxFlatLength / 2 + 1, + kLarge = cord_internal::kMaxFlatLength * 4 +}; + +// To string helper +inline absl::string_view ToString(TestCordSize size) { + switch (size) { + case TestCordSize::kEmpty: + return "Empty"; + case TestCordSize::kInlined: + return "Inlined"; + case TestCordSize::kSmall: + return "Small"; + case TestCordSize::kMedium: + return "Medium"; + case TestCordSize::kLarge: + return "Large"; + } + return "???"; +} + +// Returns the length matching the specified size +inline size_t Length(TestCordSize size) { return static_cast(size); } + +// Stream output helper +inline std::ostream& operator<<(std::ostream& stream, TestCordSize size) { + return stream << ToString(size); +} + // Creates a multi-segment Cord from an iterable container of strings. The // resulting Cord is guaranteed to have one segment for every string in the // container. This allows code to be unit tested with multi-segment Cord diff --git a/absl/strings/cordz_test.cc b/absl/strings/cordz_test.cc index 6e8deefd..b16e9686 100644 --- a/absl/strings/cordz_test.cc +++ b/absl/strings/cordz_test.cc @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include "gmock/gmock.h" @@ -20,6 +21,7 @@ #include "absl/base/internal/raw_logging.h" #include "absl/base/macros.h" #include "absl/strings/cord.h" +#include "absl/strings/cord_test_helpers.h" #include "absl/strings/cordz_test_helpers.h" #include "absl/strings/internal/cordz_functions.h" #include "absl/strings/internal/cordz_info.h" @@ -31,6 +33,8 @@ #ifdef ABSL_INTERNAL_CORDZ_ENABLED +using testing::Eq; + namespace absl { ABSL_NAMESPACE_BEGIN @@ -40,48 +44,126 @@ using cord_internal::CordzStatistics; using cord_internal::CordzUpdateTracker; using Method = CordzUpdateTracker::MethodIdentifier; +// Do not print cord contents, we only care about 'size' perhaps. +// Note that this method must be inside the named namespace. +inline void PrintTo(const Cord& cord, std::ostream* s) { + if (s) *s << "Cord[" << cord.size() << "]"; +} + namespace { -std::string MakeString(int length) { - std::string s(length, '.'); - for (int i = 4; i < length; i += 2) { - s[i] = '\b'; - } - return s; +// 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) { + thread_local std::string str; + str = std::string(size, '.'); + return str; +} + +absl::string_view MakeString(TestCordSize size) { + return MakeString(Length(size)); +} + +std::string TestParamToString(::testing::TestParamInfo size) { + return absl::StrCat("On", ToString(size.param), "Cord"); } -TEST(CordzTest, ConstructSmallStringView) { - CordzSamplingIntervalHelper sample_every(1); - Cord cord(absl::string_view(MakeString(50))); +class CordzUpdateTest : public testing::TestWithParam { + public: + Cord& cord() { return cord_; } + + Method InitialOr(Method method) const { + return (GetParam() > TestCordSize::kInlined) ? Method::kConstructorString + : method; + } + + private: + CordzSamplingIntervalHelper sample_every_{1}; + Cord cord_{MakeString(GetParam())}; +}; + +INSTANTIATE_TEST_SUITE_P(WithParam, CordzUpdateTest, + testing::Values(TestCordSize::kEmpty, + TestCordSize::kInlined, + TestCordSize::kLarge), + TestParamToString); + +TEST(CordzTest, ConstructSmallString) { + CordzSamplingIntervalHelper sample_every{1}; + Cord cord(MakeString(TestCordSize::kSmall)); EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString)); } -TEST(CordzTest, ConstructLargeStringView) { - CordzSamplingIntervalHelper sample_every(1); - Cord cord(absl::string_view(MakeString(5000))); +TEST(CordzTest, ConstructLargeString) { + CordzSamplingIntervalHelper sample_every{1}; + Cord cord(MakeString(TestCordSize::kLarge)); EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString)); } TEST(CordzTest, CopyConstruct) { - CordzSamplingIntervalHelper sample_every(1); - Cord src = UnsampledCord(MakeString(5000)); + CordzSamplingIntervalHelper sample_every{1}; + Cord src = UnsampledCord(MakeString(TestCordSize::kLarge)); Cord cord(src); EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorCord)); } -TEST(CordzTest, AppendLargeCordToEmpty) { - CordzSamplingIntervalHelper sample_every(1); - Cord cord; - Cord src = UnsampledCord(MakeString(5000)); - cord.Append(src); - EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kAppendCord)); +TEST(CordzTest, MoveConstruct) { + CordzSamplingIntervalHelper sample_every{1}; + Cord src(MakeString(TestCordSize::kLarge)); + Cord cord(std::move(src)); + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString)); } -TEST(CordzTest, MoveAppendLargeCordToEmpty) { - CordzSamplingIntervalHelper sample_every(1); +TEST_P(CordzUpdateTest, AssignCord) { + Cord src = UnsampledCord(MakeString(TestCordSize::kLarge)); + cord() = src; + EXPECT_THAT(cord(), HasValidCordzInfoOf(InitialOr(Method::kAssignCord))); +} + +TEST(CordzTest, AssignInlinedCord) { + CordzSampleToken token; + CordzSamplingIntervalHelper sample_every{1}; + Cord cord(MakeString(TestCordSize::kLarge)); + const CordzInfo* info = GetCordzInfoForTesting(cord); + Cord src = UnsampledCord(MakeString(TestCordSize::kInlined)); + cord = src; + EXPECT_THAT(GetCordzInfoForTesting(cord), Eq(nullptr)); + EXPECT_FALSE(CordzInfoIsListed(info)); +} + +TEST(CordzTest, MoveAssignCord) { + CordzSamplingIntervalHelper sample_every{1}; Cord cord; - cord.Append(UnsampledCord(MakeString(5000))); - EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kAppendCord)); + Cord src(MakeString(TestCordSize::kLarge)); + cord = std::move(src); + EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString)); +} + +TEST_P(CordzUpdateTest, AppendCord) { + Cord src = UnsampledCord(MakeString(TestCordSize::kLarge)); + cord().Append(src); + EXPECT_THAT(cord(), HasValidCordzInfoOf(InitialOr(Method::kAppendCord))); +} + +TEST_P(CordzUpdateTest, MoveAppendCord) { + cord().Append(UnsampledCord(MakeString(TestCordSize::kLarge))); + 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))); +} + +TEST_P(CordzUpdateTest, AppendLargeArray) { + cord().Append(MakeString(TestCordSize::kLarge)); + EXPECT_THAT(cord(), HasValidCordzInfoOf(InitialOr(Method::kAppendString))); } } // namespace diff --git a/absl/strings/cordz_test_helpers.h b/absl/strings/cordz_test_helpers.h index 453c2f9d..d9573c97 100644 --- a/absl/strings/cordz_test_helpers.h +++ b/absl/strings/cordz_test_helpers.h @@ -15,6 +15,8 @@ #ifndef ABSL_STRINGS_CORDZ_TEST_HELPERS_H_ #define ABSL_STRINGS_CORDZ_TEST_HELPERS_H_ +#include + #include "gmock/gmock.h" #include "gtest/gtest.h" #include "absl/base/config.h" @@ -32,6 +34,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; return cord.contents_.cordz_info(); } @@ -44,11 +47,13 @@ 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) { diff --git a/absl/strings/internal/cordz_handle.cc b/absl/strings/internal/cordz_handle.cc index 22d07978..5297ec8e 100644 --- a/absl/strings/internal/cordz_handle.cc +++ b/absl/strings/internal/cordz_handle.cc @@ -16,16 +16,19 @@ #include #include "absl/base/internal/raw_logging.h" // For ABSL_RAW_CHECK +#include "absl/base/internal/spinlock.h" namespace absl { ABSL_NAMESPACE_BEGIN namespace cord_internal { +using ::absl::base_internal::SpinLockHolder; + ABSL_CONST_INIT CordzHandle::Queue CordzHandle::global_queue_(absl::kConstInit); CordzHandle::CordzHandle(bool is_snapshot) : is_snapshot_(is_snapshot) { if (is_snapshot) { - MutexLock lock(&queue_->mutex); + SpinLockHolder lock(&queue_->mutex); CordzHandle* dq_tail = queue_->dq_tail.load(std::memory_order_acquire); if (dq_tail != nullptr) { dq_prev_ = dq_tail; @@ -40,7 +43,7 @@ CordzHandle::~CordzHandle() { if (is_snapshot_) { std::vector to_delete; { - absl::MutexLock lock(&queue_->mutex); + SpinLockHolder lock(&queue_->mutex); CordzHandle* next = dq_next_; if (dq_prev_ == nullptr) { // We were head of the queue, delete every CordzHandle until we reach @@ -70,7 +73,7 @@ void CordzHandle::Delete(CordzHandle* handle) { handle->ODRCheck(); Queue* const queue = handle->queue_; if (!handle->is_snapshot_ && !queue->IsEmpty()) { - MutexLock lock(&queue->mutex); + SpinLockHolder lock(&queue->mutex); CordzHandle* dq_tail = queue->dq_tail.load(std::memory_order_acquire); if (dq_tail != nullptr) { handle->dq_prev_ = dq_tail; @@ -85,7 +88,7 @@ void CordzHandle::Delete(CordzHandle* handle) { std::vector CordzHandle::DiagnosticsGetDeleteQueue() { std::vector handles; - MutexLock lock(&global_queue_.mutex); + SpinLockHolder lock(&global_queue_.mutex); CordzHandle* dq_tail = global_queue_.dq_tail.load(std::memory_order_acquire); for (const CordzHandle* p = dq_tail; p; p = p->dq_prev_) { handles.push_back(p); @@ -100,7 +103,7 @@ bool CordzHandle::DiagnosticsHandleIsSafeToInspect( if (handle == nullptr) return true; if (handle->is_snapshot_) return false; bool snapshot_found = false; - MutexLock lock(&queue_->mutex); + SpinLockHolder lock(&queue_->mutex); for (const CordzHandle* p = queue_->dq_tail; p; p = p->dq_prev_) { if (p == handle) return !snapshot_found; if (p == this) snapshot_found = true; @@ -117,7 +120,7 @@ CordzHandle::DiagnosticsGetSafeToInspectDeletedHandles() { return handles; } - MutexLock lock(&queue_->mutex); + SpinLockHolder lock(&queue_->mutex); for (const CordzHandle* p = dq_next_; p != nullptr; p = p->dq_next_) { if (!p->is_snapshot()) { handles.push_back(p); diff --git a/absl/strings/internal/cordz_handle.h b/absl/strings/internal/cordz_handle.h index 71563c8b..93076cfd 100644 --- a/absl/strings/internal/cordz_handle.h +++ b/absl/strings/internal/cordz_handle.h @@ -20,6 +20,7 @@ #include "absl/base/config.h" #include "absl/base/internal/raw_logging.h" +#include "absl/base/internal/spinlock.h" #include "absl/synchronization/mutex.h" namespace absl { @@ -70,9 +71,11 @@ class CordzHandle { // Global queue data. CordzHandle stores a pointer to the global queue // instance to harden against ODR violations. struct Queue { - constexpr explicit Queue(absl::ConstInitType) : mutex(absl::kConstInit) {} + constexpr explicit Queue(absl::ConstInitType) + : mutex(absl::kConstInit, + absl::base_internal::SCHEDULE_COOPERATIVE_AND_KERNEL) {} - absl::Mutex mutex; + absl::base_internal::SpinLock mutex; std::atomic dq_tail ABSL_GUARDED_BY(mutex){nullptr}; // Returns true if this delete queue is empty. This method does not acquire diff --git a/absl/strings/internal/cordz_info.cc b/absl/strings/internal/cordz_info.cc index d2200680..0461ec46 100644 --- a/absl/strings/internal/cordz_info.cc +++ b/absl/strings/internal/cordz_info.cc @@ -15,6 +15,7 @@ #include "absl/strings/internal/cordz_info.h" #include "absl/base/config.h" +#include "absl/base/internal/spinlock.h" #include "absl/debugging/stacktrace.h" #include "absl/strings/internal/cord_internal.h" #include "absl/strings/internal/cordz_handle.h" @@ -27,22 +28,34 @@ namespace absl { ABSL_NAMESPACE_BEGIN namespace cord_internal { +using ::absl::base_internal::SpinLockHolder; + constexpr int CordzInfo::kMaxStackDepth; -ABSL_CONST_INIT std::atomic CordzInfo::ci_head_{nullptr}; -ABSL_CONST_INIT absl::Mutex CordzInfo::ci_mutex_(absl::kConstInit); +ABSL_CONST_INIT CordzInfo::List CordzInfo::global_list_{absl::kConstInit}; CordzInfo* CordzInfo::Head(const CordzSnapshot& snapshot) { ABSL_ASSERT(snapshot.is_snapshot()); - ABSL_ASSERT(snapshot.DiagnosticsHandleIsSafeToInspect(ci_head_unsafe())); - return ci_head_unsafe(); + + // We can do an 'unsafe' load of 'head', as we are guaranteed that the + // instance it points to is kept alive by the provided CordzSnapshot, so we + // can simply return the current value using an acquire load. + // We do enforce in DEBUG builds that the 'head' value is present in the + // delete queue: ODR violations may lead to 'snapshot' and 'global_list_' + // being in different libraries / modules. + CordzInfo* head = global_list_.head.load(std::memory_order_acquire); + ABSL_ASSERT(snapshot.DiagnosticsHandleIsSafeToInspect(head)); + return head; } CordzInfo* CordzInfo::Next(const CordzSnapshot& snapshot) const { ABSL_ASSERT(snapshot.is_snapshot()); + + // Similar to the 'Head()' function, we do not need a mutex here. + CordzInfo* next = ci_next_.load(std::memory_order_acquire); ABSL_ASSERT(snapshot.DiagnosticsHandleIsSafeToInspect(this)); - ABSL_ASSERT(snapshot.DiagnosticsHandleIsSafeToInspect(ci_next_unsafe())); - return ci_next_unsafe(); + ABSL_ASSERT(snapshot.DiagnosticsHandleIsSafeToInspect(next)); + return next; } void CordzInfo::TrackCord(InlineData& cord, MethodIdentifier method) { @@ -63,14 +76,6 @@ void CordzInfo::TrackCord(InlineData& cord, const InlineData& src, cordz_info->Track(); } -void CordzInfo::UntrackCord(CordzInfo* cordz_info) { - assert(cordz_info); - if (cordz_info) { - cordz_info->Untrack(); - CordzHandle::Delete(cordz_info); - } -} - CordzInfo::MethodIdentifier CordzInfo::GetParentMethod(const CordzInfo* src) { if (src == nullptr) return MethodIdentifier::kUnknown; return src->parent_method_ != MethodIdentifier::kUnknown ? src->parent_method_ @@ -110,14 +115,14 @@ CordzInfo::~CordzInfo() { } void CordzInfo::Track() { - absl::MutexLock l(&ci_mutex_); + SpinLockHolder l(&list_->mutex); - CordzInfo* const head = ci_head_.load(std::memory_order_acquire); + CordzInfo* const head = list_->head.load(std::memory_order_acquire); if (head != nullptr) { head->ci_prev_.store(this, std::memory_order_release); } ci_next_.store(head, std::memory_order_release); - ci_head_.store(this, std::memory_order_release); + list_->head.store(this, std::memory_order_release); } void CordzInfo::Untrack() { @@ -128,24 +133,28 @@ void CordzInfo::Untrack() { rep_ = nullptr; } - absl::MutexLock l(&ci_mutex_); - - CordzInfo* const head = ci_head_.load(std::memory_order_acquire); - CordzInfo* const next = ci_next_.load(std::memory_order_acquire); - CordzInfo* const prev = ci_prev_.load(std::memory_order_acquire); - - if (next) { - ABSL_ASSERT(next->ci_prev_.load(std::memory_order_acquire) == this); - next->ci_prev_.store(prev, std::memory_order_release); - } - if (prev) { - ABSL_ASSERT(head != this); - ABSL_ASSERT(prev->ci_next_.load(std::memory_order_acquire) == this); - prev->ci_next_.store(next, std::memory_order_release); - } else { - ABSL_ASSERT(head == this); - ci_head_.store(next, std::memory_order_release); + ODRCheck(); + { + SpinLockHolder l(&list_->mutex); + + CordzInfo* const head = list_->head.load(std::memory_order_acquire); + CordzInfo* const next = ci_next_.load(std::memory_order_acquire); + CordzInfo* const prev = ci_prev_.load(std::memory_order_acquire); + + if (next) { + ABSL_ASSERT(next->ci_prev_.load(std::memory_order_acquire) == this); + next->ci_prev_.store(prev, std::memory_order_release); + } + if (prev) { + ABSL_ASSERT(head != this); + ABSL_ASSERT(prev->ci_next_.load(std::memory_order_acquire) == this); + prev->ci_next_.store(next, std::memory_order_release); + } else { + ABSL_ASSERT(head == this); + list_->head.store(next, std::memory_order_release); + } } + CordzHandle::Delete(this); } void CordzInfo::Lock(MethodIdentifier method) @@ -163,7 +172,6 @@ void CordzInfo::Unlock() ABSL_UNLOCK_FUNCTION(mutex_) { mutex_.Unlock(); if (!tracked) { Untrack(); - CordzHandle::Delete(this); } } diff --git a/absl/strings/internal/cordz_info.h b/absl/strings/internal/cordz_info.h index 4cf57664..f7682cb3 100644 --- a/absl/strings/internal/cordz_info.h +++ b/absl/strings/internal/cordz_info.h @@ -20,6 +20,8 @@ #include #include "absl/base/config.h" +#include "absl/base/internal/raw_logging.h" +#include "absl/base/internal/spinlock.h" #include "absl/base/thread_annotations.h" #include "absl/strings/internal/cord_internal.h" #include "absl/strings/internal/cordz_functions.h" @@ -79,17 +81,22 @@ class ABSL_LOCKABLE CordzInfo : public CordzHandle { // and before the root cordrep of the sampled cord is unreffed. // This function may extend the lifetime of the cordrep in cases where the // CordInfo instance is being held by a concurrent collection thread. - static void UntrackCord(CordzInfo* cordz_info); + void Untrack(); + + // Invokes UntrackCord() on `info` if `info` is not null. + static void MaybeUntrackCord(CordzInfo* info); CordzInfo() = delete; CordzInfo(const CordzInfo&) = delete; CordzInfo& operator=(const CordzInfo&) = delete; // Retrieves the oldest existing CordzInfo. - static CordzInfo* Head(const CordzSnapshot& snapshot); + static CordzInfo* Head(const CordzSnapshot& snapshot) + ABSL_NO_THREAD_SAFETY_ANALYSIS; // Retrieves the next oldest existing CordzInfo older than 'this' instance. - CordzInfo* Next(const CordzSnapshot& snapshot) const; + CordzInfo* Next(const CordzSnapshot& snapshot) const + ABSL_NO_THREAD_SAFETY_ANALYSIS; // Locks this instance for the update identified by `method`. // Increases the count for `method` in `update_tracker`. @@ -141,6 +148,17 @@ class ABSL_LOCKABLE CordzInfo : public CordzHandle { } private: + // Global cordz info list. CordzInfo stores a pointer to the global list + // instance to harden against ODR violations. + struct List { + constexpr explicit List(absl::ConstInitType) + : mutex(absl::kConstInit, + absl::base_internal::SCHEDULE_COOPERATIVE_AND_KERNEL) {} + + absl::base_internal::SpinLock mutex; + std::atomic head ABSL_GUARDED_BY(mutex){nullptr}; + }; + static constexpr int kMaxStackDepth = 64; explicit CordzInfo(CordRep* rep, const CordzInfo* src, @@ -148,7 +166,6 @@ class ABSL_LOCKABLE CordzInfo : public CordzHandle { ~CordzInfo() override; void Track(); - void Untrack(); // Returns the parent method from `src`, which is either `parent_method_` or // `method_` depending on `parent_method_` being kUnknown. @@ -161,23 +178,20 @@ class ABSL_LOCKABLE CordzInfo : public CordzHandle { // Returns 0 if `src` is null. static int FillParentStack(const CordzInfo* src, void** stack); - // 'Unsafe' head/next/prev accessors not requiring the lock being held. - // These are used exclusively for iterations (Head / Next) where we enforce - // a token being held, so reading an 'old' / deleted pointer is fine. - static CordzInfo* ci_head_unsafe() ABSL_NO_THREAD_SAFETY_ANALYSIS { - return ci_head_.load(std::memory_order_acquire); - } - CordzInfo* ci_next_unsafe() const ABSL_NO_THREAD_SAFETY_ANALYSIS { - return ci_next_.load(std::memory_order_acquire); - } - CordzInfo* ci_prev_unsafe() const ABSL_NO_THREAD_SAFETY_ANALYSIS { - return ci_prev_.load(std::memory_order_acquire); + void ODRCheck() const { +#ifndef NDEBUG + ABSL_RAW_CHECK(list_ == &global_list_, "ODR violation in Cord"); +#endif } - static absl::Mutex ci_mutex_; - static std::atomic ci_head_ ABSL_GUARDED_BY(ci_mutex_); - std::atomic ci_prev_ ABSL_GUARDED_BY(ci_mutex_){nullptr}; - std::atomic ci_next_ ABSL_GUARDED_BY(ci_mutex_){nullptr}; + ABSL_CONST_INIT static List global_list_; + List* const list_ = &global_list_; + + // ci_prev_ and ci_next_ require the global list mutex to be held. + // Unfortunately we can't use thread annotations such that the thread safety + // analysis understands that list_ and global_list_ are one and the same. + std::atomic ci_prev_{nullptr}; + std::atomic ci_next_{nullptr}; mutable absl::Mutex mutex_; CordRep* rep_ ABSL_GUARDED_BY(mutex_); @@ -209,6 +223,13 @@ inline ABSL_ATTRIBUTE_ALWAYS_INLINE void CordzInfo::MaybeTrackCord( } } +inline ABSL_ATTRIBUTE_ALWAYS_INLINE void CordzInfo::MaybeUntrackCord( + CordzInfo* info) { + if (ABSL_PREDICT_FALSE(info)) { + info->Untrack(); + } +} + inline void CordzInfo::AssertHeld() ABSL_ASSERT_EXCLUSIVE_LOCK(mutex_) { #ifndef NDEBUG mutex_.AssertHeld(); diff --git a/absl/strings/internal/cordz_info_test.cc b/absl/strings/internal/cordz_info_test.cc index 76aa20b0..5eaf4b9c 100644 --- a/absl/strings/internal/cordz_info_test.cc +++ b/absl/strings/internal/cordz_info_test.cc @@ -70,7 +70,7 @@ TEST(CordzInfoTest, TrackCord) { EXPECT_FALSE(info->is_snapshot()); EXPECT_THAT(CordzInfo::Head(CordzSnapshot()), Eq(info)); EXPECT_THAT(info->GetCordRepForTesting(), Eq(data.rep.rep)); - CordzInfo::UntrackCord(info); + info->Untrack(); } TEST(CordzInfoTest, UntrackCord) { @@ -79,7 +79,7 @@ TEST(CordzInfoTest, UntrackCord) { CordzInfo* info = data.data.cordz_info(); CordzSnapshot snapshot; - CordzInfo::UntrackCord(info); + info->Untrack(); EXPECT_THAT(CordzInfo::Head(CordzSnapshot()), Eq(nullptr)); EXPECT_THAT(info->GetCordRepForTesting(), Eq(nullptr)); EXPECT_THAT(DeleteQueue(), ElementsAre(info, &snapshot)); @@ -96,7 +96,7 @@ TEST(CordzInfoTest, SetCordRep) { info->Unlock(); EXPECT_THAT(info->GetCordRepForTesting(), Eq(rep.rep)); - CordzInfo::UntrackCord(info); + info->Untrack(); } TEST(CordzInfoTest, SetCordRepNullUntracksCordOnUnlock) { @@ -121,7 +121,7 @@ TEST(CordzInfoTest, SetCordRepRequiresMutex) { CordzInfo* info = data.data.cordz_info(); TestCordRep rep; EXPECT_DEBUG_DEATH(info->SetCordRep(rep.rep), ".*"); - CordzInfo::UntrackCord(info); + info->Untrack(); } #endif // GTEST_HAS_DEATH_TEST @@ -143,11 +143,11 @@ TEST(CordzInfoTest, TrackUntrackHeadFirstV2) { EXPECT_THAT(info2->Next(snapshot), Eq(info1)); EXPECT_THAT(info1->Next(snapshot), Eq(nullptr)); - CordzInfo::UntrackCord(info2); + info2->Untrack(); ASSERT_THAT(CordzInfo::Head(snapshot), Eq(info1)); EXPECT_THAT(info1->Next(snapshot), Eq(nullptr)); - CordzInfo::UntrackCord(info1); + info1->Untrack(); ASSERT_THAT(CordzInfo::Head(snapshot), Eq(nullptr)); } @@ -168,11 +168,11 @@ TEST(CordzInfoTest, TrackUntrackTailFirstV2) { EXPECT_THAT(info2->Next(snapshot), Eq(info1)); EXPECT_THAT(info1->Next(snapshot), Eq(nullptr)); - CordzInfo::UntrackCord(info1); + info1->Untrack(); ASSERT_THAT(CordzInfo::Head(snapshot), Eq(info2)); EXPECT_THAT(info2->Next(snapshot), Eq(nullptr)); - CordzInfo::UntrackCord(info2); + info2->Untrack(); ASSERT_THAT(CordzInfo::Head(snapshot), Eq(nullptr)); } @@ -208,7 +208,7 @@ TEST(CordzInfoTest, StackV2) { // got_stack. EXPECT_THAT(got_stack, HasSubstr(expected_stack)); - CordzInfo::UntrackCord(info); + info->Untrack(); } // Local helper functions to get different stacks for child and parent. @@ -231,7 +231,7 @@ TEST(CordzInfoTest, GetStatistics) { EXPECT_THAT(statistics.parent_method, Eq(kUnknownMethod)); EXPECT_THAT(statistics.update_tracker.Value(kTrackCordMethod), Eq(1)); - CordzInfo::UntrackCord(info); + info->Untrack(); } TEST(CordzInfoTest, LockCountsMethod) { @@ -246,7 +246,7 @@ TEST(CordzInfoTest, LockCountsMethod) { CordzStatistics statistics = info->GetCordzStatistics(); EXPECT_THAT(statistics.update_tracker.Value(kUpdateMethod), Eq(2)); - CordzInfo::UntrackCord(info); + info->Untrack(); } TEST(CordzInfoTest, FromParent) { @@ -265,8 +265,8 @@ TEST(CordzInfoTest, FromParent) { EXPECT_THAT(statistics.parent_method, Eq(kTrackCordMethod)); EXPECT_THAT(statistics.update_tracker.Value(kChildMethod), Eq(1)); - CordzInfo::UntrackCord(info_parent); - CordzInfo::UntrackCord(info_child); + info_parent->Untrack(); + info_child->Untrack(); } TEST(CordzInfoTest, FromParentInlined) { @@ -279,7 +279,7 @@ TEST(CordzInfoTest, FromParentInlined) { EXPECT_THAT(statistics.method, Eq(kChildMethod)); EXPECT_THAT(statistics.parent_method, Eq(kUnknownMethod)); EXPECT_THAT(statistics.update_tracker.Value(kChildMethod), Eq(1)); - CordzInfo::UntrackCord(info); + info->Untrack(); } TEST(CordzInfoTest, RecordMetrics) { @@ -293,7 +293,7 @@ TEST(CordzInfoTest, RecordMetrics) { CordzStatistics actual = info->GetCordzStatistics(); EXPECT_EQ(actual.size, expected.size); - CordzInfo::UntrackCord(info); + info->Untrack(); } } // namespace diff --git a/absl/strings/internal/cordz_sample_token_test.cc b/absl/strings/internal/cordz_sample_token_test.cc index 80f41ca1..9f54301d 100644 --- a/absl/strings/internal/cordz_sample_token_test.cc +++ b/absl/strings/internal/cordz_sample_token_test.cc @@ -96,9 +96,9 @@ TEST(CordzSampleTokenTest, Iterator) { EXPECT_THAT(found, ElementsAre(info3, info2, info1)); - CordzInfo::UntrackCord(info1); - CordzInfo::UntrackCord(info2); - CordzInfo::UntrackCord(info3); + info1->Untrack(); + info2->Untrack(); + info3->Untrack(); } TEST(CordzSampleTokenTest, IteratorEquality) { @@ -135,9 +135,9 @@ TEST(CordzSampleTokenTest, IteratorEquality) { // Both lhs and rhs are done, so they are on nullptr. EXPECT_THAT(lhs, Eq(rhs)); - CordzInfo::UntrackCord(info1); - CordzInfo::UntrackCord(info2); - CordzInfo::UntrackCord(info3); + info1->Untrack(); + info2->Untrack(); + info3->Untrack(); } TEST(CordzSampleTokenTest, MultiThreaded) { @@ -166,7 +166,7 @@ TEST(CordzSampleTokenTest, MultiThreaded) { // Track/untrack. if (cord.data.is_profiled()) { // 1) Untrack - CordzInfo::UntrackCord(cord.data.cordz_info()); + cord.data.cordz_info()->Untrack(); cord.data.clear_cordz_info();; } else { // 2) Track @@ -192,9 +192,7 @@ TEST(CordzSampleTokenTest, MultiThreaded) { } } for (TestCordData& cord : cords) { - if (cord.data.is_profiled()) { - CordzInfo::UntrackCord(cord.data.cordz_info()); - } + CordzInfo::MaybeUntrackCord(cord.data.cordz_info()); } }); } diff --git a/absl/strings/internal/cordz_update_scope.h b/absl/strings/internal/cordz_update_scope.h index 018359a8..57ba75de 100644 --- a/absl/strings/internal/cordz_update_scope.h +++ b/absl/strings/internal/cordz_update_scope.h @@ -40,6 +40,12 @@ class ABSL_SCOPED_LOCKABLE CordzUpdateScope { } } + // CordzUpdateScope can not be copied or assigned to. + CordzUpdateScope(CordzUpdateScope&& rhs) = delete; + CordzUpdateScope(const CordzUpdateScope&) = delete; + CordzUpdateScope& operator=(CordzUpdateScope&& rhs) = delete; + CordzUpdateScope& operator=(const CordzUpdateScope&) = delete; + ~CordzUpdateScope() ABSL_UNLOCK_FUNCTION() { if (ABSL_PREDICT_FALSE(info_)) { info_->Unlock(); @@ -55,7 +61,7 @@ class ABSL_SCOPED_LOCKABLE CordzUpdateScope { CordzInfo* info() const { return info_; } private: - CordzInfo* const info_; + CordzInfo* info_; }; } // namespace cord_internal diff --git a/absl/synchronization/internal/waiter.cc b/absl/synchronization/internal/waiter.cc index 2123be60..28ef311e 100644 --- a/absl/synchronization/internal/waiter.cc +++ b/absl/synchronization/internal/waiter.cc @@ -79,6 +79,7 @@ bool Waiter::Wait(KernelTimeout t) { // Note that, since the thread ticker is just reset, we don't need to check // whether the thread is idle on the very first pass of the loop. bool first_pass = true; + while (true) { int32_t x = futex_.load(std::memory_order_relaxed); while (x != 0) { @@ -90,7 +91,6 @@ bool Waiter::Wait(KernelTimeout t) { return true; // Consumed a wakeup, we are done. } - if (!first_pass) MaybeBecomeIdle(); const int err = Futex::WaitUntil(&futex_, 0, t); if (err != 0) { -- cgit v1.2.3 From 5dd240724366295970c613ed23d0092bcf392f18 Mon Sep 17 00:00:00 2001 From: Abseil Team Date: Fri, 30 Apr 2021 10:41:56 -0700 Subject: Export of internal Abseil changes -- 60b8e77be4bab1bbd3b4c3b70054879229634511 by Derek Mauro : 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 : Fix the estimated memory size for CordRepExternal PiperOrigin-RevId: 371350380 -- eaaa1d8a167aeca67a2aa3a098a2b61a9d72172f by Martijn Vels : 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 : 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 : 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 : 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 : 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 : Add Cordz instrumentation to CordReader PiperOrigin-RevId: 370990181 GitOrigin-RevId: 60b8e77be4bab1bbd3b4c3b70054879229634511 Change-Id: I96af62e6f1a643e8b1228ae01e6c84e33706bb05 --- absl/memory/memory.h | 2 +- absl/meta/type_traits.h | 2 +- absl/strings/BUILD.bazel | 1 + absl/strings/CMakeLists.txt | 1 + absl/strings/cord.cc | 154 +++++++++++---------- absl/strings/cord.h | 77 ++++------- absl/strings/cord_ring_test.cc | 47 ++++++- absl/strings/cord_test.cc | 9 +- absl/strings/cord_test_helpers.h | 23 +++ absl/strings/cordz_test.cc | 133 ++++++++++++++++-- absl/strings/internal/cordz_handle.cc | 7 +- absl/strings/internal/cordz_handle.h | 13 +- absl/strings/internal/cordz_handle_test.cc | 13 +- absl/strings/internal/cordz_info.cc | 21 ++- absl/strings/internal/cordz_info.h | 22 ++- absl/strings/internal/cordz_info_test.cc | 24 +++- absl/strings/internal/cordz_update_tracker.h | 2 + absl/strings/internal/cordz_update_tracker_test.cc | 2 + 18 files changed, 394 insertions(+), 159 deletions(-) (limited to 'absl/strings/internal/cordz_handle.h') 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 { // // 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 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::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 struct result_of; template 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*>( + 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) + 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 > -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*>( - absl::cord_internal::NewExternalRep( - original_data, StringReleaser{std::forward(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(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 > -Cord& Cord::operator=(T&& src) { - if (src.size() <= kMaxBytesToCopy) { - *this = absl::string_view(src); - } else { - *this = Cord(std::forward(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(src))); + CordRep* rep = CordRepFromString(std::forward(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(src))); + CordRep* rep = CordRepFromString(std::forward(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 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 H HashFragmented(H hash_state) const { @@ -994,8 +988,11 @@ inline CordRep* NewExternalRep(absl::string_view data, template Cord MakeCordFromExternal(absl::string_view data, Releaser&& releaser) { Cord cord; - cord.contents_.set_tree(::absl::cord_internal::NewExternalRep( - data, std::forward(releaser))); + if (auto* rep = ::absl::cord_internal::NewExternalRep( + data, std::forward(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 constexpr Cord::Cord(strings_internal::StringConstant) : contents_(strings_internal::StringConstant::value.size() <= @@ -1187,6 +1152,15 @@ inline Cord& Cord::operator=(const Cord& x) { return *this; } +template > +Cord& Cord::operator=(T&& src) { + if (src.size() <= cord_internal::kMaxBytesToCopy) { + return operator=(absl::string_view(src)); + } else { + return AssignLargeString(std::forward(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; // 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 #include +#include +#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 { Cord cord_{MakeString(GetParam())}; }; +template +std::string ParamToString(::testing::TestParamInfo 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 { + private: + CordzSamplingIntervalHelper sample_every_{1}; +}; + +INSTANTIATE_TEST_SUITE_P(WithParam, CordzStringTest, + testing::Values(TestCordSize::kInlined, + TestCordSize::kStringSso1, + TestCordSize::kStringSso2, + TestCordSize::kSmall, + TestCordSize::kLarge), + ParamToString); + +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 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, -- cgit v1.2.3