Revert "[channel_args] Optimize UnionWith" (#33159)

Reverts grpc/grpc#33154

breaks import

bazel/grpc_deps.bzl

@@ -278,15 +278,13 @@ def grpc_deps():
         )
 
     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",
+            sha256 = "f7bb5b3bd162576f3fbbe9bb768b57931fdd98581c1818789aceee5be4eeee64",
-            strip_prefix = "fuzztest-4ecaeb5084a061a862af8f86789ee184cd3d3f18",
+            strip_prefix = "fuzztest-62cf00c7341eb05d128d0a3cbce79ac31dbda032",
             urls = [
-                # 2023-05-16
+                # 2023-03-03
-                "https://github.com/google/fuzztest/archive/4ecaeb5084a061a862af8f86789ee184cd3d3f18.tar.gz",
+                "https://github.com/google/fuzztest/archive/62cf00c7341eb05d128d0a3cbce79ac31dbda032.tar.gz",
             ],
         )
 

fuzztest/core/channel/BUILD

@@ -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",
-    ],
-)

fuzztest/core/channel/union_with_test.cc

@@ -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);
-
-}

src/core/lib/avl/avl.h

@@ -87,11 +87,6 @@ class AVL {
     return QsortCompare(*this, other) < 0;
   }
 
-  size_t Height() const {
-    if (root_ == nullptr) return 0;
-    return root_->height;
-  }
-
  private:
   struct Node;
 

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 @@ ChannelArgs ChannelArgs::FromC(const grpc_channel_args* args) {
   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 @@ ChannelArgs ChannelArgs::Remove(absl::string_view key) const {
 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 (!absl::holds_alternative<int>(*v)) return absl::nullopt;
-  if (i == nullptr) return absl::nullopt;
+  return absl::get<int>(*v);
-  return *i;
 }
 
 absl::optional<Duration> ChannelArgs::GetDurationFromIntMillis(
@@ -231,9 +195,8 @@ absl::optional<absl::string_view> ChannelArgs::GetString(
     absl::string_view name) const {
   auto* v = Get(name);
   if (v == nullptr) return absl::nullopt;
-  const auto* s = v->GetIfString();
+  if (!absl::holds_alternative<std::string>(*v)) return absl::nullopt;
-  if (s == nullptr) return absl::nullopt;
+  return absl::get<std::string>(*v);
-  return *s;
 }
 
 absl::optional<std::string> ChannelArgs::GetOwnedString(
@@ -246,15 +209,14 @@ absl::optional<std::string> ChannelArgs::GetOwnedString(
 void* ChannelArgs::GetVoidPointer(absl::string_view name) const {
   auto* v = Get(name);
   if (v == nullptr) return nullptr;
-  const auto* pp = v->GetIfPointer();
+  if (!absl::holds_alternative<Pointer>(*v)) return nullptr;
-  if (pp == nullptr) return nullptr;
+  return absl::get<Pointer>(*v).c_pointer();
-  return pp->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::string ChannelArgs::ToString() const {
   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 @@ std::string ChannelArgs::ToString() const {
 }
 
 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);
   });

src/core/lib/channel/channel_args.h

@@ -281,35 +281,7 @@ class ChannelArgs {
     const grpc_arg_pointer_vtable* vtable_;
   };
 
-  class Value {
+  using Value = absl::variant<int, std::string, Pointer>;
-   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_;
-  };
 
   struct ChannelArgsDeleter {
     void operator()(const grpc_channel_args* p) const;
@@ -335,11 +307,6 @@ class ChannelArgs {
   // 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;

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 @@ absl::optional<grpc_compression_algorithm>
 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;

tools/bazel.rc

@@ -145,5 +145,3 @@ build:compdb --build_tag_filters=-nocompdb --features=-layering_check
 
 try-import %workspace%/tools/fuzztest.bazelrc
 build:fuzztest --cxxopt=-std=c++17
-
-build:fuzztest_test --cxxopt=-std=c++17

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 @@ build:fuzztest-common --copt=-DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
 # 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 @@ build:fuzztest --dynamic_mode=off
 # 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