Revert "[channel_args] Optimize UnionWith (#33154)"
This reverts commit cc3d0349487ae35126a4ba61aa23437b16e25262.
diff --git a/bazel/grpc_deps.bzl b/bazel/grpc_deps.bzl
index 869f743..dcabaf9 100644
--- a/bazel/grpc_deps.bzl
+++ b/bazel/grpc_deps.bzl
@@ -278,15 +278,13 @@
)
if "com_google_fuzztest" not in native.existing_rules():
- # when updating this remember to run:
- # bazel run @com_google_fuzztest//bazel:setup_configs > tools/fuzztest.bazelrc
http_archive(
name = "com_google_fuzztest",
- sha256 = "cdf8d8cd3cdc77280a7c59b310edf234e489a96b6e727cb271e7dfbeb9bcca8d",
- strip_prefix = "fuzztest-4ecaeb5084a061a862af8f86789ee184cd3d3f18",
+ sha256 = "f7bb5b3bd162576f3fbbe9bb768b57931fdd98581c1818789aceee5be4eeee64",
+ strip_prefix = "fuzztest-62cf00c7341eb05d128d0a3cbce79ac31dbda032",
urls = [
- # 2023-05-16
- "https://github.com/google/fuzztest/archive/4ecaeb5084a061a862af8f86789ee184cd3d3f18.tar.gz",
+ # 2023-03-03
+ "https://github.com/google/fuzztest/archive/62cf00c7341eb05d128d0a3cbce79ac31dbda032.tar.gz",
],
)
diff --git a/fuzztest/core/channel/BUILD b/fuzztest/core/channel/BUILD
deleted file mode 100644
index d6192ad..0000000
--- a/fuzztest/core/channel/BUILD
+++ /dev/null
@@ -1,27 +0,0 @@
-# Copyright 2023 gRPC 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
-#
-# http://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.
-
-load("//fuzztest:grpc_fuzz_test.bzl", "grpc_fuzz_test")
-
-grpc_fuzz_test(
- name = "union_with_test",
- srcs = ["union_with_test.cc"],
- external_deps = [
- "fuzztest",
- "fuzztest_main",
- ],
- deps = [
- "//src/core:channel_args",
- ],
-)
diff --git a/fuzztest/core/channel/union_with_test.cc b/fuzztest/core/channel/union_with_test.cc
deleted file mode 100644
index 6e3d0a0..0000000
--- a/fuzztest/core/channel/union_with_test.cc
+++ /dev/null
@@ -1,47 +0,0 @@
-// Copyright 2023 gRPC 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
-//
-// http://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.
-
-// Test to verify Fuzztest integration
-
-#include "fuzztest/fuzztest.h"
-
-#include "gtest/gtest.h"
-
-#include "src/core/lib/channel/channel_args.h"
-
-namespace grpc_core {
-
-using IntOrString = absl::variant<int, std::string>;
-using VectorOfArgs = std::vector<std::pair<std::string, IntOrString>>;
-
-ChannelArgs ChannelArgsFromVector(VectorOfArgs va) {
- ChannelArgs result;
- for (auto& [key, value] : va) {
- if (absl::holds_alternative<int>(value)) {
- result = result.Set(key, absl::get<int>(value));
- } else {
- result = result.Set(key, absl::get<std::string>(value));
- }
- }
- return result;
-}
-
-void UnionWithIsCorrect(VectorOfArgs va, VectorOfArgs vb) {
- auto a = ChannelArgsFromVector(std::move(va));
- auto b = ChannelArgsFromVector(std::move(vb));
- EXPECT_EQ(a.UnionWith(b), a.FuzzingReferenceUnionWith(b));
-}
-FUZZ_TEST(MyTestSuite, UnionWithIsCorrect);
-
-}
diff --git a/src/core/lib/avl/avl.h b/src/core/lib/avl/avl.h
index 9ce5296..e44fb5d 100644
--- a/src/core/lib/avl/avl.h
+++ b/src/core/lib/avl/avl.h
@@ -87,11 +87,6 @@
return QsortCompare(*this, other) < 0;
}
- size_t Height() const {
- if (root_ == nullptr) return 0;
- return root_->height;
- }
-
private:
struct Node;
diff --git a/src/core/lib/channel/channel_args.cc b/src/core/lib/channel/channel_args.cc
index 9fced0d..7f54a32 100644
--- a/src/core/lib/channel/channel_args.cc
+++ b/src/core/lib/channel/channel_args.cc
@@ -39,7 +39,6 @@
#include <grpc/support/string_util.h>
#include "src/core/lib/gpr/useful.h"
-#include "src/core/lib/gprpp/crash.h"
#include "src/core/lib/gprpp/match.h"
namespace grpc_core {
@@ -127,55 +126,21 @@
return result;
}
-grpc_arg ChannelArgs::Value::MakeCArg(const char* name) const {
- char* c_name = const_cast<char*>(name);
- return Match(
- rep_,
- [c_name](int i) { return grpc_channel_arg_integer_create(c_name, i); },
- [c_name](const std::shared_ptr<const std::string>& s) {
- return grpc_channel_arg_string_create(c_name,
- const_cast<char*>(s->c_str()));
- },
- [c_name](const Pointer& p) {
- return grpc_channel_arg_pointer_create(c_name, p.c_pointer(),
- p.c_vtable());
- });
-}
-
-bool ChannelArgs::Value::operator<(const Value& rhs) const {
- if (rhs.rep_.index() != rep_.index()) return rep_.index() < rhs.rep_.index();
- switch (rep_.index()) {
- case 0:
- return absl::get<int>(rep_) < absl::get<int>(rhs.rep_);
- case 1:
- return *absl::get<std::shared_ptr<const std::string>>(rep_) <
- *absl::get<std::shared_ptr<const std::string>>(rhs.rep_);
- case 2:
- return absl::get<Pointer>(rep_) < absl::get<Pointer>(rhs.rep_);
- default:
- Crash("unreachable");
- }
-}
-
-bool ChannelArgs::Value::operator==(const Value& rhs) const {
- if (rhs.rep_.index() != rep_.index()) return false;
- switch (rep_.index()) {
- case 0:
- return absl::get<int>(rep_) == absl::get<int>(rhs.rep_);
- case 1:
- return *absl::get<std::shared_ptr<const std::string>>(rep_) ==
- *absl::get<std::shared_ptr<const std::string>>(rhs.rep_);
- case 2:
- return absl::get<Pointer>(rep_) == absl::get<Pointer>(rhs.rep_);
- default:
- Crash("unreachable");
- }
-}
-
ChannelArgs::CPtr ChannelArgs::ToC() const {
std::vector<grpc_arg> c_args;
args_.ForEach([&c_args](const std::string& key, const Value& value) {
- c_args.push_back(value.MakeCArg(key.c_str()));
+ char* name = const_cast<char*>(key.c_str());
+ c_args.push_back(Match(
+ value,
+ [name](int i) { return grpc_channel_arg_integer_create(name, i); },
+ [name](const std::string& s) {
+ return grpc_channel_arg_string_create(name,
+ const_cast<char*>(s.c_str()));
+ },
+ [name](const Pointer& p) {
+ return grpc_channel_arg_pointer_create(name, p.c_pointer(),
+ p.c_vtable());
+ }));
});
return CPtr(static_cast<const grpc_channel_args*>(
grpc_channel_args_copy_and_add(nullptr, c_args.data(), c_args.size())));
@@ -213,9 +178,8 @@
absl::optional<int> ChannelArgs::GetInt(absl::string_view name) const {
auto* v = Get(name);
if (v == nullptr) return absl::nullopt;
- const auto* i = v->GetIfInt();
- if (i == nullptr) return absl::nullopt;
- return *i;
+ if (!absl::holds_alternative<int>(*v)) return absl::nullopt;
+ return absl::get<int>(*v);
}
absl::optional<Duration> ChannelArgs::GetDurationFromIntMillis(
@@ -231,9 +195,8 @@
absl::string_view name) const {
auto* v = Get(name);
if (v == nullptr) return absl::nullopt;
- const auto* s = v->GetIfString();
- if (s == nullptr) return absl::nullopt;
- return *s;
+ if (!absl::holds_alternative<std::string>(*v)) return absl::nullopt;
+ return absl::get<std::string>(*v);
}
absl::optional<std::string> ChannelArgs::GetOwnedString(
@@ -246,15 +209,14 @@
void* ChannelArgs::GetVoidPointer(absl::string_view name) const {
auto* v = Get(name);
if (v == nullptr) return nullptr;
- const auto* pp = v->GetIfPointer();
- if (pp == nullptr) return nullptr;
- return pp->c_pointer();
+ if (!absl::holds_alternative<Pointer>(*v)) return nullptr;
+ return absl::get<Pointer>(*v).c_pointer();
}
absl::optional<bool> ChannelArgs::GetBool(absl::string_view name) const {
auto* v = Get(name);
if (v == nullptr) return absl::nullopt;
- auto* i = v->GetIfInt();
+ auto* i = absl::get_if<int>(v);
if (i == nullptr) {
gpr_log(GPR_ERROR, "%s ignored: it must be an integer",
std::string(name).c_str());
@@ -276,11 +238,11 @@
std::vector<std::string> arg_strings;
args_.ForEach([&arg_strings](const std::string& key, const Value& value) {
std::string value_str;
- if (auto* i = value.GetIfInt()) {
+ if (auto* i = absl::get_if<int>(&value)) {
value_str = std::to_string(*i);
- } else if (auto* s = value.GetIfString()) {
+ } else if (auto* s = absl::get_if<std::string>(&value)) {
value_str = *s;
- } else if (auto* p = value.GetIfPointer()) {
+ } else if (auto* p = absl::get_if<Pointer>(&value)) {
value_str = absl::StrFormat("%p", p->c_pointer());
}
arg_strings.push_back(absl::StrCat(key, "=", value_str));
@@ -289,26 +251,6 @@
}
ChannelArgs ChannelArgs::UnionWith(ChannelArgs other) const {
- if (args_.Empty()) return other;
- if (other.args_.Empty()) return *this;
- if (args_.Height() <= other.args_.Height()) {
- args_.ForEach([&other](const std::string& key, const Value& value) {
- other.args_ = other.args_.Add(key, value);
- });
- return other;
- } else {
- auto result = *this;
- other.args_.ForEach([&result](const std::string& key, const Value& value) {
- if (result.args_.Lookup(key) == nullptr) {
- result.args_ = result.args_.Add(key, value);
- }
- });
- return result;
- }
-}
-
-ChannelArgs ChannelArgs::FuzzingReferenceUnionWith(ChannelArgs other) const {
- // DO NOT OPTIMIZE THIS!!
args_.ForEach([&other](const std::string& key, const Value& value) {
other.args_ = other.args_.Add(key, value);
});
diff --git a/src/core/lib/channel/channel_args.h b/src/core/lib/channel/channel_args.h
index 8bf1a9c..2ff1814 100644
--- a/src/core/lib/channel/channel_args.h
+++ b/src/core/lib/channel/channel_args.h
@@ -281,35 +281,7 @@
const grpc_arg_pointer_vtable* vtable_;
};
- class Value {
- public:
- explicit Value(int n) : rep_(n) {}
- explicit Value(std::string s)
- : rep_(std::make_shared<const std::string>(std::move(s))) {}
- explicit Value(Pointer p) : rep_(std::move(p)) {}
-
- const int* GetIfInt() const { return absl::get_if<int>(&rep_); }
- const std::string* GetIfString() const {
- auto* p = absl::get_if<std::shared_ptr<const std::string>>(&rep_);
- if (p == nullptr) return nullptr;
- return p->get();
- }
- const Pointer* GetIfPointer() const { return absl::get_if<Pointer>(&rep_); }
-
- grpc_arg MakeCArg(const char* name) const;
-
- bool operator<(const Value& rhs) const;
- bool operator==(const Value& rhs) const;
- bool operator!=(const Value& rhs) const { return !this->operator==(rhs); }
- bool operator==(absl::string_view rhs) const {
- auto* p = absl::get_if<std::shared_ptr<const std::string>>(&rep_);
- if (p == nullptr) return false;
- return **p == rhs;
- }
-
- private:
- absl::variant<int, std::shared_ptr<const std::string>, Pointer> rep_;
- };
+ using Value = absl::variant<int, std::string, Pointer>;
struct ChannelArgsDeleter {
void operator()(const grpc_channel_args* p) const;
@@ -335,11 +307,6 @@
// If a key is present in both, the value from this is used.
GRPC_MUST_USE_RESULT ChannelArgs UnionWith(ChannelArgs other) const;
- // Only used in union_with_test.cc, reference version of UnionWith for
- // differential fuzzing.
- GRPC_MUST_USE_RESULT ChannelArgs
- FuzzingReferenceUnionWith(ChannelArgs other) const;
-
const Value* Get(absl::string_view name) const;
GRPC_MUST_USE_RESULT ChannelArgs Set(absl::string_view name,
Pointer value) const;
diff --git a/src/core/lib/compression/compression_internal.cc b/src/core/lib/compression/compression_internal.cc
index 4871e46..36044e0 100644
--- a/src/core/lib/compression/compression_internal.cc
+++ b/src/core/lib/compression/compression_internal.cc
@@ -22,10 +22,13 @@
#include <stdlib.h>
+#include <string>
+
#include "absl/container/inlined_vector.h"
#include "absl/strings/ascii.h"
#include "absl/strings/str_format.h"
#include "absl/strings/str_split.h"
+#include "absl/types/variant.h"
#include <grpc/support/log.h>
@@ -227,10 +230,10 @@
DefaultCompressionAlgorithmFromChannelArgs(const ChannelArgs& args) {
auto* value = args.Get(GRPC_COMPRESSION_CHANNEL_DEFAULT_ALGORITHM);
if (value == nullptr) return absl::nullopt;
- if (auto* p = value->GetIfInt()) {
+ if (auto* p = absl::get_if<int>(value)) {
return static_cast<grpc_compression_algorithm>(*p);
}
- if (auto* p = value->GetIfString()) {
+ if (auto* p = absl::get_if<std::string>(value)) {
return ParseCompressionAlgorithm(*p);
}
return absl::nullopt;
diff --git a/tools/bazel.rc b/tools/bazel.rc
index 976ac74..da0d589 100644
--- a/tools/bazel.rc
+++ b/tools/bazel.rc
@@ -145,5 +145,3 @@
try-import %workspace%/tools/fuzztest.bazelrc
build:fuzztest --cxxopt=-std=c++17
-
-build:fuzztest_test --cxxopt=-std=c++17
diff --git a/tools/fuzztest.bazelrc b/tools/fuzztest.bazelrc
index 1721cf5..b2616ef 100644
--- a/tools/fuzztest.bazelrc
+++ b/tools/fuzztest.bazelrc
@@ -13,9 +13,8 @@
#
# Do not use directly.
-# Compile and link with Address Sanitizer (ASAN).
+# Link with Address Sanitizer (ASAN).
build:fuzztest-common --linkopt=-fsanitize=address
-build:fuzztest-common --copt=-fsanitize=address
# Standard define for "ifdef-ing" any fuzz test specific code.
build:fuzztest-common --copt=-DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
@@ -23,10 +22,6 @@
# In fuzz tests, we want to catch assertion violations even in optimized builds.
build:fuzztest-common --copt=-UNDEBUG
-# Enable libc++ assertions.
-# See https://libcxx.llvm.org/UsingLibcxx.html#enabling-the-safe-libc-mode
-build:fuzztest-common --copt=-D_LIBCPP_ENABLE_ASSERTIONS=1
-
### FuzzTest build configuration.
#
@@ -42,7 +37,7 @@
# the uninstrumented runtime.
build:fuzztest --copt=-DADDRESS_SANITIZER
-# We apply coverage tracking instrumentation to everything but the
+# We apply coverage tracking and ASAN instrumentation to everything but the
# FuzzTest framework itself (including GoogleTest and GoogleMock).
-build:fuzztest --per_file_copt=+//,-fuzztest/.*,-googletest/.*,-googlemock/.*@-fsanitize-coverage=inline-8bit-counters,-fsanitize-coverage=trace-cmp
+build:fuzztest --per_file_copt=+//,-fuzztest/.*,-googletest/.*,-googlemock/.*@-fsanitize=address,-fsanitize-coverage=inline-8bit-counters,-fsanitize-coverage=trace-cmp
