Merge pull request #19987 from arjunroy/server_slice_eq

Strongly typed slices for server method/host.

src/core/lib/slice/slice_intern.cc

@@ -50,10 +50,6 @@ typedef struct slice_shard {
   size_t capacity;
 } slice_shard;
 
-/* hash seed: decided at initialization time */
-uint32_t g_hash_seed;
-static int g_forced_hash_seed = 0;
-
 static slice_shard g_shards[SHARD_COUNT];
 
 typedef struct {
@@ -68,6 +64,10 @@ uint32_t grpc_static_metadata_hash_values[GRPC_STATIC_MDSTR_COUNT];
 
 namespace grpc_core {
 
+/* hash seed: decided at initialization time */
+uint32_t g_hash_seed;
+static bool g_forced_hash_seed = false;
+
 InternedSliceRefcount::~InternedSliceRefcount() {
   slice_shard* shard = &g_shards[SHARD_IDX(this->hash)];
   MutexLock lock(&shard->mu);
@@ -115,7 +115,7 @@ grpc_core::InternedSlice::InternedSlice(InternedSliceRefcount* s) {
 
 uint32_t grpc_slice_default_hash_impl(grpc_slice s) {
   return gpr_murmur_hash3(GRPC_SLICE_START_PTR(s), GRPC_SLICE_LENGTH(s),
-                          g_hash_seed);
+                          grpc_core::g_hash_seed);
 }
 
 uint32_t grpc_static_slice_hash(grpc_slice s) {
@@ -159,20 +159,20 @@ grpc_slice grpc_slice_intern(grpc_slice slice) {
 }
 
 // Attempt to see if the provided slice or string matches a static slice.
-// SliceArgs... is either a const grpc_slice& or a string and length. In either
-// case, hash is the pre-computed hash value.
+// SliceArgs is either a const grpc_slice& or const pair<const char*, size_t>&.
+// In either case, hash is the pre-computed hash value.
 //
 // Returns: a matching static slice, or null.
-template <class... SliceArgs>
+template <typename SliceArgs>
 static const grpc_core::StaticMetadataSlice* MatchStaticSlice(
-    uint32_t hash, SliceArgs&&... args) {
+    uint32_t hash, const SliceArgs& args) {
   for (uint32_t i = 0; i <= max_static_metadata_hash_probe; i++) {
     static_metadata_hash_ent ent =
         static_metadata_hash[(hash + i) % GPR_ARRAY_SIZE(static_metadata_hash)];
     const grpc_core::StaticMetadataSlice* static_slice_table =
         grpc_static_slice_table();
     if (ent.hash == hash && ent.idx < GRPC_STATIC_MDSTR_COUNT &&
-        static_slice_table[ent.idx].Equals(std::forward<SliceArgs>(args)...)) {
+        static_slice_table[ent.idx] == args) {
       return &static_slice_table[ent.idx];
     }
   }
@@ -182,8 +182,12 @@ static const grpc_core::StaticMetadataSlice* MatchStaticSlice(
 // Helper methods to enable us to select appropriately overloaded slice methods
 // whether we're dealing with a slice, or a buffer with length, when interning
 // strings. Helpers for FindOrCreateInternedSlice().
-static const void* GetBuffer(const void* buf, size_t len) { return buf; }
-static size_t GetLength(const void* buf, size_t len) { return len; }
+static const char* GetBuffer(const std::pair<const char*, size_t>& buflen) {
+  return buflen.first;
+}
+static size_t GetLength(const std::pair<const char*, size_t>& buflen) {
+  return buflen.second;
+}
 static const void* GetBuffer(const grpc_slice& slice) {
   return GRPC_SLICE_START_PTR(slice);
 }
@@ -192,19 +196,19 @@ static size_t GetLength(const grpc_slice& slice) {
 }
 
 // Creates an interned slice for a string that does not currently exist in the
-// intern table. SliceArgs... is either a const grpc_slice& or a string and
-// length. In either case, hash is the pre-computed hash value. We must already
-// hold the shard lock. Helper for FindOrCreateInternedSlice().
+// intern table. SliceArgs is either a const grpc_slice& or a const
+// pair<const char*, size_t>&. Hash is the pre-computed hash value. We must
+// already hold the shard lock. Helper for FindOrCreateInternedSlice().
 //
 // Returns: a newly interned slice.
-template <class... SliceArgs>
+template <typename SliceArgs>
 static InternedSliceRefcount* InternNewStringLocked(slice_shard* shard,
                                                     size_t shard_idx,
                                                     uint32_t hash,
-                                                    SliceArgs&&... args) {
+                                                    const SliceArgs& args) {
   /* string data goes after the internal_string header */
-  size_t len = GetLength(std::forward<SliceArgs>(args)...);
-  const void* buffer = GetBuffer(std::forward<SliceArgs>(args)...);
+  size_t len = GetLength(args);
+  const void* buffer = GetBuffer(args);
   InternedSliceRefcount* s =
       static_cast<InternedSliceRefcount*>(gpr_malloc(sizeof(*s) + len));
   new (s) grpc_core::InternedSliceRefcount(len, hash, shard->strs[shard_idx]);
@@ -227,16 +231,15 @@ static InternedSliceRefcount* InternNewStringLocked(slice_shard* shard,
 // shard lock. Helper for FindOrCreateInternedSlice().
 //
 // Returns: a pre-existing matching static slice, or null.
-template <class... SliceArgs>
+template <typename SliceArgs>
 static InternedSliceRefcount* MatchInternedSliceLocked(uint32_t hash,
                                                        size_t idx,
-                                                       SliceArgs&&... args) {
+                                                       const SliceArgs& args) {
   InternedSliceRefcount* s;
   slice_shard* shard = &g_shards[SHARD_IDX(hash)];
   /* search for an existing string */
   for (s = shard->strs[idx]; s; s = s->bucket_next) {
-    if (s->hash == hash &&
-        grpc_core::InternedSlice(s).Equals(std::forward<SliceArgs>(args)...)) {
+    if (s->hash == hash && grpc_core::InternedSlice(s) == args) {
       if (s->refcnt.RefIfNonZero()) {
         return s;
       }
@@ -248,22 +251,20 @@ static InternedSliceRefcount* MatchInternedSliceLocked(uint32_t hash,
 // Attempt to see if the provided slice or string matches an existing interned
 // slice, and failing that, create an interned slice with its contents. Returns
 // either the existing matching interned slice or the newly created one.
-// SliceArgs... is either a const grpc_slice& or a string and length. In either
-// case, hash is the pre-computed hash value. We do not hold the shard lock
-// here, but do take it.
+// SliceArgs is either a const grpc_slice& or const pair<const char*, size_t>&.
+// In either case, hash is the pre-computed hash value. We do not hold the
+// shard lock here, but do take it.
 //
 // Returns: an interned slice, either pre-existing/matched or newly created.
-template <class... SliceArgs>
+template <typename SliceArgs>
 static InternedSliceRefcount* FindOrCreateInternedSlice(uint32_t hash,
-                                                        SliceArgs&&... args) {
+                                                        const SliceArgs& args) {
   slice_shard* shard = &g_shards[SHARD_IDX(hash)];
   gpr_mu_lock(&shard->mu);
   const size_t idx = TABLE_IDX(hash, shard->capacity);
-  InternedSliceRefcount* s =
-      MatchInternedSliceLocked(hash, idx, std::forward<SliceArgs>(args)...);
+  InternedSliceRefcount* s = MatchInternedSliceLocked(hash, idx, args);
   if (s == nullptr) {
-    s = InternNewStringLocked(shard, idx, hash,
-                              std::forward<SliceArgs>(args)...);
+    s = InternNewStringLocked(shard, idx, hash, args);
   }
   gpr_mu_unlock(&shard->mu);
   return s;
@@ -277,12 +278,13 @@ grpc_core::ManagedMemorySlice::ManagedMemorySlice(const char* string,
                                                   size_t len) {
   GPR_TIMER_SCOPE("grpc_slice_intern", 0);
   const uint32_t hash = gpr_murmur_hash3(string, len, g_hash_seed);
-  const StaticMetadataSlice* static_slice = MatchStaticSlice(hash, string, len);
+  const StaticMetadataSlice* static_slice =
+      MatchStaticSlice(hash, std::pair<const char*, size_t>(string, len));
   if (static_slice) {
     *this = *static_slice;
   } else {
-    *this =
-        grpc_core::InternedSlice(FindOrCreateInternedSlice(hash, string, len));
+    *this = grpc_core::InternedSlice(FindOrCreateInternedSlice(
+        hash, std::pair<const char*, size_t>(string, len)));
   }
 }
 
@@ -303,13 +305,14 @@ grpc_core::ManagedMemorySlice::ManagedMemorySlice(const grpc_slice* slice_ptr) {
 }
 
 void grpc_test_only_set_slice_hash_seed(uint32_t seed) {
-  g_hash_seed = seed;
-  g_forced_hash_seed = 1;
+  grpc_core::g_hash_seed = seed;
+  grpc_core::g_forced_hash_seed = true;
 }
 
 void grpc_slice_intern_init(void) {
-  if (!g_forced_hash_seed) {
-    g_hash_seed = static_cast<uint32_t>(gpr_now(GPR_CLOCK_REALTIME).tv_nsec);
+  if (!grpc_core::g_forced_hash_seed) {
+    grpc_core::g_hash_seed =
+        static_cast<uint32_t>(gpr_now(GPR_CLOCK_REALTIME).tv_nsec);
   }
   for (size_t i = 0; i < SHARD_COUNT; i++) {
     slice_shard* shard = &g_shards[i];

src/core/lib/slice/slice_internal.h

@@ -36,7 +36,6 @@
 // Interned slices have specific fast-path operations for hashing. To inline
 // these operations, we need to forward declare them here.
 extern uint32_t grpc_static_metadata_hash_values[GRPC_STATIC_MDSTR_COUNT];
-extern uint32_t g_hash_seed;
 
 // grpc_slice_refcount : A reference count for grpc_slice.
 //
@@ -259,7 +258,8 @@ inline uint32_t grpc_slice_refcount::Hash(const grpc_slice& slice) {
       break;
   }
   return gpr_murmur_hash3(grpc_refcounted_slice_data(slice),
-                          grpc_refcounted_slice_length(slice), g_hash_seed);
+                          grpc_refcounted_slice_length(slice),
+                          grpc_core::g_hash_seed);
 }
 
 inline const grpc_slice& grpc_slice_ref_internal(const grpc_slice& slice) {
@@ -327,7 +327,7 @@ inline uint32_t grpc_slice_hash_refcounted(const grpc_slice& s) {
 
 inline uint32_t grpc_slice_default_hash_internal(const grpc_slice& s) {
   return gpr_murmur_hash3(GRPC_SLICE_START_PTR(s), GRPC_SLICE_LENGTH(s),
-                          g_hash_seed);
+                          grpc_core::g_hash_seed);
 }
 
 inline uint32_t grpc_slice_hash_internal(const grpc_slice& s) {

src/core/lib/slice/slice_utils.h

@@ -25,6 +25,12 @@
 
 #include <grpc/slice.h>
 
+#include "src/core/lib/gpr/murmur_hash.h"
+
+namespace grpc_core {
+extern uint32_t g_hash_seed;
+}  // namespace grpc_core
+
 // When we compare two slices, and we know the latter is not inlined, we can
 // short circuit our comparison operator. We specifically use differs()
 // semantics instead of equals() semantics due to more favourable code
@@ -101,15 +107,16 @@ struct ManagedMemorySlice : public grpc_slice {
   explicit ManagedMemorySlice(const char* string);
   ManagedMemorySlice(const char* buf, size_t len);
   explicit ManagedMemorySlice(const grpc_slice* slice);
-  bool Equals(const grpc_slice& other) const {
+  bool operator==(const grpc_slice& other) const {
     if (refcount == other.refcount) {
       return true;
     }
     return !grpc_slice_differs_refcounted(other, *this);
   }
-  bool Equals(const char* buf, const size_t len) const {
-    return data.refcounted.length == len && buf != nullptr &&
-           memcmp(buf, data.refcounted.bytes, len) == 0;
+  bool operator!=(const grpc_slice& other) const { return !(*this == other); }
+  bool operator==(std::pair<const char*, size_t> buflen) const {
+    return data.refcounted.length == buflen.second && buflen.first != nullptr &&
+           memcmp(buflen.first, data.refcounted.bytes, buflen.second) == 0;
   }
 };
 struct UnmanagedMemorySlice : public grpc_slice {
@@ -150,6 +157,16 @@ struct ExternallyManagedSlice : public UnmanagedMemorySlice {
     data.refcounted.length = length;
     data.refcounted.bytes = bytes;
   }
+  bool operator==(const grpc_slice& other) const {
+    return data.refcounted.length == GRPC_SLICE_LENGTH(other) &&
+           memcmp(data.refcounted.bytes, GRPC_SLICE_START_PTR(other),
+                  data.refcounted.length) == 0;
+  }
+  bool operator!=(const grpc_slice& other) const { return !(*this == other); }
+  uint32_t Hash() {
+    return gpr_murmur_hash3(data.refcounted.bytes, data.refcounted.length,
+                            g_hash_seed);
+  }
 };
 
 struct StaticMetadataSlice : public ManagedMemorySlice {

src/core/lib/surface/server.cc

@@ -99,8 +99,8 @@ struct channel_registered_method {
   registered_method* server_registered_method;
   uint32_t flags;
   bool has_host;
-  grpc_slice method;
-  grpc_slice host;
+  grpc_core::ExternallyManagedSlice method;
+  grpc_core::ExternallyManagedSlice host;
 };
 
 struct channel_data {
@@ -630,8 +630,8 @@ static void start_new_rpc(grpc_call_element* elem) {
                                       chand->registered_method_slots];
       if (rm->server_registered_method == nullptr) break;
       if (!rm->has_host) continue;
-      if (!grpc_slice_eq(rm->host, calld->host)) continue;
-      if (!grpc_slice_eq(rm->method, calld->path)) continue;
+      if (rm->host != calld->host) continue;
+      if (rm->method != calld->path) continue;
       if ((rm->flags & GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST) &&
           0 == (calld->recv_initial_metadata_flags &
                 GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST)) {
@@ -648,7 +648,7 @@ static void start_new_rpc(grpc_call_element* elem) {
                                       chand->registered_method_slots];
       if (rm->server_registered_method == nullptr) break;
       if (rm->has_host) continue;
-      if (!grpc_slice_eq(rm->method, calld->path)) continue;
+      if (rm->method != calld->path) continue;
       if ((rm->flags & GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST) &&
           0 == (calld->recv_initial_metadata_flags &
                 GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST)) {
@@ -921,8 +921,14 @@ static void server_destroy_channel_elem(grpc_channel_element* elem) {
   if (chand->registered_methods) {
     for (i = 0; i < chand->registered_method_slots; i++) {
       grpc_slice_unref_internal(chand->registered_methods[i].method);
+      GPR_DEBUG_ASSERT(chand->registered_methods[i].method.refcount ==
+                           &grpc_core::kNoopRefcount ||
+                       chand->registered_methods[i].method.refcount == nullptr);
       if (chand->registered_methods[i].has_host) {
         grpc_slice_unref_internal(chand->registered_methods[i].host);
+        GPR_DEBUG_ASSERT(chand->registered_methods[i].host.refcount ==
+                             &grpc_core::kNoopRefcount ||
+                         chand->registered_methods[i].host.refcount == nullptr);
       }
     }
     gpr_free(chand->registered_methods);
@@ -1202,18 +1208,13 @@ void grpc_server_setup_transport(
     chand->registered_methods =
         static_cast<channel_registered_method*>(gpr_zalloc(alloc));
     for (rm = s->registered_methods; rm; rm = rm->next) {
-      grpc_slice host;
-      bool has_host;
-      grpc_slice method;
-      if (rm->host != nullptr) {
-        host = grpc_slice_from_static_string(rm->host);
-        has_host = true;
-      } else {
-        has_host = false;
+      grpc_core::ExternallyManagedSlice host;
+      grpc_core::ExternallyManagedSlice method(rm->method);
+      const bool has_host = rm->host != nullptr;
+      if (has_host) {
+        host = grpc_core::ExternallyManagedSlice(rm->host);
       }
-      method = grpc_slice_from_static_string(rm->method);
-      hash = GRPC_MDSTR_KV_HASH(has_host ? grpc_slice_hash_internal(host) : 0,
-                                grpc_slice_hash_internal(method));
+      hash = GRPC_MDSTR_KV_HASH(has_host ? host.Hash() : 0, method.Hash());
       for (probes = 0; chand->registered_methods[(hash + probes) % slots]
                            .server_registered_method != nullptr;
            probes++)