From 2acf901aecc6ba984d8dc6621a47832b83b98219 Mon Sep 17 00:00:00 2001 From: code Date: Tue, 10 Sep 2024 22:49:13 +0800 Subject: [PATCH] json: make the streamer a template class (#36001) Commit Message: json: make the streamer a template class Additional Description: This PR make Streamer a template to accept different types of output (Buffer::Instance, std::string, etc.) Risk Level: low. Testing: unit. Docs Changes: n/a. Release Notes: n/a. Platform Specific Features: n/a. --------- Signed-off-by: wangbaiping Signed-off-by: wangbaiping --- source/common/buffer/BUILD | 1 - source/common/buffer/buffer_util.cc | 45 ---------- source/common/buffer/buffer_util.h | 38 ++++++++- source/common/json/json_streamer.h | 74 +++++++++++------ test/common/json/json_streamer_test.cc | 110 ++++++++++++++----------- 5 files changed, 148 insertions(+), 120 deletions(-) delete mode 100644 source/common/buffer/buffer_util.cc diff --git a/source/common/buffer/BUILD b/source/common/buffer/BUILD index 0545b3b1558d..8236df805fdb 100644 --- a/source/common/buffer/BUILD +++ b/source/common/buffer/BUILD @@ -46,7 +46,6 @@ envoy_cc_library( envoy_cc_library( name = "buffer_util_lib", - srcs = ["buffer_util.cc"], hdrs = ["buffer_util.h"], deps = [ "//envoy/buffer:buffer_interface", diff --git a/source/common/buffer/buffer_util.cc b/source/common/buffer/buffer_util.cc deleted file mode 100644 index 849549714527..000000000000 --- a/source/common/buffer/buffer_util.cc +++ /dev/null @@ -1,45 +0,0 @@ -#include "source/common/buffer/buffer_util.h" - -#include -#include - -#include "source/common/common/macros.h" - -namespace Envoy { -namespace Buffer { - -void Util::serializeDouble(double number, Buffer::Instance& buffer) { - // Converting a double to a string: who would think it would be so complex? - // It's easy if you don't care about speed or accuracy :). Here we are measuring - // the speed with test/server/admin/stats_handler_speed_test --benchmark_filter=BM_HistogramsJson - // Here are some options: - // * absl::StrCat(number) -- fast (19ms on speed test) but loses precision (drops decimals). - // * absl::StrFormat("%.15g") -- works great but a bit slow (24ms on speed test) - // * `snprintf`(buf, sizeof(buf), "%.15g", ...) -- works but slow as molasses: 30ms. - // * fmt::format("{}") -- works great and is a little faster than absl::StrFormat: 21ms. - // * fmt::to_string -- works great and is a little faster than fmt::format: 19ms. - // * std::to_chars -- fast (16ms) and precise, but requires a few lines to - // generate the string_view, and does not work on all platforms yet. - // - // The accuracy is checked in buffer_util_test. -#if defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 14000 - // This version is awkward, and doesn't work on all platforms used in Envoy CI - // as of August 2023, but it is the fastest correct option on modern compilers. - char buf[100]; - std::to_chars_result result = std::to_chars(buf, buf + sizeof(buf), number); - ENVOY_BUG(result.ec == std::errc{}, std::make_error_code(result.ec).message()); - buffer.addFragments({absl::string_view(buf, result.ptr - buf)}); - - // Note: there is room to speed this up further by serializing the number directly - // into the buffer. However, buffer does not currently make it easy and fast - // to get (say) 100 characters of raw buffer to serialize into. -#else - // On older compilers, such as those found on Apple, and gcc, std::to_chars - // does not work with 'double', so we revert to the next fastest correct - // implementation. - buffer.addFragments({fmt::to_string(number)}); -#endif -} - -} // namespace Buffer -} // namespace Envoy diff --git a/source/common/buffer/buffer_util.h b/source/common/buffer/buffer_util.h index 9e8227fa1238..7a8e3aef3c07 100644 --- a/source/common/buffer/buffer_util.h +++ b/source/common/buffer/buffer_util.h @@ -1,7 +1,12 @@ #pragma once +#include +#include + #include "envoy/buffer/buffer.h" +#include "source/common/common/macros.h" + namespace Envoy { namespace Buffer { @@ -20,7 +25,38 @@ class Util { * @param number the number to convert. * @param buffer the buffer in which to write the double. */ - static void serializeDouble(double number, Buffer::Instance& buffer); + template static void serializeDouble(double number, Output& buffer) { + // Converting a double to a string: who would think it would be so complex? + // It's easy if you don't care about speed or accuracy :). Here we are measuring + // the speed with test/server/admin/stats_handler_speed_test + // --benchmark_filter=BM_HistogramsJson Here are some options: + // * absl::StrCat(number) -- fast (19ms on speed test) but loses precision (drops decimals). + // * absl::StrFormat("%.15g") -- works great but a bit slow (24ms on speed test) + // * `snprintf`(buf, sizeof(buf), "%.15g", ...) -- works but slow as molasses: 30ms. + // * fmt::format("{}") -- works great and is a little faster than absl::StrFormat: 21ms. + // * fmt::to_string -- works great and is a little faster than fmt::format: 19ms. + // * std::to_chars -- fast (16ms) and precise, but requires a few lines to + // generate the string_view, and does not work on all platforms yet. + // + // The accuracy is checked in buffer_util_test. +#if defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 14000 + // This version is awkward, and doesn't work on all platforms used in Envoy CI + // as of August 2023, but it is the fastest correct option on modern compilers. + char buf[100]; + std::to_chars_result result = std::to_chars(buf, buf + sizeof(buf), number); + ENVOY_BUG(result.ec == std::errc{}, std::make_error_code(result.ec).message()); + buffer.add(absl::string_view(buf, result.ptr - buf)); + + // Note: there is room to speed this up further by serializing the number directly + // into the buffer. However, buffer does not currently make it easy and fast + // to get (say) 100 characters of raw buffer to serialize into. +#else + // On older compilers, such as those found on Apple, and gcc, std::to_chars + // does not work with 'double', so we revert to the next fastest correct + // implementation. + buffer.add(fmt::to_string(number)); +#endif + } }; } // namespace Buffer diff --git a/source/common/json/json_streamer.h b/source/common/json/json_streamer.h index 97d7ef424d54..8087cbf4ada6 100644 --- a/source/common/json/json_streamer.h +++ b/source/common/json/json_streamer.h @@ -39,7 +39,9 @@ namespace Json { #define ASSERT_LEVELS_EMPTY ASSERT(this->levels_.empty()) #endif -// Simple abstraction that provide a output buffer for streaming JSON output. +// Buffer wrapper that implements the necessary abstraction for the template +// StreamerBase. +// This could be used to stream JSON output of StreamerBase to a Buffer. class BufferOutput { public: void add(absl::string_view a) { buffer_.addFragments({a}); } @@ -51,24 +53,41 @@ class BufferOutput { Buffer::Instance& buffer_; }; +// String wrapper that implements the necessary abstraction for the template +// StreamerBase. +// This could be used to stream JSON output of StreamerBase to a single string. +class StringOutput { +public: + void add(absl::string_view a) { buffer_.append(a); } + void add(absl::string_view a, absl::string_view b, absl::string_view c) { + absl::StrAppend(&buffer_, a, b, c); + } + explicit StringOutput(std::string& output) : buffer_(output) {} + + std::string& buffer_; +}; + /** * Provides an API for streaming JSON output, as an alternative to populating a * JSON structure with an image of what you want to serialize, or using a * protobuf with reflection. The advantage of this approach is that it does not * require building an intermediate data structure with redundant copies of all * strings, maps, and arrays. + * + * NOTE: This template take a type that can be used to stream output. This is either + * BufferOutput, StringOutput or any other types that have implemented + * add(absl::string_view) and + * add(absl::string_view, absl::string_view, absl::string_view) methods. */ -class Streamer { +template class StreamerBase { public: using Value = absl::variant; /** - * @param response The buffer in which to stream output. Note: this buffer can - * be flushed during population; it is not necessary to hold - * the entire json structure in memory before streaming it to - * the network. + * @param response The buffer in which to stream output. + * NOTE: The response must could be used to construct instance of OutputBufferType. */ - explicit Streamer(Buffer::Instance& response) : response_(response) {} + template explicit StreamerBase(T& response) : response_(response) {} class Array; using ArrayPtr = std::unique_ptr; @@ -81,15 +100,15 @@ class Streamer { */ class Level { public: - Level(Streamer& streamer, absl::string_view opener, absl::string_view closer) + Level(StreamerBase& streamer, absl::string_view opener, absl::string_view closer) : streamer_(streamer), closer_(closer) { - streamer_.addConstantString(opener); + streamer_.addWithoutSanitizing(opener); #ifndef NDEBUG streamer_.push(this); #endif } virtual ~Level() { - streamer_.addConstantString(closer_); + streamer_.addWithoutSanitizing(closer_); #ifndef NDEBUG streamer_.pop(this); #endif @@ -180,7 +199,7 @@ class Streamer { if (is_first_) { is_first_ = false; } else { - streamer_.addConstantString(","); + streamer_.addWithoutSanitizing(","); } } @@ -223,11 +242,9 @@ class Streamer { } } - private: - friend Streamer; - + protected: bool is_first_{true}; // Used to control whether a comma-separator is added for a new entry. - Streamer& streamer_; + StreamerBase& streamer_; absl::string_view closer_; }; using LevelPtr = std::unique_ptr; @@ -241,7 +258,7 @@ class Streamer { using NameValue = std::pair; using Entries = absl::Span; - Map(Streamer& streamer) : Level(streamer, "{", "}") {} + Map(StreamerBase& streamer) : Level(streamer, "{", "}") {} /** * Initiates a new map key. This must be followed by rendering a value, @@ -292,7 +309,7 @@ class Streamer { */ class Array : public Level { public: - Array(Streamer& streamer) : Level(streamer, "[", "]") {} + Array(StreamerBase& streamer) : Level(streamer, "[", "]") {} using Entries = absl::Span; /** @@ -333,11 +350,6 @@ class Streamer { return std::make_unique(*this); } -private: - friend Level; - friend Map; - friend Array; - /** * Takes a raw string, sanitizes it using JSON syntax, surrounds it * with a prefix and suffix, and streams it out. @@ -361,7 +373,7 @@ class Streamer { if (std::isnan(d)) { response_.add(Constants::Null); } else { - Buffer::Util::serializeDouble(d, response_.buffer_); + Buffer::Util::serializeDouble(d, response_); } } void addNumber(uint64_t u) { response_.add(absl::StrCat(u)); } @@ -373,11 +385,14 @@ class Streamer { void addBool(bool b) { response_.add(b ? Constants::True : Constants::False); } /** - * Adds a constant string to the output stream. The string must outlive the - * Streamer object, and is intended for literal strings such as punctuation. + * Adds a pre-sanitized string or which doesn't require sanitizing to the output stream. + * NOTE: use this with care as it bypasses the sanitization process and may result in + * invalid JSON. If you are not sure if the string is already sanitized, use addString() + * or addSanitized() instead. */ - void addConstantString(absl::string_view str) { response_.add(str); } + void addWithoutSanitizing(absl::string_view str) { response_.add(str); } +private: #ifndef NDEBUG /** * @return the top Level*. This is used for asserts. @@ -399,7 +414,7 @@ class Streamer { #endif - BufferOutput response_; + OutputBufferType response_; std::string sanitize_buffer_; #ifndef NDEBUG @@ -409,5 +424,10 @@ class Streamer { #endif }; +/** + * A Streamer that streams to a Buffer::Instance. + */ +using Streamer = StreamerBase; + } // namespace Json } // namespace Envoy diff --git a/test/common/json/json_streamer_test.cc b/test/common/json/json_streamer_test.cc index 231fc33d1ba7..0e332c0ccd8f 100644 --- a/test/common/json/json_streamer_test.cc +++ b/test/common/json/json_streamer_test.cc @@ -11,124 +11,142 @@ namespace Envoy { namespace Json { namespace { -class JsonStreamerTest : public testing::Test { -protected: - Buffer::OwnedImpl buffer_; - Json::Streamer streamer_{buffer_}; +class BufferOutputWrapper { +public: + using Type = BufferOutput; + std::string toString() { return underlying_buffer_.toString(); } + Buffer::OwnedImpl underlying_buffer_; }; -TEST_F(JsonStreamerTest, Empty) { EXPECT_EQ("", buffer_.toString()); } +class StringOutputWrapper { +public: + using Type = StringOutput; + std::string toString() { return underlying_buffer_; } + std::string underlying_buffer_; +}; + +template class JsonStreamerTest : public testing::Test { +public: + T buffer_; + Json::StreamerBase streamer_{this->buffer_.underlying_buffer_}; +}; + +using OutputBufferTypes = ::testing::Types; +TYPED_TEST_SUITE(JsonStreamerTest, OutputBufferTypes); + +TYPED_TEST(JsonStreamerTest, Empty) { EXPECT_EQ("", this->buffer_.toString()); } -TEST_F(JsonStreamerTest, EmptyMap) { - streamer_.makeRootMap(); - EXPECT_EQ("{}", buffer_.toString()); +TYPED_TEST(JsonStreamerTest, EmptyMap) { + this->streamer_.makeRootMap(); + EXPECT_EQ("{}", this->buffer_.toString()); } -TEST_F(JsonStreamerTest, MapOneDouble) { +TYPED_TEST(JsonStreamerTest, MapOneDouble) { { - Streamer::MapPtr map = streamer_.makeRootMap(); + auto map = this->streamer_.makeRootMap(); map->addEntries({{"a", 3.141592654}}); } - EXPECT_EQ(R"EOF({"a":3.141592654})EOF", buffer_.toString()); + EXPECT_EQ(R"EOF({"a":3.141592654})EOF", this->buffer_.toString()); } -TEST_F(JsonStreamerTest, MapTwoDoubles) { +TYPED_TEST(JsonStreamerTest, MapTwoDoubles) { { - Streamer::MapPtr map = streamer_.makeRootMap(); + auto map = this->streamer_.makeRootMap(); map->addEntries({{"a", -989282.1087}, {"b", 1.23456789012345e+67}}); } - EXPECT_EQ(R"EOF({"a":-989282.1087,"b":1.23456789012345e+67})EOF", buffer_.toString()); + EXPECT_EQ(R"EOF({"a":-989282.1087,"b":1.23456789012345e+67})EOF", this->buffer_.toString()); } -TEST_F(JsonStreamerTest, MapOneUInt) { +TYPED_TEST(JsonStreamerTest, MapOneUInt) { { - Streamer::MapPtr map = streamer_.makeRootMap(); + auto map = this->streamer_.makeRootMap(); map->addEntries({{"a", static_cast(0xffffffffffffffff)}}); } - EXPECT_EQ(R"EOF({"a":18446744073709551615})EOF", buffer_.toString()); + EXPECT_EQ(R"EOF({"a":18446744073709551615})EOF", this->buffer_.toString()); } -TEST_F(JsonStreamerTest, MapTwoInts) { +TYPED_TEST(JsonStreamerTest, MapTwoInts) { { - Streamer::MapPtr map = streamer_.makeRootMap(); + auto map = this->streamer_.makeRootMap(); map->addEntries({{"a", static_cast(0x7fffffffffffffff)}, {"b", static_cast(0x8000000000000000)}}); } - EXPECT_EQ(R"EOF({"a":9223372036854775807,"b":-9223372036854775808})EOF", buffer_.toString()); + EXPECT_EQ(R"EOF({"a":9223372036854775807,"b":-9223372036854775808})EOF", + this->buffer_.toString()); } -TEST_F(JsonStreamerTest, MapOneString) { +TYPED_TEST(JsonStreamerTest, MapOneString) { { - Streamer::MapPtr map = streamer_.makeRootMap(); + auto map = this->streamer_.makeRootMap(); map->addEntries({{"a", "b"}}); } - EXPECT_EQ(R"EOF({"a":"b"})EOF", buffer_.toString()); + EXPECT_EQ(R"EOF({"a":"b"})EOF", this->buffer_.toString()); } -TEST_F(JsonStreamerTest, MapOneBool) { +TYPED_TEST(JsonStreamerTest, MapOneBool) { { - Streamer::MapPtr map = streamer_.makeRootMap(); + auto map = this->streamer_.makeRootMap(); map->addEntries({{"a", true}}); } - EXPECT_EQ(R"EOF({"a":true})EOF", buffer_.toString()); + EXPECT_EQ(R"EOF({"a":true})EOF", this->buffer_.toString()); } -TEST_F(JsonStreamerTest, MapTwoBools) { +TYPED_TEST(JsonStreamerTest, MapTwoBools) { { - Streamer::MapPtr map = streamer_.makeRootMap(); + auto map = this->streamer_.makeRootMap(); map->addEntries({{"a", true}, {"b", false}}); } - EXPECT_EQ(R"EOF({"a":true,"b":false})EOF", buffer_.toString()); + EXPECT_EQ(R"EOF({"a":true,"b":false})EOF", this->buffer_.toString()); } -TEST_F(JsonStreamerTest, MapOneSanitized) { +TYPED_TEST(JsonStreamerTest, MapOneSanitized) { { - Streamer::MapPtr map = streamer_.makeRootMap(); + auto map = this->streamer_.makeRootMap(); map->addKey("a"); map->addString("\b\001"); } - EXPECT_EQ(R"EOF({"a":"\b\u0001"})EOF", buffer_.toString()); + EXPECT_EQ(R"EOF({"a":"\b\u0001"})EOF", this->buffer_.toString()); } -TEST_F(JsonStreamerTest, MapTwoSanitized) { +TYPED_TEST(JsonStreamerTest, MapTwoSanitized) { { - Streamer::MapPtr map = streamer_.makeRootMap(); + auto map = this->streamer_.makeRootMap(); map->addKey("a"); map->addString("\b\001"); map->addKey("b"); map->addString("\r\002"); } - EXPECT_EQ(R"EOF({"a":"\b\u0001","b":"\r\u0002"})EOF", buffer_.toString()); + EXPECT_EQ(R"EOF({"a":"\b\u0001","b":"\r\u0002"})EOF", this->buffer_.toString()); } -TEST_F(JsonStreamerTest, SubArray) { - Streamer::MapPtr map = streamer_.makeRootMap(); +TYPED_TEST(JsonStreamerTest, SubArray) { + auto map = this->streamer_.makeRootMap(); map->addKey("a"); - Streamer::ArrayPtr array = map->addArray(); + auto array = map->addArray(); array->addEntries({1.0, "two", 3.5, true, false, std::nan("")}); array.reset(); map->addEntries({{"embedded\"quote", "value"}}); map.reset(); EXPECT_EQ(R"EOF({"a":[1,"two",3.5,true,false,null],"embedded\"quote":"value"})EOF", - buffer_.toString()); + this->buffer_.toString()); } -TEST_F(JsonStreamerTest, TopArray) { +TYPED_TEST(JsonStreamerTest, TopArray) { { - Streamer::ArrayPtr array = streamer_.makeRootArray(); + auto array = this->streamer_.makeRootArray(); array->addEntries({1.0, "two", 3.5, true, false, std::nan("")}); } - EXPECT_EQ(R"EOF([1,"two",3.5,true,false,null])EOF", buffer_.toString()); + EXPECT_EQ(R"EOF([1,"two",3.5,true,false,null])EOF", this->buffer_.toString()); } -TEST_F(JsonStreamerTest, SubMap) { - Streamer::MapPtr map = streamer_.makeRootMap(); +TYPED_TEST(JsonStreamerTest, SubMap) { + auto map = this->streamer_.makeRootMap(); map->addKey("a"); - Streamer::MapPtr sub_map = map->addMap(); + auto sub_map = map->addMap(); sub_map->addEntries({{"one", 1.0}, {"three.5", 3.5}}); sub_map.reset(); map.reset(); - EXPECT_EQ(R"EOF({"a":{"one":1,"three.5":3.5}})EOF", buffer_.toString()); + EXPECT_EQ(R"EOF({"a":{"one":1,"three.5":3.5}})EOF", this->buffer_.toString()); } } // namespace