Merge pull request #19288 from soheilhy/slice-move

Introduce grpc_slice_from_moved_string.

src/core/ext/transport/chttp2/transport/frame_data.cc

@@ -109,7 +109,6 @@ grpc_error* grpc_deframe_unprocessed_incoming_frames(
     end = GRPC_SLICE_END_PTR(*slice);
     cur = beg;
     uint32_t message_flags;
-    char* msg;
 
     if (cur == end) {
       grpc_slice_buffer_remove_first(slices);
@@ -132,15 +131,16 @@ grpc_error* grpc_deframe_unprocessed_incoming_frames(
             p->is_frame_compressed = true; /* GPR_TRUE */
             break;
           default:
+            char* msg;
             gpr_asprintf(&msg, "Bad GRPC frame type 0x%02x", p->frame_type);
             p->error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(msg);
             p->error = grpc_error_set_int(p->error, GRPC_ERROR_INT_STREAM_ID,
                                           static_cast<intptr_t>(s->id));
             gpr_free(msg);
-            msg = grpc_dump_slice(*slice, GPR_DUMP_HEX | GPR_DUMP_ASCII);
-            p->error = grpc_error_set_str(p->error, GRPC_ERROR_STR_RAW_BYTES,
-                                          grpc_slice_from_copied_string(msg));
-            gpr_free(msg);
+            p->error =
+                grpc_error_set_str(p->error, GRPC_ERROR_STR_RAW_BYTES,
+                                   grpc_dump_slice_to_slice(
+                                       *slice, GPR_DUMP_HEX | GPR_DUMP_ASCII));
             p->error =
                 grpc_error_set_int(p->error, GRPC_ERROR_INT_OFFSET, cur - beg);
             p->state = GRPC_CHTTP2_DATA_ERROR;

src/core/ext/transport/chttp2/transport/frame_rst_stream.cc

@@ -26,6 +26,7 @@
 #include <grpc/support/string_util.h>
 
 #include "src/core/ext/transport/chttp2/transport/frame.h"
+#include "src/core/lib/gprpp/memory.h"
 #include "src/core/lib/transport/http2_errors.h"
 
 grpc_slice grpc_chttp2_rst_stream_create(uint32_t id, uint32_t code,
@@ -102,9 +103,9 @@ grpc_error* grpc_chttp2_rst_stream_parser_parse(void* parser,
       error = grpc_error_set_int(
           grpc_error_set_str(GRPC_ERROR_CREATE_FROM_STATIC_STRING("RST_STREAM"),
                              GRPC_ERROR_STR_GRPC_MESSAGE,
-                             grpc_slice_from_copied_string(message)),
+                             grpc_slice_from_moved_string(
+                                 grpc_core::UniquePtr<char>(message))),
           GRPC_ERROR_INT_HTTP2_ERROR, static_cast<intptr_t>(reason));
-      gpr_free(message);
     }
     grpc_chttp2_mark_stream_closed(t, s, true, true, error);
   }

src/core/lib/gpr/string.cc

@@ -126,7 +126,8 @@ static void asciidump(dump_out* out, const char* buf, size_t len) {
   }
 }
 
-char* gpr_dump(const char* buf, size_t len, uint32_t flags) {
+char* gpr_dump_return_len(const char* buf, size_t len, uint32_t flags,
+                          size_t* out_len) {
   dump_out out = dump_out_create();
   if (flags & GPR_DUMP_HEX) {
     hexdump(&out, buf, len);
@@ -135,9 +136,15 @@ char* gpr_dump(const char* buf, size_t len, uint32_t flags) {
     asciidump(&out, buf, len);
   }
   dump_out_append(&out, 0);
+  *out_len = out.length;
   return out.data;
 }
 
+char* gpr_dump(const char* buf, size_t len, uint32_t flags) {
+  size_t unused;
+  return gpr_dump_return_len(buf, len, flags, &unused);
+}
+
 int gpr_parse_bytes_to_uint32(const char* buf, size_t len, uint32_t* result) {
   uint32_t out = 0;
   uint32_t new_val;

src/core/lib/gpr/string.h

@@ -35,6 +35,11 @@
 /* Converts array buf, of length len, into a C string according to the flags.
    Result should be freed with gpr_free() */
 char* gpr_dump(const char* buf, size_t len, uint32_t flags);
+/* Converts array buf, of length len, into a C string according to the flags.
+   The length of the returned buffer is stored in out_len.
+   Result should be freed with gpr_free() */
+char* gpr_dump_return_len(const char* buf, size_t len, uint32_t flags,
+                          size_t* out_len);
 
 /* Parses an array of bytes into an integer (base 10). Returns 1 on success,
    0 on failure. */

src/core/lib/http/httpcli.cc

@@ -28,6 +28,7 @@
 
 #include "src/core/lib/channel/channel_args.h"
 #include "src/core/lib/gpr/string.h"
+#include "src/core/lib/gprpp/memory.h"
 #include "src/core/lib/http/format_request.h"
 #include "src/core/lib/http/parser.h"
 #include "src/core/lib/iomgr/endpoint.h"
@@ -112,12 +113,11 @@ static void append_error(internal_request* req, grpc_error* error) {
         GRPC_ERROR_CREATE_FROM_STATIC_STRING("Failed HTTP/1 client request");
   }
   grpc_resolved_address* addr = &req->addresses->addrs[req->next_address - 1];
-  char* addr_text = grpc_sockaddr_to_uri(addr);
+  grpc_core::UniquePtr<char> addr_text(grpc_sockaddr_to_uri(addr));
   req->overall_error = grpc_error_add_child(
       req->overall_error,
       grpc_error_set_str(error, GRPC_ERROR_STR_TARGET_ADDRESS,
-                         grpc_slice_from_copied_string(addr_text)));
-  gpr_free(addr_text);
+                         grpc_slice_from_moved_string(std::move(addr_text))));
 }
 
 static void do_read(internal_request* req) {

src/core/lib/slice/slice.cc

@@ -102,18 +102,19 @@ class NewSliceRefcount {
   }
 
   NewSliceRefcount(void (*destroy)(void*), void* user_data)
-      : rc_(grpc_slice_refcount::Type::REGULAR, &refs_, Destroy, this, &rc_),
+      : base_(grpc_slice_refcount::Type::REGULAR, &refs_, Destroy, this,
+              &base_),
         user_destroy_(destroy),
         user_data_(user_data) {}
 
   GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
 
-  grpc_slice_refcount* base_refcount() { return &rc_; }
+  grpc_slice_refcount* base_refcount() { return &base_; }
 
  private:
   ~NewSliceRefcount() { user_destroy_(user_data_); }
 
-  grpc_slice_refcount rc_;
+  grpc_slice_refcount base_;
   RefCount refs_;
   void (*user_destroy_)(void*);
   void* user_data_;
@@ -141,7 +142,6 @@ grpc_slice grpc_slice_new(void* p, size_t len, void (*destroy)(void*)) {
 namespace grpc_core {
 /* grpc_slice_new_with_len support structures - we create a refcount object
    extended with the user provided data pointer & destroy function */
-
 class NewWithLenSliceRefcount {
  public:
   static void Destroy(void* arg) {
@@ -150,25 +150,48 @@ class NewWithLenSliceRefcount {
 
   NewWithLenSliceRefcount(void (*destroy)(void*, size_t), void* user_data,
                           size_t user_length)
-      : rc_(grpc_slice_refcount::Type::REGULAR, &refs_, Destroy, this, &rc_),
+      : base_(grpc_slice_refcount::Type::REGULAR, &refs_, Destroy, this,
+              &base_),
         user_data_(user_data),
         user_length_(user_length),
         user_destroy_(destroy) {}
 
   GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
 
-  grpc_slice_refcount* base_refcount() { return &rc_; }
+  grpc_slice_refcount* base_refcount() { return &base_; }
 
  private:
   ~NewWithLenSliceRefcount() { user_destroy_(user_data_, user_length_); }
 
-  grpc_slice_refcount rc_;
+  grpc_slice_refcount base_;
   RefCount refs_;
   void* user_data_;
   size_t user_length_;
   void (*user_destroy_)(void*, size_t);
 };
 
+/** grpc_slice_from_moved_(string|buffer) ref count .*/
+class MovedStringSliceRefCount {
+ public:
+  MovedStringSliceRefCount(grpc_core::UniquePtr<char>&& str)
+      : base_(grpc_slice_refcount::Type::REGULAR, &refs_, Destroy, this,
+              &base_),
+        str_(std::move(str)) {}
+
+  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
+
+  grpc_slice_refcount* base_refcount() { return &base_; }
+
+ private:
+  static void Destroy(void* arg) {
+    Delete(static_cast<MovedStringSliceRefCount*>(arg));
+  }
+
+  grpc_slice_refcount base_;
+  grpc_core::RefCount refs_;
+  grpc_core::UniquePtr<char> str_;
+};
+
 }  // namespace grpc_core
 
 grpc_slice grpc_slice_new_with_len(void* p, size_t len,
@@ -193,6 +216,29 @@ grpc_slice grpc_slice_from_copied_string(const char* source) {
   return grpc_slice_from_copied_buffer(source, strlen(source));
 }
 
+grpc_slice grpc_slice_from_moved_buffer(grpc_core::UniquePtr<char> p,
+                                        size_t len) {
+  uint8_t* ptr = reinterpret_cast<uint8_t*>(p.get());
+  grpc_slice slice;
+  if (len <= sizeof(slice.data.inlined.bytes)) {
+    slice.refcount = nullptr;
+    slice.data.inlined.length = len;
+    memcpy(GRPC_SLICE_START_PTR(slice), ptr, len);
+  } else {
+    slice.refcount =
+        grpc_core::New<grpc_core::MovedStringSliceRefCount>(std::move(p))
+            ->base_refcount();
+    slice.data.refcounted.bytes = ptr;
+    slice.data.refcounted.length = len;
+  }
+  return slice;
+}
+
+grpc_slice grpc_slice_from_moved_string(grpc_core::UniquePtr<char> p) {
+  const size_t len = strlen(p.get());
+  return grpc_slice_from_moved_buffer(std::move(p), len);
+}
+
 namespace {
 
 class MallocRefCount {

src/core/lib/slice/slice_internal.h

@@ -28,6 +28,7 @@
 #include <string.h>
 
 #include "src/core/lib/gpr/murmur_hash.h"
+#include "src/core/lib/gprpp/memory.h"
 #include "src/core/lib/gprpp/ref_counted.h"
 #include "src/core/lib/transport/static_metadata.h"
 
@@ -302,6 +303,10 @@ inline uint32_t grpc_slice_hash_internal(const grpc_slice& s) {
                                : grpc_slice_hash_refcounted(s);
 }
 
+grpc_slice grpc_slice_from_moved_buffer(grpc_core::UniquePtr<char> p,
+                                        size_t len);
+grpc_slice grpc_slice_from_moved_string(grpc_core::UniquePtr<char> p);
+
 // Returns the memory used by this slice, not counting the slice structure
 // itself. This means that inlined and slices from static strings will return
 // 0. All other slices will return the size of the allocated chars.

src/core/lib/slice/slice_string_helpers.cc

@@ -25,6 +25,7 @@
 #include <grpc/support/log.h>
 
 #include "src/core/lib/gpr/string.h"
+#include "src/core/lib/gprpp/memory.h"
 #include "src/core/lib/slice/slice_internal.h"
 
 char* grpc_dump_slice(const grpc_slice& s, uint32_t flags) {
@@ -32,6 +33,14 @@ char* grpc_dump_slice(const grpc_slice& s, uint32_t flags) {
                   GRPC_SLICE_LENGTH(s), flags);
 }
 
+grpc_slice grpc_dump_slice_to_slice(const grpc_slice& s, uint32_t flags) {
+  size_t len;
+  grpc_core::UniquePtr<char> ptr(
+      gpr_dump_return_len(reinterpret_cast<const char*> GRPC_SLICE_START_PTR(s),
+                          GRPC_SLICE_LENGTH(s), flags, &len));
+  return grpc_slice_from_moved_buffer(std::move(ptr), len);
+}
+
 /** Finds the initial (\a begin) and final (\a end) offsets of the next
  * substring from \a str + \a read_offset until the next \a sep or the end of \a
  * str.

src/core/lib/slice/slice_string_helpers.h

@@ -31,6 +31,8 @@
 
 /* Calls gpr_dump on a slice. */
 char* grpc_dump_slice(const grpc_slice& slice, uint32_t flags);
+/* Calls gpr_dump on a slice and returns the result as a slice. */
+grpc_slice grpc_dump_slice_to_slice(const grpc_slice& slice, uint32_t flags);
 
 /** Split \a str by the separator \a sep. Results are stored in \a dst, which
  * should be a properly initialized instance. */

src/core/lib/surface/validate_metadata.cc

@@ -24,6 +24,7 @@
 #include <grpc/grpc.h>
 #include <grpc/support/alloc.h>
 
+#include "src/core/lib/gprpp/memory.h"
 #include "src/core/lib/iomgr/error.h"
 #include "src/core/lib/slice/slice_internal.h"
 #include "src/core/lib/slice/slice_string_helpers.h"
@@ -39,13 +40,12 @@ static grpc_error* conforms_to(const grpc_slice& slice,
     int byte = idx / 8;
     int bit = idx % 8;
     if ((legal_bits[byte] & (1 << bit)) == 0) {
-      char* dump = grpc_dump_slice(slice, GPR_DUMP_HEX | GPR_DUMP_ASCII);
       grpc_error* error = grpc_error_set_str(
           grpc_error_set_int(GRPC_ERROR_CREATE_FROM_COPIED_STRING(err_desc),
                              GRPC_ERROR_INT_OFFSET,
                              p - GRPC_SLICE_START_PTR(slice)),
-          GRPC_ERROR_STR_RAW_BYTES, grpc_slice_from_copied_string(dump));
-      gpr_free(dump);
+          GRPC_ERROR_STR_RAW_BYTES,
+          grpc_dump_slice_to_slice(slice, GPR_DUMP_HEX | GPR_DUMP_ASCII));
       return error;
     }
   }

test/core/slice/slice_test.cc

@@ -27,6 +27,7 @@
 #include <grpc/support/alloc.h>
 #include <grpc/support/log.h>
 
+#include "src/core/lib/gprpp/memory.h"
 #include "src/core/lib/slice/slice_internal.h"
 #include "src/core/lib/transport/static_metadata.h"
 #include "test/core/util/test_config.h"
@@ -285,6 +286,44 @@ static void test_static_slice_copy_interning(void) {
   grpc_shutdown();
 }
 
+static void test_moved_string_slice(void) {
+  LOG_TEST_NAME("test_moved_string_slice");
+
+  grpc_init();
+
+  // Small string should be inlined.
+  constexpr char kSmallStr[] = "hello12345";
+  char* small_ptr = strdup(kSmallStr);
+  grpc_slice small =
+      grpc_slice_from_moved_string(grpc_core::UniquePtr<char>(small_ptr));
+  GPR_ASSERT(GRPC_SLICE_LENGTH(small) == strlen(kSmallStr));
+  GPR_ASSERT(GRPC_SLICE_START_PTR(small) !=
+             reinterpret_cast<uint8_t*>(small_ptr));
+  grpc_slice_unref(small);
+
+  // Large string should be move the reference.
+  constexpr char kSLargeStr[] = "hello123456789123456789123456789";
+  char* large_ptr = strdup(kSLargeStr);
+  grpc_slice large =
+      grpc_slice_from_moved_string(grpc_core::UniquePtr<char>(large_ptr));
+  GPR_ASSERT(GRPC_SLICE_LENGTH(large) == strlen(kSLargeStr));
+  GPR_ASSERT(GRPC_SLICE_START_PTR(large) ==
+             reinterpret_cast<uint8_t*>(large_ptr));
+  grpc_slice_unref(large);
+
+  // Moved buffer must respect the provided length not the actual length of the
+  // string.
+  large_ptr = strdup(kSLargeStr);
+  small = grpc_slice_from_moved_buffer(grpc_core::UniquePtr<char>(large_ptr),
+                                       strlen(kSmallStr));
+  GPR_ASSERT(GRPC_SLICE_LENGTH(small) == strlen(kSmallStr));
+  GPR_ASSERT(GRPC_SLICE_START_PTR(small) !=
+             reinterpret_cast<uint8_t*>(large_ptr));
+  grpc_slice_unref(small);
+
+  grpc_shutdown();
+}
+
 int main(int argc, char** argv) {
   unsigned length;
   grpc::testing::TestEnvironment env(argc, argv);
@@ -302,6 +341,7 @@ int main(int argc, char** argv) {
   test_slice_interning();
   test_static_slice_interning();
   test_static_slice_copy_interning();
+  test_moved_string_slice();
   grpc_shutdown();
   return 0;
 }