Export of internal Abseil changes
--
9fc37c11b9e46287acef00ee06ed9adcba54dd13 by Greg Falcon <gfalcon@google.com>:
Rename absl::hash_internal::HashState to absl::hash_internal::MixingHashState.
Before this change, we had two classes named HashState: absl::HashState, the public API used for type erasure, and absl::hash_internal::HashState, the internal concrete implementation ordinarily used.
The internal class used to be named `CityHashState`, but we renamed it to `HashState` it when we changed underlying hash implementation to wyhash. This inadvertent naming conflict made the code much harder to read, and this change intends to undo that.
PiperOrigin-RevId: 373481959
--
4aec55ffddebd085c239352a2e20721091f719a1 by Greg Falcon <gfalcon@google.com>:
Introduce absl::HashOf(), a convenience wrapper around absl::Hash that calculates hashes from the values of its arguments.
PiperOrigin-RevId: 373461406
--
86b5fd8db50bbc8bd0aa9258523527381fe0445d by Abseil Team <absl-team@google.com>:
Improve speed of BlockingCounter by making its most common path lock free.
With the new implementation, the fast path of BlockingCounter::DecrementCount()
is only a fetch_sub operation. This is most times much more efficient than the
previous implementation (full mutex lock/unlock). As a matter of fact, in most
actual usecases in practice, the waiter thread is already waiting on the
Wait() call when DecrementCount() is called, which makes Mutex::Unlock() take
the slow path as there's a waiter thread that it might need to wake up.
PiperOrigin-RevId: 373394164
--
65c876be5eac0cd32583ff8535ede4109d39cf3f by Martijn Vels <mvels@google.com>:
Move the 'sample copied cord' logic into MaybeTrackCord(),
This changes move the logic for selecting if a cord should remain being sampled from Cord to CordzInfo::MaybeTrackCord, and updates the documentation for the latter method.
PiperOrigin-RevId: 373363168
--
e84410bd0aada293a81dfb82656c952e209e21fb by Martijn Vels <mvels@google.com>:
Add check for the first call to cordz_should_profile() for each thread.
This prevents the first cord of a newly created thread to be always sampled, which is a 'bad' kind of determinism for sampling.
PiperOrigin-RevId: 373229768
--
bf09c589dc099ac8f4af780bf7e609c53c27574c by Samuel Benzaquen <sbenza@google.com>:
Refactor the Flags structure into an enum.
This gives us more control over the representation and allows for easier
merging during parsing.
PiperOrigin-RevId: 373163038
--
b947b0c51083b7b6508284b5d31819596c91729e by Derek Mauro <dmauro@google.com>:
Fixes warnings about shadowed variables
Fixes #956
PiperOrigin-RevId: 373158133
GitOrigin-RevId: 9fc37c11b9e46287acef00ee06ed9adcba54dd13
Change-Id: I91f35699f9bf439d1a870c6493946a310afe088c
diff --git a/absl/copts/GENERATED_AbseilCopts.cmake b/absl/copts/GENERATED_AbseilCopts.cmake
index 18a1e5c..22a25eb 100644
--- a/absl/copts/GENERATED_AbseilCopts.cmake
+++ b/absl/copts/GENERATED_AbseilCopts.cmake
@@ -77,7 +77,7 @@
"-Woverlength-strings"
"-Wpointer-arith"
"-Wself-assign"
- "-Wshadow"
+ "-Wshadow-all"
"-Wstring-conversion"
"-Wtautological-overlap-compare"
"-Wundef"
diff --git a/absl/copts/GENERATED_copts.bzl b/absl/copts/GENERATED_copts.bzl
index d2bd560..0c7a91b 100644
--- a/absl/copts/GENERATED_copts.bzl
+++ b/absl/copts/GENERATED_copts.bzl
@@ -78,7 +78,7 @@
"-Woverlength-strings",
"-Wpointer-arith",
"-Wself-assign",
- "-Wshadow",
+ "-Wshadow-all",
"-Wstring-conversion",
"-Wtautological-overlap-compare",
"-Wundef",
diff --git a/absl/copts/copts.py b/absl/copts/copts.py
index ce30df8..7268c68 100644
--- a/absl/copts/copts.py
+++ b/absl/copts/copts.py
@@ -93,7 +93,7 @@
"-Woverlength-strings",
"-Wpointer-arith",
"-Wself-assign",
- "-Wshadow",
+ "-Wshadow-all",
"-Wstring-conversion",
"-Wtautological-overlap-compare",
"-Wundef",
diff --git a/absl/hash/hash.h b/absl/hash/hash.h
index 5de132c..69c44fd 100644
--- a/absl/hash/hash.h
+++ b/absl/hash/hash.h
@@ -73,6 +73,8 @@
#ifndef ABSL_HASH_HASH_H_
#define ABSL_HASH_HASH_H_
+#include <tuple>
+
#include "absl/hash/internal/hash.h"
namespace absl {
@@ -214,6 +216,26 @@
template <typename T>
using Hash = absl::hash_internal::Hash<T>;
+// HashOf
+//
+// absl::HashOf() is a helper that generates a hash from the values of its
+// arguments. It dispatches to absl::Hash directly, as follows:
+// * HashOf(t) == absl::Hash<T>{}(t)
+// * HashOf(a, b, c) == HashOf(std::make_tuple(a, b, c))
+//
+// HashOf(a1, a2, ...) == HashOf(b1, b2, ...) is guaranteed when
+// * The argument lists have pairwise identical C++ types
+// * a1 == b1 && a2 == b2 && ...
+//
+// The requirement that the arguments match in both type and value is critical.
+// It means that `a == b` does not necessarily imply `HashOf(a) == HashOf(b)` if
+// `a` and `b` have different types. For example, `HashOf(2) != HashOf(2.0)`.
+template <typename... Types>
+size_t HashOf(const Types&... values) {
+ auto tuple = std::tie(values...);
+ return absl::Hash<decltype(tuple)>{}(tuple);
+}
+
// HashState
//
// A type erased version of the hash state concept, for use in user-defined
diff --git a/absl/hash/hash_test.cc b/absl/hash/hash_test.cc
index 1d2e6cf..02b7733 100644
--- a/absl/hash/hash_test.cc
+++ b/absl/hash/hash_test.cc
@@ -973,4 +973,26 @@
absl::Hash<ValueWithBoolConversion>()(ValueWithBoolConversion{1}));
}
+TEST(HashOf, MatchesHashForSingleArgument) {
+ std::string s = "forty two";
+ int i = 42;
+ double d = 42.0;
+ std::tuple<int, int> t{4, 2};
+
+ EXPECT_EQ(absl::HashOf(s), absl::Hash<std::string>{}(s));
+ EXPECT_EQ(absl::HashOf(i), absl::Hash<int>{}(i));
+ EXPECT_EQ(absl::HashOf(d), absl::Hash<double>{}(d));
+ EXPECT_EQ(absl::HashOf(t), (absl::Hash<std::tuple<int, int>>{}(t)));
+}
+
+TEST(HashOf, MatchesHashOfTupleForMultipleArguments) {
+ std::string hello = "hello";
+ std::string world = "world";
+
+ EXPECT_EQ(absl::HashOf(), absl::HashOf(std::make_tuple()));
+ EXPECT_EQ(absl::HashOf(hello), absl::HashOf(std::make_tuple(hello)));
+ EXPECT_EQ(absl::HashOf(hello, world),
+ absl::HashOf(std::make_tuple(hello, world)));
+}
+
} // namespace
diff --git a/absl/hash/internal/hash.cc b/absl/hash/internal/hash.cc
index 1433eb9..06f53a5 100644
--- a/absl/hash/internal/hash.cc
+++ b/absl/hash/internal/hash.cc
@@ -18,9 +18,8 @@
ABSL_NAMESPACE_BEGIN
namespace hash_internal {
-uint64_t HashState::CombineLargeContiguousImpl32(uint64_t state,
- const unsigned char* first,
- size_t len) {
+uint64_t MixingHashState::CombineLargeContiguousImpl32(
+ uint64_t state, const unsigned char* first, size_t len) {
while (len >= PiecewiseChunkSize()) {
state =
Mix(state, absl::hash_internal::CityHash32(reinterpret_cast<const char*>(first),
@@ -33,9 +32,8 @@
std::integral_constant<int, 4>{});
}
-uint64_t HashState::CombineLargeContiguousImpl64(uint64_t state,
- const unsigned char* first,
- size_t len) {
+uint64_t MixingHashState::CombineLargeContiguousImpl64(
+ uint64_t state, const unsigned char* first, size_t len) {
while (len >= PiecewiseChunkSize()) {
state = Mix(state, Hash64(first, PiecewiseChunkSize()));
len -= PiecewiseChunkSize();
@@ -46,7 +44,7 @@
std::integral_constant<int, 8>{});
}
-ABSL_CONST_INIT const void* const HashState::kSeed = &kSeed;
+ABSL_CONST_INIT const void* const MixingHashState::kSeed = &kSeed;
// The salt array used by Wyhash. This array is NOT the mechanism used to make
// absl::Hash non-deterministic between program invocations. See `Seed()` for
@@ -61,7 +59,7 @@
uint64_t{0x452821E638D01377},
};
-uint64_t HashState::WyhashImpl(const unsigned char* data, size_t len) {
+uint64_t MixingHashState::WyhashImpl(const unsigned char* data, size_t len) {
return Wyhash(data, len, Seed(), kWyhashSalt);
}
diff --git a/absl/hash/internal/hash.h b/absl/hash/internal/hash.h
index 7fb0af0..69dbbc6 100644
--- a/absl/hash/internal/hash.h
+++ b/absl/hash/internal/hash.h
@@ -379,7 +379,7 @@
// This SFINAE gets MSVC confused under some conditions. Let's just disable it
// for now.
H
-#else // _MSC_VER
+#else // _MSC_VER
typename std::enable_if<absl::conjunction<is_hashable<Ts>...>::value, H>::type
#endif // _MSC_VER
AbslHashValue(H hash_state, const std::tuple<Ts...>& t) {
@@ -714,8 +714,8 @@
struct is_hashable
: std::integral_constant<bool, HashSelect::template Apply<T>::value> {};
-// HashState
-class ABSL_DLL HashState : public HashStateBase<HashState> {
+// MixingHashState
+class ABSL_DLL MixingHashState : public HashStateBase<MixingHashState> {
// absl::uint128 is not an alias or a thin wrapper around the intrinsic.
// We use the intrinsic when available to improve performance.
#ifdef ABSL_HAVE_INTRINSIC_INT128
@@ -734,22 +734,23 @@
public:
// Move only
- HashState(HashState&&) = default;
- HashState& operator=(HashState&&) = default;
+ MixingHashState(MixingHashState&&) = default;
+ MixingHashState& operator=(MixingHashState&&) = default;
- // HashState::combine_contiguous()
+ // MixingHashState::combine_contiguous()
//
// Fundamental base case for hash recursion: mixes the given range of bytes
// into the hash state.
- static HashState combine_contiguous(HashState hash_state,
- const unsigned char* first, size_t size) {
- return HashState(
+ static MixingHashState combine_contiguous(MixingHashState hash_state,
+ const unsigned char* first,
+ size_t size) {
+ return MixingHashState(
CombineContiguousImpl(hash_state.state_, first, size,
std::integral_constant<int, sizeof(size_t)>{}));
}
- using HashState::HashStateBase::combine_contiguous;
+ using MixingHashState::HashStateBase::combine_contiguous;
- // HashState::hash()
+ // MixingHashState::hash()
//
// For performance reasons in non-opt mode, we specialize this for
// integral types.
@@ -761,24 +762,24 @@
return static_cast<size_t>(Mix(Seed(), static_cast<uint64_t>(value)));
}
- // Overload of HashState::hash()
+ // Overload of MixingHashState::hash()
template <typename T, absl::enable_if_t<!IntegralFastPath<T>::value, int> = 0>
static size_t hash(const T& value) {
- return static_cast<size_t>(combine(HashState{}, value).state_);
+ return static_cast<size_t>(combine(MixingHashState{}, value).state_);
}
private:
// Invoked only once for a given argument; that plus the fact that this is
// move-only ensures that there is only one non-moved-from object.
- HashState() : state_(Seed()) {}
+ MixingHashState() : state_(Seed()) {}
// Workaround for MSVC bug.
// We make the type copyable to fix the calling convention, even though we
// never actually copy it. Keep it private to not affect the public API of the
// type.
- HashState(const HashState&) = default;
+ MixingHashState(const MixingHashState&) = default;
- explicit HashState(uint64_t state) : state_(state) {}
+ explicit MixingHashState(uint64_t state) : state_(state) {}
// Implementation of the base case for combine_contiguous where we actually
// mix the bytes into the state.
@@ -793,7 +794,6 @@
std::integral_constant<int, 8>
/* sizeof_size_t */);
-
// Slow dispatch path for calls to CombineContiguousImpl with a size argument
// larger than PiecewiseChunkSize(). Has the same effect as calling
// CombineContiguousImpl() repeatedly with the chunk stride size.
@@ -911,8 +911,8 @@
uint64_t state_;
};
-// HashState::CombineContiguousImpl()
-inline uint64_t HashState::CombineContiguousImpl(
+// MixingHashState::CombineContiguousImpl()
+inline uint64_t MixingHashState::CombineContiguousImpl(
uint64_t state, const unsigned char* first, size_t len,
std::integral_constant<int, 4> /* sizeof_size_t */) {
// For large values we use CityHash, for small ones we just use a
@@ -934,8 +934,8 @@
return Mix(state, v);
}
-// Overload of HashState::CombineContiguousImpl()
-inline uint64_t HashState::CombineContiguousImpl(
+// Overload of MixingHashState::CombineContiguousImpl()
+inline uint64_t MixingHashState::CombineContiguousImpl(
uint64_t state, const unsigned char* first, size_t len,
std::integral_constant<int, 8> /* sizeof_size_t */) {
// For large values we use Wyhash or CityHash depending on the platform, for
@@ -976,7 +976,9 @@
template <typename T>
struct HashImpl {
- size_t operator()(const T& value) const { return HashState::hash(value); }
+ size_t operator()(const T& value) const {
+ return MixingHashState::hash(value);
+ }
};
template <typename T>
diff --git a/absl/status/internal/status_internal.h b/absl/status/internal/status_internal.h
index ccafd70..ac12940 100644
--- a/absl/status/internal/status_internal.h
+++ b/absl/status/internal/status_internal.h
@@ -47,12 +47,12 @@
// Reference-counted representation of Status data.
struct StatusRep {
- StatusRep(absl::StatusCode code, absl::string_view message,
- std::unique_ptr<status_internal::Payloads> payloads)
+ StatusRep(absl::StatusCode code_arg, absl::string_view message_arg,
+ std::unique_ptr<status_internal::Payloads> payloads_arg)
: ref(int32_t{1}),
- code(code),
- message(message),
- payloads(std::move(payloads)) {}
+ code(code_arg),
+ message(message_arg),
+ payloads(std::move(payloads_arg)) {}
std::atomic<int32_t> ref;
absl::StatusCode code;
diff --git a/absl/strings/cord.cc b/absl/strings/cord.cc
index 5dad781..f5aa6e4 100644
--- a/absl/strings/cord.cc
+++ b/absl/strings/cord.cc
@@ -536,24 +536,10 @@
return;
}
- // See b/187581164: unsample cord if already sampled
- // TODO(b/117940323): continuously 'assigned to' cords would reach 100%
- // sampling probability. Imagine a cord x in some cache:
- // cache.SetCord(const Cord& foo) {
- // x = foo;
- // }
- // CordzInfo::MaybeTrackCord does:
- // x.profiled = foo.profiled | x.profiled | random(cordz_mean_interval)
- // Which means it will on the long run converge to 'always samples'
- // The real fix is in CordzMaybeTrackCord, but the below is a low risk
- // forward fix for b/187581164 and similar BT benchmark regressions.
- if (ABSL_PREDICT_FALSE(is_profiled())) {
- cordz_info()->Untrack();
- clear_cordz_info();
- }
-
CordRep* tree = as_tree();
if (CordRep* src_tree = src.tree()) {
+ // Leave any existing `cordz_info` in place, and let MaybeTrackCord()
+ // decide if this cord should be (or remains to be) sampled or not.
data_.set_tree(CordRep::Ref(src_tree));
CordzInfo::MaybeTrackCord(data_, src.data_, method);
} else {
diff --git a/absl/strings/cordz_test.cc b/absl/strings/cordz_test.cc
index 0e11f5c..2b7d30b 100644
--- a/absl/strings/cordz_test.cc
+++ b/absl/strings/cordz_test.cc
@@ -69,6 +69,7 @@
// Returns a cord with a sampled method of kAppendString.
absl::Cord MakeAppendStringCord(TestCordSize size) {
+ CordzSamplingIntervalHelper always(1);
absl::Cord cord;
cord.Append(MakeString(size));
return cord;
@@ -136,21 +137,21 @@
}
}
-TEST(CordzTest, CopyConstruct) {
+TEST(CordzTest, CopyConstructFromUnsampled) {
CordzSamplingIntervalHelper sample_every{1};
Cord src = UnsampledCord(MakeString(TestCordSize::kLarge));
Cord cord(src);
- EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorCord));
+ EXPECT_THAT(GetCordzInfoForTesting(cord), Eq(nullptr));
}
TEST(CordzTest, CopyConstructFromSampled) {
- CordzSamplingIntervalHelper sample_every{1};
- Cord src(MakeString(TestCordSize::kLarge));
+ CordzSamplingIntervalHelper sample_never{99999};
+ Cord src = MakeAppendStringCord(TestCordSize::kLarge);
Cord cord(src);
ASSERT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorCord));
CordzStatistics stats = GetCordzInfoForTesting(cord)->GetCordzStatistics();
- EXPECT_THAT(stats.parent_method, Eq(Method::kConstructorString));
- EXPECT_THAT(stats.update_tracker.Value(Method::kConstructorString), Eq(1));
+ EXPECT_THAT(stats.parent_method, Eq(Method::kAppendString));
+ EXPECT_THAT(stats.update_tracker.Value(Method::kAppendString), Eq(1));
}
TEST(CordzTest, MoveConstruct) {
@@ -160,12 +161,12 @@
EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kConstructorString));
}
-TEST_P(CordzUpdateTest, AssignCord) {
+TEST_P(CordzUpdateTest, AssignUnsampledCord) {
Cord src = UnsampledCord(MakeString(TestCordSize::kLarge));
+ const CordzInfo* info = GetCordzInfoForTesting(cord());
cord() = src;
- EXPECT_THAT(cord(), HasValidCordzInfoOf(Method::kAssignCord));
- CordzStatistics stats = GetCordzInfoForTesting(cord())->GetCordzStatistics();
- EXPECT_THAT(stats.update_tracker.Value(Method::kConstructorString), Eq(0));
+ EXPECT_THAT(GetCordzInfoForTesting(cord()), Eq(nullptr));
+ EXPECT_FALSE(CordzInfoIsListed(info));
}
TEST_P(CordzUpdateTest, AssignSampledCord) {
@@ -178,10 +179,10 @@
EXPECT_THAT(stats.update_tracker.Value(Method::kConstructorString), Eq(0));
}
-TEST(CordzUpdateTest, AssignSampledCordToUnsampledCord) {
- CordzSamplingIntervalHelper sample_every{1};
+TEST(CordzUpdateTest, AssignSampledCordToInlined) {
+ CordzSamplingIntervalHelper sample_never{99999};
+ Cord cord;
Cord src = MakeAppendStringCord(TestCordSize::kLarge);
- Cord cord = UnsampledCord(MakeString(TestCordSize::kLarge));
cord = src;
ASSERT_THAT(cord, HasValidCordzInfoOf(Method::kAssignCord));
CordzStatistics stats = GetCordzInfoForTesting(cord)->GetCordzStatistics();
@@ -190,6 +191,38 @@
EXPECT_THAT(stats.update_tracker.Value(Method::kConstructorString), Eq(0));
}
+TEST(CordzUpdateTest, AssignSampledCordToUnsampledCord) {
+ CordzSamplingIntervalHelper sample_never{99999};
+ Cord cord = UnsampledCord(MakeString(TestCordSize::kLarge));
+ Cord src = MakeAppendStringCord(TestCordSize::kLarge);
+ cord = src;
+ ASSERT_THAT(cord, HasValidCordzInfoOf(Method::kAssignCord));
+ CordzStatistics stats = GetCordzInfoForTesting(cord)->GetCordzStatistics();
+ EXPECT_THAT(stats.parent_method, Eq(Method::kAppendString));
+ EXPECT_THAT(stats.update_tracker.Value(Method::kAppendString), Eq(1));
+ EXPECT_THAT(stats.update_tracker.Value(Method::kConstructorString), Eq(0));
+}
+
+TEST(CordzUpdateTest, AssignUnsampledCordToSampledCordWithoutSampling) {
+ CordzSamplingIntervalHelper sample_never{99999};
+ Cord cord = MakeAppendStringCord(TestCordSize::kLarge);
+ const CordzInfo* info = GetCordzInfoForTesting(cord);
+ Cord src = UnsampledCord(MakeString(TestCordSize::kLarge));
+ cord = src;
+ EXPECT_THAT(GetCordzInfoForTesting(cord), Eq(nullptr));
+ EXPECT_FALSE(CordzInfoIsListed(info));
+}
+
+TEST(CordzUpdateTest, AssignUnsampledCordToSampledCordWithSampling) {
+ CordzSamplingIntervalHelper sample_every{1};
+ Cord cord = MakeAppendStringCord(TestCordSize::kLarge);
+ const CordzInfo* info = GetCordzInfoForTesting(cord);
+ Cord src = UnsampledCord(MakeString(TestCordSize::kLarge));
+ cord = src;
+ EXPECT_THAT(GetCordzInfoForTesting(cord), Eq(nullptr));
+ EXPECT_FALSE(CordzInfoIsListed(info));
+}
+
TEST(CordzUpdateTest, AssignSampledCordToSampledCord) {
CordzSamplingIntervalHelper sample_every{1};
Cord src = MakeAppendStringCord(TestCordSize::kLarge);
@@ -202,7 +235,19 @@
EXPECT_THAT(stats.update_tracker.Value(Method::kConstructorString), Eq(0));
}
-TEST(CordzTest, AssignInlinedCord) {
+TEST(CordzUpdateTest, AssignUnsampledCordToSampledCord) {
+ CordzSamplingIntervalHelper sample_every{1};
+ Cord src = MakeAppendStringCord(TestCordSize::kLarge);
+ Cord cord(MakeString(TestCordSize::kLarge));
+ cord = src;
+ ASSERT_THAT(cord, HasValidCordzInfoOf(Method::kAssignCord));
+ CordzStatistics stats = GetCordzInfoForTesting(cord)->GetCordzStatistics();
+ EXPECT_THAT(stats.parent_method, Eq(Method::kAppendString));
+ EXPECT_THAT(stats.update_tracker.Value(Method::kAppendString), Eq(1));
+ EXPECT_THAT(stats.update_tracker.Value(Method::kConstructorString), Eq(0));
+}
+
+TEST(CordzTest, AssignInlinedCordToSampledCord) {
CordzSampleToken token;
CordzSamplingIntervalHelper sample_every{1};
Cord cord(MakeString(TestCordSize::kLarge));
@@ -389,28 +434,28 @@
EXPECT_THAT(GetCordzInfoForTesting(cord), Eq(nullptr));
}
-TEST(CordzTest, SubCord) {
+TEST(CordzTest, SubCordFromUnsampledCord) {
CordzSamplingIntervalHelper sample_every{1};
Cord src = UnsampledCord(MakeString(TestCordSize::kLarge));
Cord cord = src.Subcord(10, src.size() / 2);
- EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kSubCord));
-}
-
-TEST(CordzTest, SmallSubCord) {
- CordzSamplingIntervalHelper sample_every{1};
- Cord src = UnsampledCord(MakeString(TestCordSize::kLarge));
- Cord cord = src.Subcord(10, kMaxInline + 1);
- EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kSubCord));
+ EXPECT_THAT(GetCordzInfoForTesting(cord), Eq(nullptr));
}
TEST(CordzTest, SubCordFromSampledCord) {
- CordzSamplingIntervalHelper sample_every{1};
- Cord src(MakeString(TestCordSize::kLarge));
+ CordzSamplingIntervalHelper sample_never{99999};
+ Cord src = MakeAppendStringCord(TestCordSize::kLarge);
Cord cord = src.Subcord(10, src.size() / 2);
- EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kSubCord));
+ ASSERT_THAT(cord, HasValidCordzInfoOf(Method::kSubCord));
CordzStatistics stats = GetCordzInfoForTesting(cord)->GetCordzStatistics();
- EXPECT_THAT(stats.parent_method, Eq(Method::kConstructorString));
- EXPECT_THAT(stats.update_tracker.Value(Method::kConstructorString), Eq(1));
+ EXPECT_THAT(stats.parent_method, Eq(Method::kAppendString));
+ EXPECT_THAT(stats.update_tracker.Value(Method::kAppendString), Eq(1));
+}
+
+TEST(CordzTest, SmallSubCord) {
+ CordzSamplingIntervalHelper sample_never{99999};
+ Cord src = MakeAppendStringCord(TestCordSize::kLarge);
+ Cord cord = src.Subcord(10, kMaxInline + 1);
+ EXPECT_THAT(cord, HasValidCordzInfoOf(Method::kSubCord));
}
} // namespace
diff --git a/absl/strings/internal/cord_rep_ring.cc b/absl/strings/internal/cord_rep_ring.cc
index 0995129..f78c94e 100644
--- a/absl/strings/internal/cord_rep_ring.cc
+++ b/absl/strings/internal/cord_rep_ring.cc
@@ -32,15 +32,6 @@
ABSL_NAMESPACE_BEGIN
namespace cord_internal {
-// See https://bugs.llvm.org/show_bug.cgi?id=48477
-#ifdef __clang__
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wshadow"
-#if __has_warning("-Wshadow-field")
-#pragma clang diagnostic ignored "-Wshadow-field"
-#endif
-#endif
-
namespace {
using index_type = CordRepRing::index_type;
@@ -450,12 +441,12 @@
}
CordRepRing* CordRepRing::CreateFromLeaf(CordRep* child, size_t offset,
- size_t length, size_t extra) {
+ size_t len, size_t extra) {
CordRepRing* rep = CordRepRing::New(1, extra);
rep->head_ = 0;
rep->tail_ = rep->advance(0);
- rep->length = length;
- rep->entry_end_pos()[0] = length;
+ rep->length = len;
+ rep->entry_end_pos()[0] = len;
rep->entry_child()[0] = child;
rep->entry_data_offset()[0] = static_cast<offset_type>(offset);
return Validate(rep);
@@ -463,16 +454,16 @@
CordRepRing* CordRepRing::CreateSlow(CordRep* child, size_t extra) {
CordRepRing* rep = nullptr;
- Consume(child, [&](CordRep* child, size_t offset, size_t length) {
- if (IsFlatOrExternal(child)) {
- rep = rep ? AppendLeaf(rep, child, offset, length)
- : CreateFromLeaf(child, offset, length, extra);
+ Consume(child, [&](CordRep* child_arg, size_t offset, size_t len) {
+ if (IsFlatOrExternal(child_arg)) {
+ rep = rep ? AppendLeaf(rep, child_arg, offset, len)
+ : CreateFromLeaf(child_arg, offset, len, extra);
} else if (rep) {
- rep = AddRing<AddMode::kAppend>(rep, child->ring(), offset, length);
- } else if (offset == 0 && child->length == length) {
- rep = Mutable(child->ring(), extra);
+ rep = AddRing<AddMode::kAppend>(rep, child_arg->ring(), offset, len);
+ } else if (offset == 0 && child_arg->length == len) {
+ rep = Mutable(child_arg->ring(), extra);
} else {
- rep = SubRing(child->ring(), offset, length, extra);
+ rep = SubRing(child_arg->ring(), offset, len, extra);
}
});
return Validate(rep, nullptr, __LINE__);
@@ -491,18 +482,18 @@
template <CordRepRing::AddMode mode>
CordRepRing* CordRepRing::AddRing(CordRepRing* rep, CordRepRing* ring,
- size_t offset, size_t length) {
+ size_t offset, size_t len) {
assert(offset < ring->length);
constexpr bool append = mode == AddMode::kAppend;
Position head = ring->Find(offset);
- Position tail = ring->FindTail(head.index, offset + length);
+ Position tail = ring->FindTail(head.index, offset + len);
const index_type entries = ring->entries(head.index, tail.index);
rep = Mutable(rep, entries);
// The delta for making ring[head].end_pos into 'len - offset'
const pos_type delta_length =
- (append ? rep->begin_pos_ + rep->length : rep->begin_pos_ - length) -
+ (append ? rep->begin_pos_ + rep->length : rep->begin_pos_ - len) -
ring->entry_begin_pos(head.index) - head.offset;
// Start filling at `tail`, or `entries` before `head`
@@ -543,36 +534,36 @@
}
// Commit changes
- rep->length += length;
+ rep->length += len;
if (append) {
rep->tail_ = filler.pos();
} else {
rep->head_ = filler.head();
- rep->begin_pos_ -= length;
+ rep->begin_pos_ -= len;
}
return Validate(rep);
}
CordRepRing* CordRepRing::AppendSlow(CordRepRing* rep, CordRep* child) {
- Consume(child, [&rep](CordRep* child, size_t offset, size_t length) {
- if (child->tag == RING) {
- rep = AddRing<AddMode::kAppend>(rep, child->ring(), offset, length);
+ Consume(child, [&rep](CordRep* child_arg, size_t offset, size_t len) {
+ if (child_arg->tag == RING) {
+ rep = AddRing<AddMode::kAppend>(rep, child_arg->ring(), offset, len);
} else {
- rep = AppendLeaf(rep, child, offset, length);
+ rep = AppendLeaf(rep, child_arg, offset, len);
}
});
return rep;
}
CordRepRing* CordRepRing::AppendLeaf(CordRepRing* rep, CordRep* child,
- size_t offset, size_t length) {
+ size_t offset, size_t len) {
rep = Mutable(rep, 1);
index_type back = rep->tail_;
const pos_type begin_pos = rep->begin_pos_ + rep->length;
rep->tail_ = rep->advance(rep->tail_);
- rep->length += length;
- rep->entry_end_pos()[back] = begin_pos + length;
+ rep->length += len;
+ rep->entry_end_pos()[back] = begin_pos + len;
rep->entry_child()[back] = child;
rep->entry_data_offset()[back] = static_cast<offset_type>(offset);
return Validate(rep, nullptr, __LINE__);
@@ -590,24 +581,24 @@
}
CordRepRing* CordRepRing::PrependSlow(CordRepRing* rep, CordRep* child) {
- RConsume(child, [&](CordRep* child, size_t offset, size_t length) {
- if (IsFlatOrExternal(child)) {
- rep = PrependLeaf(rep, child, offset, length);
+ RConsume(child, [&](CordRep* child_arg, size_t offset, size_t len) {
+ if (IsFlatOrExternal(child_arg)) {
+ rep = PrependLeaf(rep, child_arg, offset, len);
} else {
- rep = AddRing<AddMode::kPrepend>(rep, child->ring(), offset, length);
+ rep = AddRing<AddMode::kPrepend>(rep, child_arg->ring(), offset, len);
}
});
return Validate(rep);
}
CordRepRing* CordRepRing::PrependLeaf(CordRepRing* rep, CordRep* child,
- size_t offset, size_t length) {
+ size_t offset, size_t len) {
rep = Mutable(rep, 1);
index_type head = rep->retreat(rep->head_);
pos_type end_pos = rep->begin_pos_;
rep->head_ = head;
- rep->length += length;
- rep->begin_pos_ -= length;
+ rep->length += len;
+ rep->begin_pos_ -= len;
rep->entry_end_pos()[head] = end_pos;
rep->entry_child()[head] = child;
rep->entry_data_offset()[head] = static_cast<offset_type>(offset);
@@ -787,18 +778,18 @@
}
CordRepRing* CordRepRing::SubRing(CordRepRing* rep, size_t offset,
- size_t length, size_t extra) {
+ size_t len, size_t extra) {
assert(offset <= rep->length);
- assert(offset <= rep->length - length);
+ assert(offset <= rep->length - len);
- if (length == 0) {
+ if (len == 0) {
CordRep::Unref(rep);
return nullptr;
}
// Find position of first byte
Position head = rep->Find(offset);
- Position tail = rep->FindTail(head.index, offset + length);
+ Position tail = rep->FindTail(head.index, offset + len);
const size_t new_entries = rep->entries(head.index, tail.index);
if (rep->refcount.IsOne() && extra <= (rep->capacity() - new_entries)) {
@@ -815,7 +806,7 @@
}
// Adjust begin_pos and length
- rep->length = length;
+ rep->length = len;
rep->begin_pos_ += offset;
// Adjust head and tail blocks
@@ -889,10 +880,6 @@
return Validate(rep);
}
-#ifdef __clang__
-#pragma clang diagnostic pop
-#endif
-
} // namespace cord_internal
ABSL_NAMESPACE_END
} // namespace absl
diff --git a/absl/strings/internal/cord_rep_ring.h b/absl/strings/internal/cord_rep_ring.h
index 830f2b2..2082a56 100644
--- a/absl/strings/internal/cord_rep_ring.h
+++ b/absl/strings/internal/cord_rep_ring.h
@@ -30,15 +30,6 @@
ABSL_NAMESPACE_BEGIN
namespace cord_internal {
-// See https://bugs.llvm.org/show_bug.cgi?id=48477
-#ifdef __clang__
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wshadow"
-#if __has_warning("-Wshadow-field")
-#pragma clang diagnostic ignored "-Wshadow-field"
-#endif
-#endif
-
// All operations modifying a ring buffer are implemented as static methods
// requiring a CordRepRing instance with a reference adopted by the method.
//
@@ -210,23 +201,23 @@
// referencing up to `size` capacity directly before the existing data.
Span<char> GetPrependBuffer(size_t size);
- // Returns a cord ring buffer containing `length` bytes of data starting at
+ // Returns a cord ring buffer containing `len` bytes of data starting at
// `offset`. If the input is not shared, this function will remove all head
// and tail child nodes outside of the requested range, and adjust the new
// head and tail nodes as required. If the input is shared, this function
// returns a new instance sharing some or all of the nodes from the input.
- static CordRepRing* SubRing(CordRepRing* r, size_t offset, size_t length,
+ static CordRepRing* SubRing(CordRepRing* r, size_t offset, size_t len,
size_t extra = 0);
- // Returns a cord ring buffer with the first `length` bytes removed.
+ // Returns a cord ring buffer with the first `len` bytes removed.
// If the input is not shared, this function will remove all head child nodes
// fully inside the first `length` bytes, and adjust the new head as required.
// If the input is shared, this function returns a new instance sharing some
// or all of the nodes from the input.
- static CordRepRing* RemoveSuffix(CordRepRing* r, size_t length,
+ static CordRepRing* RemoveSuffix(CordRepRing* r, size_t len,
size_t extra = 0);
- // Returns a cord ring buffer with the last `length` bytes removed.
+ // Returns a cord ring buffer with the last `len` bytes removed.
// If the input is not shared, this function will remove all head child nodes
// fully inside the first `length` bytes, and adjust the new head as required.
// If the input is shared, this function returns a new instance sharing some
@@ -242,12 +233,12 @@
// function returns false, and `fragment` is left unchanged.
bool IsFlat(absl::string_view* fragment) const;
- // Returns true if the data starting at `offset` with length `length` is
+ // Returns true if the data starting at `offset` with length `len` is
// managed by this instance inside a single contiguous buffer, in which case
// the (optional) output parameter `fragment` is set to the contiguous memory
// starting at offset `offset` with length `length`. Otherwise, the function
// returns false, and `fragment` is left unchanged.
- bool IsFlat(size_t offset, size_t length, absl::string_view* fragment) const;
+ bool IsFlat(size_t offset, size_t len, absl::string_view* fragment) const;
// Testing only: set capacity to requested capacity.
void SetCapacityForTesting(size_t capacity);
@@ -473,10 +464,10 @@
size_t length, size_t extra);
// Appends or prepends (depending on AddMode) the ring buffer in `ring' to
- // `rep` starting at `offset` with length `length`.
+ // `rep` starting at `offset` with length `len`.
template <AddMode mode>
static CordRepRing* AddRing(CordRepRing* rep, CordRepRing* ring,
- size_t offset, size_t length);
+ size_t offset, size_t len);
// Increases the data offset for entry `index` by `n`.
void AddDataOffset(index_type index, size_t n);
@@ -596,12 +587,12 @@
return false;
}
-inline bool CordRepRing::IsFlat(size_t offset, size_t length,
+inline bool CordRepRing::IsFlat(size_t offset, size_t len,
absl::string_view* fragment) const {
const Position pos = Find(offset);
const absl::string_view data = entry_data(pos.index);
- if (data.length() >= length && data.length() - length >= pos.offset) {
- if (fragment) *fragment = data.substr(pos.offset, length);
+ if (data.length() >= len && data.length() - len >= pos.offset) {
+ if (fragment) *fragment = data.substr(pos.offset, len);
return true;
}
return false;
@@ -609,10 +600,6 @@
std::ostream& operator<<(std::ostream& s, const CordRepRing& rep);
-#ifdef __clang__
-#pragma clang diagnostic pop
-#endif
-
} // namespace cord_internal
ABSL_NAMESPACE_END
} // namespace absl
diff --git a/absl/strings/internal/cordz_functions.cc b/absl/strings/internal/cordz_functions.cc
index 6ad864f..f30080f 100644
--- a/absl/strings/internal/cordz_functions.cc
+++ b/absl/strings/internal/cordz_functions.cc
@@ -44,7 +44,10 @@
#ifdef ABSL_INTERNAL_CORDZ_ENABLED
-ABSL_CONST_INIT thread_local int64_t cordz_next_sample = 0;
+// Special negative 'not initialized' per thread value for cordz_next_sample.
+static constexpr int64_t kInitCordzNextSample = -1;
+
+ABSL_CONST_INIT thread_local int64_t cordz_next_sample = kInitCordzNextSample;
// kIntervalIfDisabled is the number of profile-eligible events need to occur
// before the code will confirm that cordz is still disabled.
@@ -77,8 +80,11 @@
}
if (cordz_next_sample <= 0) {
+ // If first check on current thread, check cordz_should_profile()
+ // again using the created (initial) stride in cordz_next_sample.
+ const bool initialized = cordz_next_sample != kInitCordzNextSample;
cordz_next_sample = exponential_biased_generator.GetStride(mean_interval);
- return true;
+ return initialized || cordz_should_profile();
}
--cordz_next_sample;
diff --git a/absl/strings/internal/cordz_functions_test.cc b/absl/strings/internal/cordz_functions_test.cc
index f2cefae..350623c 100644
--- a/absl/strings/internal/cordz_functions_test.cc
+++ b/absl/strings/internal/cordz_functions_test.cc
@@ -14,6 +14,8 @@
#include "absl/strings/internal/cordz_functions.h"
+#include <thread> // NOLINT we need real clean new threads
+
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include "absl/base/config.h"
@@ -63,6 +65,22 @@
set_cordz_mean_interval(orig_sample_rate);
}
+TEST(CordzFunctionsTest, DoesNotAlwaysSampleFirstCord) {
+ // Set large enough interval such that the chance of 'tons' of threads
+ // randomly sampling the first call is infinitely small.
+ set_cordz_mean_interval(10000);
+ int tries = 0;
+ bool sampled = false;
+ do {
+ ++tries;
+ ASSERT_THAT(tries, Le(1000));
+ std::thread thread([&sampled] {
+ sampled = cordz_should_profile();
+ });
+ thread.join();
+ } while (sampled);
+}
+
TEST(CordzFunctionsTest, ShouldProfileRate) {
static constexpr int kDesiredMeanInterval = 1000;
static constexpr int kSamples = 10000;
diff --git a/absl/strings/internal/cordz_info.cc b/absl/strings/internal/cordz_info.cc
index a6b045f..a3a0b9c 100644
--- a/absl/strings/internal/cordz_info.cc
+++ b/absl/strings/internal/cordz_info.cc
@@ -293,6 +293,16 @@
cordz_info->Track();
}
+void CordzInfo::MaybeTrackCordImpl(InlineData& cord, const InlineData& src,
+ MethodIdentifier method) {
+ if (src.is_profiled()) {
+ TrackCord(cord, src, method);
+ } else if (cord.is_profiled()) {
+ cord.cordz_info()->Untrack();
+ cord.clear_cordz_info();
+ }
+}
+
CordzInfo::MethodIdentifier CordzInfo::GetParentMethod(const CordzInfo* src) {
if (src == nullptr) return MethodIdentifier::kUnknown;
return src->parent_method_ != MethodIdentifier::kUnknown ? src->parent_method_
diff --git a/absl/strings/internal/cordz_info.h b/absl/strings/internal/cordz_info.h
index 2923793..13aaee1 100644
--- a/absl/strings/internal/cordz_info.h
+++ b/absl/strings/internal/cordz_info.h
@@ -77,13 +77,31 @@
static void MaybeTrackCord(InlineData& cord, MethodIdentifier method);
// Maybe sample the cord identified by 'cord' for method 'method'.
- // `src` identifies a 'parent' cord which content is copied into the current
- // cord, typically the input cord for an assign emthod or copy constructor.
- // Invokes the corresponding `TrackCord` method if either cord is sampled, or
- // if `cord` is randomly picked for sampling. Possible scenarios:
- // * `src` is sampled: `cord` will be set to sampled if not already sampled.
- // Parent stack and update stats of `src` are copied into `cord`
- // * `src` is not sampled: `cord` may be randomly picked for sampling.
+ // `src` identifies a 'parent' cord which is assigned to `cord`, typically the
+ // input cord for a copy constructor, or an assign method such as `operator=`
+ // `cord` will be sampled if (and only if) `src` is sampled.
+ // If `cord` is currently being sampled and `src` is not being sampled, then
+ // this function will stop sampling the cord and reset the cord's cordz_info.
+ //
+ // Previously this function defined that `cord` will be sampled if either
+ // `src` is sampled, or if `cord` is randomly picked for sampling. However,
+ // this can cause issues, as there may be paths where some cord is assigned an
+ // indirect copy of it's own value. As such a 'string of copies' would then
+ // remain sampled (`src.is_profiled`), then assigning such a cord back to
+ // 'itself' creates a cycle where the cord will converge to 'always sampled`.
+ //
+ // For example:
+ //
+ // Cord x;
+ // for (...) {
+ // // Copy ctor --> y.is_profiled := x.is_profiled | random(...)
+ // Cord y = x;
+ // ...
+ // // Assign x = y --> x.is_profiled = y.is_profiled | random(...)
+ // // ==> x.is_profiled |= random(...)
+ // // ==> x converges to 'always profiled'
+ // x = y;
+ // }
static void MaybeTrackCord(InlineData& cord, const InlineData& src,
MethodIdentifier method);
@@ -201,6 +219,12 @@
#endif
}
+ // Non-inlined implementation of `MaybeTrackCord`, which is executed if
+ // either `src` is sampled or `cord` is sampled, and either untracks or
+ // tracks `cord` as documented per `MaybeTrackCord`.
+ static void MaybeTrackCordImpl(InlineData& cord, const InlineData& src,
+ MethodIdentifier method);
+
ABSL_CONST_INIT static List global_list_;
List* const list_ = &global_list_;
@@ -232,9 +256,8 @@
inline ABSL_ATTRIBUTE_ALWAYS_INLINE void CordzInfo::MaybeTrackCord(
InlineData& cord, const InlineData& src, MethodIdentifier method) {
- if (ABSL_PREDICT_FALSE(InlineData::is_either_profiled(cord, src)) ||
- ABSL_PREDICT_FALSE(cordz_should_profile())) {
- TrackCord(cord, src, method);
+ if (ABSL_PREDICT_FALSE(InlineData::is_either_profiled(cord, src))) {
+ MaybeTrackCordImpl(cord, src, method);
}
}
diff --git a/absl/strings/internal/cordz_info_test.cc b/absl/strings/internal/cordz_info_test.cc
index 59a8c52..b98343a 100644
--- a/absl/strings/internal/cordz_info_test.cc
+++ b/absl/strings/internal/cordz_info_test.cc
@@ -74,19 +74,49 @@
info->Untrack();
}
-TEST(CordzInfoTest, MaybeTrackCordOnSampledCord) {
- TestCordData data1;
- CordzInfo::TrackCord(data1.data, kTrackCordMethod);
- CordzInfo* info1 = data1.data.cordz_info();
- TestCordData data2;
- CordzInfo::MaybeTrackCord(data2.data, data1.data, kTrackCordMethod);
- CordzInfo* info2 = data2.data.cordz_info();
- ASSERT_THAT(info2, Ne(nullptr));
- EXPECT_THAT(info2->GetCordRepForTesting(), Eq(data2.rep.rep));
- info2->Untrack();
- info1->Untrack();
+TEST(CordzInfoTest, MaybeTrackChildCordWithoutSampling) {
+ CordzSamplingIntervalHelper sample_none(99999);
+ TestCordData parent, child;
+ CordzInfo::MaybeTrackCord(child.data, parent.data, kTrackCordMethod);
+ EXPECT_THAT(child.data.cordz_info(), Eq(nullptr));
}
+TEST(CordzInfoTest, MaybeTrackChildCordWithSampling) {
+ CordzSamplingIntervalHelper sample_all(1);
+ TestCordData parent, child;
+ CordzInfo::MaybeTrackCord(child.data, parent.data, kTrackCordMethod);
+ EXPECT_THAT(child.data.cordz_info(), Eq(nullptr));
+}
+
+TEST(CordzInfoTest, MaybeTrackChildCordWithoutSamplingParentSampled) {
+ CordzSamplingIntervalHelper sample_none(99999);
+ TestCordData parent, child;
+ CordzInfo::TrackCord(parent.data, kTrackCordMethod);
+ CordzInfo::MaybeTrackCord(child.data, parent.data, kTrackCordMethod);
+ CordzInfo* parent_info = parent.data.cordz_info();
+ CordzInfo* child_info = child.data.cordz_info();
+ ASSERT_THAT(child_info, Ne(nullptr));
+ EXPECT_THAT(child_info->GetCordRepForTesting(), Eq(child.rep.rep));
+ EXPECT_THAT(child_info->GetParentStack(), parent_info->GetStack());
+ parent_info->Untrack();
+ child_info->Untrack();
+}
+
+TEST(CordzInfoTest, MaybeTrackChildCordWithoutSamplingChildSampled) {
+ CordzSamplingIntervalHelper sample_none(99999);
+ TestCordData parent, child;
+ CordzInfo::TrackCord(child.data, kTrackCordMethod);
+ CordzInfo::MaybeTrackCord(child.data, parent.data, kTrackCordMethod);
+ EXPECT_THAT(child.data.cordz_info(), Eq(nullptr));
+}
+
+TEST(CordzInfoTest, MaybeTrackChildCordWithSamplingChildSampled) {
+ CordzSamplingIntervalHelper sample_all(1);
+ TestCordData parent, child;
+ CordzInfo::TrackCord(child.data, kTrackCordMethod);
+ CordzInfo::MaybeTrackCord(child.data, parent.data, kTrackCordMethod);
+ EXPECT_THAT(child.data.cordz_info(), Eq(nullptr));
+}
TEST(CordzInfoTest, UntrackCord) {
TestCordData data;
diff --git a/absl/strings/internal/str_format/bind.cc b/absl/strings/internal/str_format/bind.cc
index 4e68b90..c988ba8 100644
--- a/absl/strings/internal/str_format/bind.cc
+++ b/absl/strings/internal/str_format/bind.cc
@@ -58,7 +58,7 @@
if (static_cast<size_t>(arg_position - 1) >= pack_.size()) return false;
arg = &pack_[arg_position - 1]; // 1-based
- if (!unbound->flags.basic) {
+ if (unbound->flags != Flags::kBasic) {
int width = unbound->width.value();
bool force_left = false;
if (unbound->width.is_from_arg()) {
@@ -84,9 +84,8 @@
FormatConversionSpecImplFriend::SetPrecision(precision, bound);
if (force_left) {
- Flags flags = unbound->flags;
- flags.left = true;
- FormatConversionSpecImplFriend::SetFlags(flags, bound);
+ FormatConversionSpecImplFriend::SetFlags(unbound->flags | Flags::kLeft,
+ bound);
} else {
FormatConversionSpecImplFriend::SetFlags(unbound->flags, bound);
}
diff --git a/absl/strings/internal/str_format/extension.cc b/absl/strings/internal/str_format/extension.cc
index bb0d96c..484f6eb 100644
--- a/absl/strings/internal/str_format/extension.cc
+++ b/absl/strings/internal/str_format/extension.cc
@@ -23,13 +23,13 @@
ABSL_NAMESPACE_BEGIN
namespace str_format_internal {
-std::string Flags::ToString() const {
+std::string FlagsToString(Flags v) {
std::string s;
- s.append(left ? "-" : "");
- s.append(show_pos ? "+" : "");
- s.append(sign_col ? " " : "");
- s.append(alt ? "#" : "");
- s.append(zero ? "0" : "");
+ s.append(FlagsContains(v, Flags::kLeft) ? "-" : "");
+ s.append(FlagsContains(v, Flags::kShowPos) ? "+" : "");
+ s.append(FlagsContains(v, Flags::kSignCol) ? " " : "");
+ s.append(FlagsContains(v, Flags::kAlt) ? "#" : "");
+ s.append(FlagsContains(v, Flags::kZero) ? "0" : "");
return s;
}
diff --git a/absl/strings/internal/str_format/extension.h b/absl/strings/internal/str_format/extension.h
index a9b9e13..55cbb56 100644
--- a/absl/strings/internal/str_format/extension.h
+++ b/absl/strings/internal/str_format/extension.h
@@ -128,19 +128,33 @@
char buf_[1024];
};
-struct Flags {
- bool basic : 1; // fastest conversion: no flags, width, or precision
- bool left : 1; // "-"
- bool show_pos : 1; // "+"
- bool sign_col : 1; // " "
- bool alt : 1; // "#"
- bool zero : 1; // "0"
- std::string ToString() const;
- friend std::ostream& operator<<(std::ostream& os, const Flags& v) {
- return os << v.ToString();
- }
+enum class Flags : uint8_t {
+ kBasic = 0,
+ kLeft = 1 << 0,
+ kShowPos = 1 << 1,
+ kSignCol = 1 << 2,
+ kAlt = 1 << 3,
+ kZero = 1 << 4,
+ // This is not a real flag. It just exists to turn off kBasic when no other
+ // flags are set. This is for when width/precision are specified.
+ kNonBasic = 1 << 5,
};
+constexpr Flags operator|(Flags a, Flags b) {
+ return static_cast<Flags>(static_cast<uint8_t>(a) | static_cast<uint8_t>(b));
+}
+
+constexpr bool FlagsContains(Flags haystack, Flags needle) {
+ return (static_cast<uint8_t>(haystack) & static_cast<uint8_t>(needle)) ==
+ static_cast<uint8_t>(needle);
+}
+
+std::string FlagsToString(Flags v);
+
+inline std::ostream& operator<<(std::ostream& os, Flags v) {
+ return os << FlagsToString(v);
+}
+
// clang-format off
#define ABSL_INTERNAL_CONVERSION_CHARS_EXPAND_(X_VAL, X_SEP) \
/* text */ \
@@ -257,12 +271,16 @@
class FormatConversionSpecImpl {
public:
// Width and precison are not specified, no flags are set.
- bool is_basic() const { return flags_.basic; }
- bool has_left_flag() const { return flags_.left; }
- bool has_show_pos_flag() const { return flags_.show_pos; }
- bool has_sign_col_flag() const { return flags_.sign_col; }
- bool has_alt_flag() const { return flags_.alt; }
- bool has_zero_flag() const { return flags_.zero; }
+ bool is_basic() const { return flags_ == Flags::kBasic; }
+ bool has_left_flag() const { return FlagsContains(flags_, Flags::kLeft); }
+ bool has_show_pos_flag() const {
+ return FlagsContains(flags_, Flags::kShowPos);
+ }
+ bool has_sign_col_flag() const {
+ return FlagsContains(flags_, Flags::kSignCol);
+ }
+ bool has_alt_flag() const { return FlagsContains(flags_, Flags::kAlt); }
+ bool has_zero_flag() const { return FlagsContains(flags_, Flags::kZero); }
FormatConversionChar conversion_char() const {
// Keep this field first in the struct . It generates better code when
@@ -306,7 +324,7 @@
conv->precision_ = p;
}
static std::string FlagsToString(const FormatConversionSpecImpl& spec) {
- return spec.flags_.ToString();
+ return str_format_internal::FlagsToString(spec.flags_);
}
};
diff --git a/absl/strings/internal/str_format/parser.cc b/absl/strings/internal/str_format/parser.cc
index f308d02..2c9c07d 100644
--- a/absl/strings/internal/str_format/parser.cc
+++ b/absl/strings/internal/str_format/parser.cc
@@ -34,60 +34,67 @@
using CC = FormatConversionCharInternal;
using LM = LengthMod;
+// Abbreviations to fit in the table below.
+constexpr auto f_sign = Flags::kSignCol;
+constexpr auto f_alt = Flags::kAlt;
+constexpr auto f_pos = Flags::kShowPos;
+constexpr auto f_left = Flags::kLeft;
+constexpr auto f_zero = Flags::kZero;
+
ABSL_CONST_INIT const ConvTag kTags[256] = {
- {}, {}, {}, {}, {}, {}, {}, {}, // 00-07
- {}, {}, {}, {}, {}, {}, {}, {}, // 08-0f
- {}, {}, {}, {}, {}, {}, {}, {}, // 10-17
- {}, {}, {}, {}, {}, {}, {}, {}, // 18-1f
- {}, {}, {}, {}, {}, {}, {}, {}, // 20-27
- {}, {}, {}, {}, {}, {}, {}, {}, // 28-2f
- {}, {}, {}, {}, {}, {}, {}, {}, // 30-37
- {}, {}, {}, {}, {}, {}, {}, {}, // 38-3f
- {}, CC::A, {}, {}, {}, CC::E, CC::F, CC::G, // @ABCDEFG
- {}, {}, {}, {}, LM::L, {}, {}, {}, // HIJKLMNO
- {}, {}, {}, {}, {}, {}, {}, {}, // PQRSTUVW
- CC::X, {}, {}, {}, {}, {}, {}, {}, // XYZ[\]^_
- {}, CC::a, {}, CC::c, CC::d, CC::e, CC::f, CC::g, // `abcdefg
- LM::h, CC::i, LM::j, {}, LM::l, {}, CC::n, CC::o, // hijklmno
- CC::p, LM::q, {}, CC::s, LM::t, CC::u, {}, {}, // pqrstuvw
- CC::x, {}, LM::z, {}, {}, {}, {}, {}, // xyz{|}!
- {}, {}, {}, {}, {}, {}, {}, {}, // 80-87
- {}, {}, {}, {}, {}, {}, {}, {}, // 88-8f
- {}, {}, {}, {}, {}, {}, {}, {}, // 90-97
- {}, {}, {}, {}, {}, {}, {}, {}, // 98-9f
- {}, {}, {}, {}, {}, {}, {}, {}, // a0-a7
- {}, {}, {}, {}, {}, {}, {}, {}, // a8-af
- {}, {}, {}, {}, {}, {}, {}, {}, // b0-b7
- {}, {}, {}, {}, {}, {}, {}, {}, // b8-bf
- {}, {}, {}, {}, {}, {}, {}, {}, // c0-c7
- {}, {}, {}, {}, {}, {}, {}, {}, // c8-cf
- {}, {}, {}, {}, {}, {}, {}, {}, // d0-d7
- {}, {}, {}, {}, {}, {}, {}, {}, // d8-df
- {}, {}, {}, {}, {}, {}, {}, {}, // e0-e7
- {}, {}, {}, {}, {}, {}, {}, {}, // e8-ef
- {}, {}, {}, {}, {}, {}, {}, {}, // f0-f7
- {}, {}, {}, {}, {}, {}, {}, {}, // f8-ff
+ {}, {}, {}, {}, {}, {}, {}, {}, // 00-07
+ {}, {}, {}, {}, {}, {}, {}, {}, // 08-0f
+ {}, {}, {}, {}, {}, {}, {}, {}, // 10-17
+ {}, {}, {}, {}, {}, {}, {}, {}, // 18-1f
+ f_sign, {}, {}, f_alt, {}, {}, {}, {}, // !"#$%&'
+ {}, {}, {}, f_pos, {}, f_left, {}, {}, // ()*+,-./
+ f_zero, {}, {}, {}, {}, {}, {}, {}, // 01234567
+ {}, {}, {}, {}, {}, {}, {}, {}, // 89:;<=>?
+ {}, CC::A, {}, {}, {}, CC::E, CC::F, CC::G, // @ABCDEFG
+ {}, {}, {}, {}, LM::L, {}, {}, {}, // HIJKLMNO
+ {}, {}, {}, {}, {}, {}, {}, {}, // PQRSTUVW
+ CC::X, {}, {}, {}, {}, {}, {}, {}, // XYZ[\]^_
+ {}, CC::a, {}, CC::c, CC::d, CC::e, CC::f, CC::g, // `abcdefg
+ LM::h, CC::i, LM::j, {}, LM::l, {}, CC::n, CC::o, // hijklmno
+ CC::p, LM::q, {}, CC::s, LM::t, CC::u, {}, {}, // pqrstuvw
+ CC::x, {}, LM::z, {}, {}, {}, {}, {}, // xyz{|}!
+ {}, {}, {}, {}, {}, {}, {}, {}, // 80-87
+ {}, {}, {}, {}, {}, {}, {}, {}, // 88-8f
+ {}, {}, {}, {}, {}, {}, {}, {}, // 90-97
+ {}, {}, {}, {}, {}, {}, {}, {}, // 98-9f
+ {}, {}, {}, {}, {}, {}, {}, {}, // a0-a7
+ {}, {}, {}, {}, {}, {}, {}, {}, // a8-af
+ {}, {}, {}, {}, {}, {}, {}, {}, // b0-b7
+ {}, {}, {}, {}, {}, {}, {}, {}, // b8-bf
+ {}, {}, {}, {}, {}, {}, {}, {}, // c0-c7
+ {}, {}, {}, {}, {}, {}, {}, {}, // c8-cf
+ {}, {}, {}, {}, {}, {}, {}, {}, // d0-d7
+ {}, {}, {}, {}, {}, {}, {}, {}, // d8-df
+ {}, {}, {}, {}, {}, {}, {}, {}, // e0-e7
+ {}, {}, {}, {}, {}, {}, {}, {}, // e8-ef
+ {}, {}, {}, {}, {}, {}, {}, {}, // f0-f7
+ {}, {}, {}, {}, {}, {}, {}, {}, // f8-ff
};
namespace {
bool CheckFastPathSetting(const UnboundConversion& conv) {
- bool should_be_basic = !conv.flags.left && //
- !conv.flags.show_pos && //
- !conv.flags.sign_col && //
- !conv.flags.alt && //
- !conv.flags.zero && //
- (conv.width.value() == -1) &&
- (conv.precision.value() == -1);
- if (should_be_basic != conv.flags.basic) {
+ bool width_precision_needed =
+ conv.width.value() >= 0 || conv.precision.value() >= 0;
+ if (width_precision_needed && conv.flags == Flags::kBasic) {
fprintf(stderr,
"basic=%d left=%d show_pos=%d sign_col=%d alt=%d zero=%d "
"width=%d precision=%d\n",
- conv.flags.basic, conv.flags.left, conv.flags.show_pos,
- conv.flags.sign_col, conv.flags.alt, conv.flags.zero,
- conv.width.value(), conv.precision.value());
+ conv.flags == Flags::kBasic ? 1 : 0,
+ FlagsContains(conv.flags, Flags::kLeft) ? 1 : 0,
+ FlagsContains(conv.flags, Flags::kShowPos) ? 1 : 0,
+ FlagsContains(conv.flags, Flags::kSignCol) ? 1 : 0,
+ FlagsContains(conv.flags, Flags::kAlt) ? 1 : 0,
+ FlagsContains(conv.flags, Flags::kZero) ? 1 : 0, conv.width.value(),
+ conv.precision.value());
+ return false;
}
- return should_be_basic == conv.flags.basic;
+ return true;
}
template <bool is_positional>
@@ -131,40 +138,21 @@
ABSL_FORMAT_PARSER_INTERNAL_GET_CHAR();
// We should start with the basic flag on.
- assert(conv->flags.basic);
+ assert(conv->flags == Flags::kBasic);
// Any non alpha character makes this conversion not basic.
// This includes flags (-+ #0), width (1-9, *) or precision (.).
// All conversion characters and length modifiers are alpha characters.
if (c < 'A') {
- conv->flags.basic = false;
-
- for (; c <= '0';) {
- // FIXME: We might be able to speed this up reusing the lookup table from
- // above. It might require changing Flags to be a plain integer where we
- // can |= a value.
- switch (c) {
- case '-':
- conv->flags.left = true;
- break;
- case '+':
- conv->flags.show_pos = true;
- break;
- case ' ':
- conv->flags.sign_col = true;
- break;
- case '#':
- conv->flags.alt = true;
- break;
- case '0':
- conv->flags.zero = true;
- break;
- default:
- goto flags_done;
+ while (c <= '0') {
+ auto tag = GetTagForChar(c);
+ if (tag.is_flags()) {
+ conv->flags = conv->flags | tag.as_flags();
+ ABSL_FORMAT_PARSER_INTERNAL_GET_CHAR();
+ } else {
+ break;
}
- ABSL_FORMAT_PARSER_INTERNAL_GET_CHAR();
}
-flags_done:
if (c <= '9') {
if (c >= '0') {
@@ -173,12 +161,12 @@
if (ABSL_PREDICT_FALSE(*next_arg != 0)) return nullptr;
// Positional conversion.
*next_arg = -1;
- conv->flags = Flags();
- conv->flags.basic = true;
return ConsumeConversion<true>(original_pos, end, conv, next_arg);
}
+ conv->flags = conv->flags | Flags::kNonBasic;
conv->width.set_value(maybe_width);
} else if (c == '*') {
+ conv->flags = conv->flags | Flags::kNonBasic;
ABSL_FORMAT_PARSER_INTERNAL_GET_CHAR();
if (is_positional) {
if (ABSL_PREDICT_FALSE(c < '1' || c > '9')) return nullptr;
@@ -192,6 +180,7 @@
}
if (c == '.') {
+ conv->flags = conv->flags | Flags::kNonBasic;
ABSL_FORMAT_PARSER_INTERNAL_GET_CHAR();
if (std::isdigit(c)) {
conv->precision.set_value(parse_digits());
diff --git a/absl/strings/internal/str_format/parser.h b/absl/strings/internal/str_format/parser.h
index 6504dd3..ad8646e 100644
--- a/absl/strings/internal/str_format/parser.h
+++ b/absl/strings/internal/str_format/parser.h
@@ -41,10 +41,7 @@
// The analyzed properties of a single specified conversion.
struct UnboundConversion {
- UnboundConversion()
- : flags() /* This is required to zero all the fields of flags. */ {
- flags.basic = true;
- }
+ UnboundConversion() {}
class InputValue {
public:
@@ -79,7 +76,7 @@
InputValue width;
InputValue precision;
- Flags flags;
+ Flags flags = Flags::kBasic;
LengthMod length_mod = LengthMod::none;
FormatConversionChar conv = FormatConversionCharInternal::kNone;
};
@@ -93,32 +90,43 @@
UnboundConversion* conv, int* next_arg);
// Helper tag class for the table below.
-// It allows fast `char -> ConversionChar/LengthMod` checking and
+// It allows fast `char -> ConversionChar/LengthMod/Flags` checking and
// conversions.
class ConvTag {
public:
constexpr ConvTag(FormatConversionChar conversion_char) // NOLINT
- : tag_(static_cast<int8_t>(conversion_char)) {}
- // We invert the length modifiers to make them negative so that we can easily
- // test for them.
+ : tag_(static_cast<uint8_t>(conversion_char)) {}
constexpr ConvTag(LengthMod length_mod) // NOLINT
- : tag_(~static_cast<std::int8_t>(length_mod)) {}
- // Everything else is -128, which is negative to make is_conv() simpler.
- constexpr ConvTag() : tag_(-128) {}
+ : tag_(0x80 | static_cast<uint8_t>(length_mod)) {}
+ constexpr ConvTag(Flags flags) // NOLINT
+ : tag_(0xc0 | static_cast<uint8_t>(flags)) {}
+ constexpr ConvTag() : tag_(0xFF) {}
- bool is_conv() const { return tag_ >= 0; }
- bool is_length() const { return tag_ < 0 && tag_ != -128; }
+ bool is_conv() const { return (tag_ & 0x80) == 0; }
+ bool is_length() const { return (tag_ & 0xC0) == 0x80; }
+ bool is_flags() const { return (tag_ & 0xE0) == 0xC0; }
+
FormatConversionChar as_conv() const {
assert(is_conv());
+ assert(!is_length());
+ assert(!is_flags());
return static_cast<FormatConversionChar>(tag_);
}
LengthMod as_length() const {
+ assert(!is_conv());
assert(is_length());
- return static_cast<LengthMod>(~tag_);
+ assert(!is_flags());
+ return static_cast<LengthMod>(tag_ & 0x3F);
+ }
+ Flags as_flags() const {
+ assert(!is_conv());
+ assert(!is_length());
+ assert(is_flags());
+ return static_cast<Flags>(tag_ & 0x1F);
}
private:
- std::int8_t tag_;
+ uint8_t tag_;
};
extern const ConvTag kTags[256];
diff --git a/absl/strings/internal/str_format/parser_test.cc b/absl/strings/internal/str_format/parser_test.cc
index a5fa1c7..fe0d296 100644
--- a/absl/strings/internal/str_format/parser_test.cc
+++ b/absl/strings/internal/str_format/parser_test.cc
@@ -270,15 +270,22 @@
for (int k = 0; k < kNumFlags; ++k)
if ((i >> k) & 1) fmt += kAllFlags[k];
// flag order shouldn't matter
- if (rev == 1) { std::reverse(fmt.begin(), fmt.end()); }
+ if (rev == 1) {
+ std::reverse(fmt.begin(), fmt.end());
+ }
fmt += 'd';
SCOPED_TRACE(fmt);
EXPECT_TRUE(Run(fmt.c_str()));
- EXPECT_EQ(fmt.find('-') == std::string::npos, !o.flags.left);
- EXPECT_EQ(fmt.find('+') == std::string::npos, !o.flags.show_pos);
- EXPECT_EQ(fmt.find(' ') == std::string::npos, !o.flags.sign_col);
- EXPECT_EQ(fmt.find('#') == std::string::npos, !o.flags.alt);
- EXPECT_EQ(fmt.find('0') == std::string::npos, !o.flags.zero);
+ EXPECT_EQ(fmt.find('-') == std::string::npos,
+ !FlagsContains(o.flags, Flags::kLeft));
+ EXPECT_EQ(fmt.find('+') == std::string::npos,
+ !FlagsContains(o.flags, Flags::kShowPos));
+ EXPECT_EQ(fmt.find(' ') == std::string::npos,
+ !FlagsContains(o.flags, Flags::kSignCol));
+ EXPECT_EQ(fmt.find('#') == std::string::npos,
+ !FlagsContains(o.flags, Flags::kAlt));
+ EXPECT_EQ(fmt.find('0') == std::string::npos,
+ !FlagsContains(o.flags, Flags::kZero));
}
}
}
@@ -288,14 +295,14 @@
for (const char* fmt : {"d", "llx", "G", "1$X"}) {
SCOPED_TRACE(fmt);
EXPECT_TRUE(Run(fmt));
- EXPECT_TRUE(o.flags.basic);
+ EXPECT_EQ(o.flags, Flags::kBasic);
}
// Flag is off
for (const char* fmt : {"3d", ".llx", "-G", "1$#X"}) {
SCOPED_TRACE(fmt);
EXPECT_TRUE(Run(fmt));
- EXPECT_FALSE(o.flags.basic);
+ EXPECT_NE(o.flags, Flags::kBasic);
}
}
diff --git a/absl/synchronization/blocking_counter.cc b/absl/synchronization/blocking_counter.cc
index 3cea7ae..d2f82da 100644
--- a/absl/synchronization/blocking_counter.cc
+++ b/absl/synchronization/blocking_counter.cc
@@ -14,41 +14,51 @@
#include "absl/synchronization/blocking_counter.h"
+#include <atomic>
+
#include "absl/base/internal/raw_logging.h"
namespace absl {
ABSL_NAMESPACE_BEGIN
-// Return whether int *arg is zero.
-static bool IsZero(void *arg) {
- return 0 == *reinterpret_cast<int *>(arg);
+namespace {
+
+// Return whether int *arg is true.
+bool IsDone(void *arg) { return *reinterpret_cast<bool *>(arg); }
+
+} // namespace
+
+BlockingCounter::BlockingCounter(int initial_count)
+ : count_(initial_count),
+ num_waiting_(0),
+ done_{initial_count == 0 ? true : false} {
+ ABSL_RAW_CHECK(initial_count >= 0, "BlockingCounter initial_count negative");
}
bool BlockingCounter::DecrementCount() {
- MutexLock l(&lock_);
- count_--;
- if (count_ < 0) {
- ABSL_RAW_LOG(
- FATAL,
- "BlockingCounter::DecrementCount() called too many times. count=%d",
- count_);
+ int count = count_.fetch_sub(1, std::memory_order_acq_rel) - 1;
+ ABSL_RAW_CHECK(count >= 0,
+ "BlockingCounter::DecrementCount() called too many times");
+ if (count == 0) {
+ MutexLock l(&lock_);
+ done_ = true;
+ return true;
}
- return count_ == 0;
+ return false;
}
void BlockingCounter::Wait() {
MutexLock l(&this->lock_);
- ABSL_RAW_CHECK(count_ >= 0, "BlockingCounter underflow");
// only one thread may call Wait(). To support more than one thread,
// implement a counter num_to_exit, like in the Barrier class.
ABSL_RAW_CHECK(num_waiting_ == 0, "multiple threads called Wait()");
num_waiting_++;
- this->lock_.Await(Condition(IsZero, &this->count_));
+ this->lock_.Await(Condition(IsDone, &this->done_));
- // At this point, We know that all threads executing DecrementCount have
- // released the lock, and so will not touch this object again.
+ // At this point, we know that all threads executing DecrementCount
+ // will not touch this object again.
// Therefore, the thread calling this method is free to delete the object
// after we return from this method.
}
diff --git a/absl/synchronization/blocking_counter.h b/absl/synchronization/blocking_counter.h
index 1f53f9f..1908fdb 100644
--- a/absl/synchronization/blocking_counter.h
+++ b/absl/synchronization/blocking_counter.h
@@ -20,6 +20,8 @@
#ifndef ABSL_SYNCHRONIZATION_BLOCKING_COUNTER_H_
#define ABSL_SYNCHRONIZATION_BLOCKING_COUNTER_H_
+#include <atomic>
+
#include "absl/base/thread_annotations.h"
#include "absl/synchronization/mutex.h"
@@ -60,8 +62,7 @@
//
class BlockingCounter {
public:
- explicit BlockingCounter(int initial_count)
- : count_(initial_count), num_waiting_(0) {}
+ explicit BlockingCounter(int initial_count);
BlockingCounter(const BlockingCounter&) = delete;
BlockingCounter& operator=(const BlockingCounter&) = delete;
@@ -89,8 +90,9 @@
private:
Mutex lock_;
- int count_ ABSL_GUARDED_BY(lock_);
+ std::atomic<int> count_;
int num_waiting_ ABSL_GUARDED_BY(lock_);
+ bool done_ ABSL_GUARDED_BY(lock_);
};
ABSL_NAMESPACE_END
diff --git a/absl/synchronization/blocking_counter_test.cc b/absl/synchronization/blocking_counter_test.cc
index 2926224..06885f5 100644
--- a/absl/synchronization/blocking_counter_test.cc
+++ b/absl/synchronization/blocking_counter_test.cc
@@ -63,6 +63,18 @@
}
}
+TEST(BlockingCounterTest, WaitZeroInitialCount) {
+ BlockingCounter counter(0);
+ counter.Wait();
+}
+
+#if GTEST_HAS_DEATH_TEST
+TEST(BlockingCounterTest, WaitNegativeInitialCount) {
+ EXPECT_DEATH(BlockingCounter counter(-1),
+ "BlockingCounter initial_count negative");
+}
+#endif
+
} // namespace
ABSL_NAMESPACE_END
} // namespace absl
