Don't pass std::string by value to AttributeValueRef() (#41)

This causes an accidental copy of the string, and (with VS2017) frees
that string before it's used:
#22

* Add benchmarks for AttributeValueRef constructor.

* Add tests for StrCat()ed keys and values.

Author: g-easy

opencensus/trace/BUILD

@@ -269,6 +269,20 @@ cc_test(
 # Benchmarks
 # ========================================================================= #
 #
+
+cc_binary(
+    name = "attribute_value_ref_benchmark",
+    testonly = 1,
+    srcs = ["internal/attribute_value_ref_benchmark.cc"],
+    copts = TEST_COPTS,
+    linkopts = ["-pthread"],  # Required for absl/synchronization bits.
+    linkstatic = 1,
+    deps = [
+        ":trace",
+        "@com_google_benchmark//:benchmark",
+    ],
+)
+
 cc_binary(
     name = "span_benchmark",
     testonly = 1,

opencensus/trace/attribute_value_ref.h

@@ -48,12 +48,19 @@ class AttributeValueRef final {
   // We provide implicit constructors for string, int, and bool. We work around
   // the compiler coercing things like pointers and ints to bool.
 
-  // Construct from anything that converts to absl::string_view. Otherwise we
-  // have to handle const char[] separately.
-  template <typename T, typename std::enable_if<std::is_constructible<
-                            absl::string_view, T>::value>::type* = nullptr>
-  AttributeValueRef(T string_value)
-      : string_value_(absl::string_view(string_value)), type_(Type::kString) {}
+  // Construct from absl::string_view.
+  AttributeValueRef(absl::string_view string_value)
+      : string_value_(string_value), type_(Type::kString) {}
+
+  // Construct from C-style string.
+  AttributeValueRef(const char* string_value)
+      : string_value_(string_value), type_(Type::kString) {}
+
+  // Construct from std::string.
+  template <typename Allocator>
+  AttributeValueRef(const std::basic_string<char, std::char_traits<char>,
+                                            Allocator>& string_value)
+      : string_value_(string_value), type_(Type::kString) {}
 
   // Construct from integer. We have to supply this form explicitly because
   // otherwise AttributeValueRef(int) is ambiguous between int and bool!

opencensus/trace/internal/attribute_value_ref_benchmark.cc

@@ -0,0 +1,42 @@
+// Copyright 2018, OpenCensus 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.
+
+#include "benchmark/benchmark.h"
+#include "opencensus/trace/attribute_value_ref.h"
+
+namespace opencensus {
+namespace trace {
+namespace {
+
+void BM_ConstructFromLiteral(benchmark::State& state) {
+  while (state.KeepRunning()) {
+    benchmark::DoNotOptimize(
+        AttributeValueRef("literal string, showing no string copy happens"));
+  }
+}
+BENCHMARK(BM_ConstructFromLiteral);
+
+void BM_ConstructFromString(benchmark::State& state) {
+  std::string s(8192, 'a');
+  while (state.KeepRunning()) {
+    benchmark::DoNotOptimize(AttributeValueRef(s));
+  }
+}
+BENCHMARK(BM_ConstructFromString);
+
+}  // namespace
+}  // namespace trace
+}  // namespace opencensus
+
+BENCHMARK_MAIN();

opencensus/trace/internal/span_test.cc

@@ -16,6 +16,7 @@
 
 #include <cstdint>
 
+#include "absl/strings/str_cat.h"
 #include "gtest/gtest.h"
 #include "opencensus/trace/attribute_value_ref.h"
 #include "opencensus/trace/exporter/attribute_value.h"
@@ -174,6 +175,18 @@ TEST(SpanTest, FullSpanTest) {
   span.AddAttribute("key2", "value2");
   span.AddAttributes({{"key3", "value3"}, {"key4", 123}, {"key5", false}});
 
+  // String (as opposed to literal).
+  std::string val = "value";
+  val += "6";
+  span.AddAttribute("key6", val);
+
+  // Test that StrCat's output outlives the AddAttribute call.
+  span.AddAttribute(absl::StrCat("ke", "y7"), absl::StrCat("val", "ue7"));
+  span.AddAttributes(
+      {{absl::StrCat("key", "10"), "value10"},
+       {"key11", absl::StrCat("value", "11")},
+       {absl::StrCat("key", "12"), absl::StrCat("value", "12")}});
+
   span.AddAnnotation("anno1");
   span.AddAnnotation(
       "anno2", {{"str_attr", "hello"}, {"int_attr", 123}, {"bool_attr", true}});
@@ -212,6 +225,11 @@ TEST(SpanTest, FullSpanTest) {
   EXPECT_EQ("value3", attributes.at("key3").string_value());
   EXPECT_EQ(123, attributes.at("key4").int_value());
   EXPECT_EQ(false, attributes.at("key5").bool_value());
+  EXPECT_EQ("value6", attributes.at("key6").string_value());
+  EXPECT_EQ("value7", attributes.at("key7").string_value());
+  EXPECT_EQ("value10", attributes.at("key10").string_value());
+  EXPECT_EQ("value11", attributes.at("key11").string_value());
+  EXPECT_EQ("value12", attributes.at("key12").string_value());
   EXPECT_EQ(attributes.end(), attributes.find("key_invalid"));
 
   EXPECT_EQ(2, data.annotations().events().size());