From 7492997159bc7c9bbce0a794eec9222dcbae2a6d Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Fri, 26 Oct 2018 19:16:17 +0000 Subject: [PATCH 1/3] Improve performance by removing MD5 for check cache keys Signed-off-by: Wayne Zhang --- include/istio/utils/BUILD | 2 +- include/istio/utils/concat_hash.h | 71 ++++++++++++++++++++++++ include/istio/utils/md5.h | 68 ----------------------- src/istio/mixerclient/BUILD | 1 - src/istio/mixerclient/referenced.cc | 11 ++-- src/istio/mixerclient/referenced.h | 4 +- src/istio/mixerclient/referenced_test.cc | 26 ++++++--- src/istio/utils/BUILD | 25 --------- src/istio/utils/md5.cc | 55 ------------------ src/istio/utils/md5_test.cc | 41 -------------- 10 files changed, 99 insertions(+), 205 deletions(-) create mode 100644 include/istio/utils/concat_hash.h delete mode 100644 include/istio/utils/md5.h delete mode 100644 src/istio/utils/md5.cc delete mode 100644 src/istio/utils/md5_test.cc diff --git a/include/istio/utils/BUILD b/include/istio/utils/BUILD index 3a5045b8486..788c4e8a8c1 100644 --- a/include/istio/utils/BUILD +++ b/include/istio/utils/BUILD @@ -18,8 +18,8 @@ cc_library( name = "headers_lib", hdrs = [ "attributes_builder.h", + "concat_hash.h", "local_attributes.h", - "md5.h", "protobuf.h", "status.h", ], diff --git a/include/istio/utils/concat_hash.h b/include/istio/utils/concat_hash.h new file mode 100644 index 00000000000..6e9dd125a0a --- /dev/null +++ b/include/istio/utils/concat_hash.h @@ -0,0 +1,71 @@ +/* Copyright 2017 Istio Authors. All Rights Reserved. + * + * 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. + */ + +#ifndef ISTIO_UTILS_CONCAT_HASH_H_ +#define ISTIO_UTILS_CONCAT_HASH_H_ + +#include +#include + +namespace istio { +namespace utils { + +// This class concatenates multiple values into a string as hash +class ConcatHash { + public: + ConcatHash(size_t reserve_size) { hash_.reserve(reserve_size); } + + // Updates the context with data. + ConcatHash& Update(const void* data, size_t size) { + hash_.append(static_cast(data), size); + return *this; + } + + // A helper function for int + ConcatHash& Update(int d) { return Update(&d, sizeof(d)); } + + // A helper function for const char* + ConcatHash& Update(const char* str) { + hash_.append(str); + return *this; + } + + // A helper function for const string + ConcatHash& Update(const std::string& str) { + hash_.append(str); + return *this; + } + + // Returns the concated string as hash. + std::string getHash() const { return hash_; } + + // Converts a binary string to a printable string for unit-test only + static std::string DebugString(const std::string& hash) { + char buf[hash.size() * 2]; + char* p = buf; + for (size_t i = 0; i < hash.size(); ++i, p += 2) { + sprintf(p, "%02x", (unsigned char)hash[i]); + } + return std::string(buf, hash.size() * 2); + } + + private: + std::string hash_; +}; + +} // namespace utils +} // namespace istio + +#endif // ISTIO_UTILS_CONCAT_HASH_H_ diff --git a/include/istio/utils/md5.h b/include/istio/utils/md5.h deleted file mode 100644 index 19b1c9dbe8f..00000000000 --- a/include/istio/utils/md5.h +++ /dev/null @@ -1,68 +0,0 @@ -/* Copyright 2017 Istio Authors. All Rights Reserved. - * - * 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. - */ - -#ifndef ISTIO_UTILS_MD5_H_ -#define ISTIO_UTILS_MD5_H_ - -#include -#include -#include "openssl/md5.h" - -namespace istio { -namespace utils { - -// Define a MD5 Digest by calling OpenSSL -class MD5 { - public: - MD5(); - - // Updates the context with data. - MD5& Update(const void* data, size_t size); - - // A helper function for const char* - MD5& Update(const char* str) { return Update(str, strlen(str)); } - - // A helper function for const string - MD5& Update(const std::string& str) { return Update(str.data(), str.size()); } - - // A helper function for int - MD5& Update(int d) { return Update(&d, sizeof(d)); } - - // The MD5 digest is always 128 bits = 16 bytes - static const int kDigestLength = 16; - - // Returns the digest as string. - std::string Digest(); - - // A short form of generating MD5 for a string - std::string operator()(const void* data, size_t size); - - // Converts a binary digest string to a printable string. - // It is for debugging and unit-test only. - static std::string DebugString(const std::string& digest); - - private: - // MD5 context. - MD5_CTX ctx_; - // The final MD5 digest. - unsigned char digest_[kDigestLength]; - // A flag to indicate if MD5_final is called or not. - bool finalized_; -}; - -} // namespace utils -} // namespace istio - -#endif // ISTIO_UTILS_MD5_H_ diff --git a/src/istio/mixerclient/BUILD b/src/istio/mixerclient/BUILD index eb589cf22d5..6d5d8010b38 100644 --- a/src/istio/mixerclient/BUILD +++ b/src/istio/mixerclient/BUILD @@ -58,7 +58,6 @@ cc_library( "//include/istio/quota_config:requirement_header", "//include/istio/utils:simple_lru_cache", "//src/istio/prefetch:quota_prefetch_lib", - "//src/istio/utils:md5_lib", "//src/istio/utils:utils_lib", ], ) diff --git a/src/istio/mixerclient/referenced.cc b/src/istio/mixerclient/referenced.cc index afc51f72527..187ef22b45f 100644 --- a/src/istio/mixerclient/referenced.cc +++ b/src/istio/mixerclient/referenced.cc @@ -31,6 +31,7 @@ namespace mixerclient { namespace { const char kDelimiter[] = "\0"; const int kDelimiterLength = 1; +const size_t kMaxConcatHashSize = 4096; const std::string kWordDelimiter = ":"; // Decode dereferences index into str using global and local word lists. @@ -63,7 +64,7 @@ bool Decode(int idx, const std::vector &global_words, // Updates hasher with keys void Referenced::UpdateHash(const std::vector &keys, - utils::MD5 *hasher) { + utils::ConcatHash *hasher) { // keys are already sorted during Fill for (const AttributeRef &key : keys) { hasher->Update(key.name); @@ -203,7 +204,7 @@ void Referenced::CalculateSignature(const Attributes &attributes, std::string *signature) const { const auto &attributes_map = attributes.attributes(); - utils::MD5 hasher; + utils::ConcatHash hasher(kMaxConcatHashSize); for (std::size_t i = 0; i < exact_keys_.size(); ++i) { const auto &key = exact_keys_[i]; const auto it = attributes_map.find(key.name); @@ -274,18 +275,18 @@ void Referenced::CalculateSignature(const Attributes &attributes, } hasher.Update(extra_key); - *signature = hasher.Digest(); + *signature = hasher.getHash(); } std::string Referenced::Hash() const { - utils::MD5 hasher; + utils::ConcatHash hasher(kMaxConcatHashSize); // keys are sorted during Fill UpdateHash(absence_keys_, &hasher); hasher.Update(kWordDelimiter); UpdateHash(exact_keys_, &hasher); - return hasher.Digest(); + return hasher.getHash(); } std::string Referenced::DebugString() const { diff --git a/src/istio/mixerclient/referenced.h b/src/istio/mixerclient/referenced.h index ec95680dd7b..14e247d6f87 100644 --- a/src/istio/mixerclient/referenced.h +++ b/src/istio/mixerclient/referenced.h @@ -18,7 +18,7 @@ #include -#include "include/istio/utils/md5.h" +#include "include/istio/utils/concat_hash.h" #include "mixer/v1/check.pb.h" namespace istio { @@ -86,7 +86,7 @@ class Referenced { // Updates hasher with keys static void UpdateHash(const std::vector &keys, - utils::MD5 *hasher); + utils::ConcatHash *hasher); }; } // namespace mixerclient diff --git a/src/istio/mixerclient/referenced_test.cc b/src/istio/mixerclient/referenced_test.cc index 3eb934468a5..63d50218bee 100644 --- a/src/istio/mixerclient/referenced_test.cc +++ b/src/istio/mixerclient/referenced_test.cc @@ -16,7 +16,7 @@ #include "src/istio/mixerclient/referenced.h" #include "include/istio/utils/attributes_builder.h" -#include "include/istio/utils/md5.h" +#include "include/istio/utils/concat_hash.h" #include "google/protobuf/text_format.h" #include "gtest/gtest.h" @@ -177,8 +177,12 @@ TEST(ReferencedTest, FillSuccessTest) { "duration-key, int-key, string-key, string-map-key[If-Match], " "time-key, "); - EXPECT_EQ(utils::MD5::DebugString(referenced.Hash()), - "602d5bbd45b623c3560d2bdb6104f3ab"); + EXPECT_EQ(utils::ConcatHash::DebugString(referenced.Hash()), + "737472696e672d6d61702d6b657900557365722d4167656e74007461726765742e" + "6e616d65007461726765742e73657276696365003a626f6f6c2d6b657900627974" + "65732d6b657900646f75626c652d6b6579006475726174696f6e2d6b657900696e" + "742d6b657900737472696e672d6b657900737472696e672d6d61702d6b65790049" + "662d4d617463680074696d652d6b657900"); } TEST(ReferencedTest, FillFail1Test) { @@ -249,8 +253,14 @@ TEST(ReferencedTest, OKSignature1Test) { std::string signature; EXPECT_TRUE(referenced.Signature(attributes, "extra", &signature)); - EXPECT_EQ(utils::MD5::DebugString(signature), - "751b028b2e2c230ef9c4e59ac556ca04"); + EXPECT_EQ( + utils::ConcatHash::DebugString(signature), + "626f6f6c2d6b657900010062797465732d6b657900746869732069732061206279746573" + "2076616c756500646f75626c652d6b6579009a99999999f95840006475726174696f6e2d" + "6b6579000500000000000000000000000000696e742d6b65790023000000000000000073" + "7472696e672d6b65790074686973206973206120737472696e672076616c756500737472" + "696e672d6d61702d6b65790049662d4d617463680076616c756531000074696d652d6b65" + "790000000000000000000000000000006578747261"); } TEST(ReferencedTest, StringMapReferencedTest) { @@ -271,8 +281,10 @@ TEST(ReferencedTest, StringMapReferencedTest) { std::string signature; EXPECT_TRUE(referenced.Signature(attrs, "extra", &signature)); - EXPECT_EQ(utils::MD5::DebugString(signature), - "bc055468af1a0d4d03ec7f6fa2265b9b"); + EXPECT_EQ(utils::ConcatHash::DebugString(signature), + "6d61702d6b6579310076616c756531006d61702d6b6579320065786163742d7375" + "626b6579340073756276616c7565340065786163742d7375626b65793500737562" + "76616c75653500006578747261"); // negative test: map-key3 must absence ::istio::mixer::v1::Attributes attr1(attrs); diff --git a/src/istio/utils/BUILD b/src/istio/utils/BUILD index 0c815b43d6e..4cc7ff72413 100644 --- a/src/istio/utils/BUILD +++ b/src/istio/utils/BUILD @@ -45,16 +45,6 @@ cc_test( ], ) -cc_library( - name = "md5_lib", - srcs = ["md5.cc"], - visibility = ["//visibility:public"], - deps = [ - "//external:boringssl_crypto", - "//include/istio/utils:headers_lib", - ], -) - cc_test( name = "simple_lru_cache_test", size = "small", @@ -70,21 +60,6 @@ cc_test( ], ) -cc_test( - name = "md5_test", - size = "small", - srcs = ["md5_test.cc"], - linkopts = [ - "-lm", - "-lpthread", - ], - linkstatic = 1, - deps = [ - ":md5_lib", - "//external:googletest_main", - ], -) - cc_library( name = "attribute_names_lib", srcs = [ diff --git a/src/istio/utils/md5.cc b/src/istio/utils/md5.cc deleted file mode 100644 index e1c3b89d341..00000000000 --- a/src/istio/utils/md5.cc +++ /dev/null @@ -1,55 +0,0 @@ -/* Copyright 2017 Istio Authors. All Rights Reserved. - * - * 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 "include/istio/utils/md5.h" -#include - -namespace istio { -namespace utils { - -MD5::MD5() : finalized_(false) { MD5_Init(&ctx_); } - -MD5& MD5::Update(const void* data, size_t size) { - // Not update after finalized. - assert(!finalized_); - MD5_Update(&ctx_, data, size); - return *this; -} - -std::string MD5::Digest() { - if (!finalized_) { - MD5_Final(digest_, &ctx_); - finalized_ = true; - } - return std::string(reinterpret_cast(digest_), kDigestLength); -} - -std::string MD5::DebugString(const std::string& digest) { - assert(digest.size() == kDigestLength); - char buf[kDigestLength * 2 + 1]; - char* p = buf; - for (int i = 0; i < kDigestLength; i++, p += 2) { - sprintf(p, "%02x", (unsigned char)digest[i]); - } - *p = 0; - return std::string(buf, kDigestLength * 2); -} - -std::string MD5::operator()(const void* data, size_t size) { - return Update(data, size).Digest(); -} - -} // namespace utils -} // namespace istio diff --git a/src/istio/utils/md5_test.cc b/src/istio/utils/md5_test.cc deleted file mode 100644 index b8279217167..00000000000 --- a/src/istio/utils/md5_test.cc +++ /dev/null @@ -1,41 +0,0 @@ -/* Copyright 2017 Istio Authors. All Rights Reserved. - * - * 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 "include/istio/utils/md5.h" -#include "gtest/gtest.h" - -namespace istio { -namespace utils { -namespace { - -TEST(MD5Test, TestPriableGigest) { - static const char data[] = "Test Data"; - ASSERT_EQ("0a22b2ac9d829ff3605d81d5ae5e9d16", - MD5::DebugString(MD5()(data, sizeof(data)))); -} - -TEST(MD5Test, TestGigestEqual) { - static const char data1[] = "Test Data1"; - static const char data2[] = "Test Data2"; - auto d1 = MD5()(data1, sizeof(data1)); - auto d11 = MD5()(data1, sizeof(data1)); - auto d2 = MD5()(data2, sizeof(data2)); - ASSERT_EQ(d11, d1); - ASSERT_NE(d1, d2); -} - -} // namespace -} // namespace utils -} // namespace istio From 2eea1051e5a5f4d00cee078de33a9ed5c2001f0f Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Fri, 26 Oct 2018 21:58:35 +0000 Subject: [PATCH 2/3] not to allocate memory from stack Signed-off-by: Wayne Zhang --- include/istio/utils/concat_hash.h | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/include/istio/utils/concat_hash.h b/include/istio/utils/concat_hash.h index 6e9dd125a0a..75ef9b73377 100644 --- a/include/istio/utils/concat_hash.h +++ b/include/istio/utils/concat_hash.h @@ -53,12 +53,14 @@ class ConcatHash { // Converts a binary string to a printable string for unit-test only static std::string DebugString(const std::string& hash) { - char buf[hash.size() * 2]; - char* p = buf; - for (size_t i = 0; i < hash.size(); ++i, p += 2) { - sprintf(p, "%02x", (unsigned char)hash[i]); + std::string out; + out.reserve(hash.size() * 2); + for (size_t i = 0; i < hash.size(); ++i) { + char buf[10]; + sprintf(buf, "%02x", (unsigned char)hash[i]); + out.append(buf); } - return std::string(buf, hash.size() * 2); + return out; } private: From b342dab4c7a04b1014580a3904389eab53bf60ea Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Fri, 26 Oct 2018 22:10:06 +0000 Subject: [PATCH 3/3] Make debug string readable Signed-off-by: Wayne Zhang --- include/istio/utils/concat_hash.h | 12 ++++++---- src/istio/mixerclient/referenced_test.cc | 28 ++++++++++-------------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/include/istio/utils/concat_hash.h b/include/istio/utils/concat_hash.h index 75ef9b73377..bc48e4d6338 100644 --- a/include/istio/utils/concat_hash.h +++ b/include/istio/utils/concat_hash.h @@ -55,10 +55,14 @@ class ConcatHash { static std::string DebugString(const std::string& hash) { std::string out; out.reserve(hash.size() * 2); - for (size_t i = 0; i < hash.size(); ++i) { - char buf[10]; - sprintf(buf, "%02x", (unsigned char)hash[i]); - out.append(buf); + for (auto c : hash) { + if (std::isprint(c)) { + out.append(1, c); + } else { + char buf[10]; + sprintf(buf, "%02x", (unsigned char)c); + out.append(buf); + } } return out; } diff --git a/src/istio/mixerclient/referenced_test.cc b/src/istio/mixerclient/referenced_test.cc index 63d50218bee..5ddc7ccb503 100644 --- a/src/istio/mixerclient/referenced_test.cc +++ b/src/istio/mixerclient/referenced_test.cc @@ -178,11 +178,9 @@ TEST(ReferencedTest, FillSuccessTest) { "time-key, "); EXPECT_EQ(utils::ConcatHash::DebugString(referenced.Hash()), - "737472696e672d6d61702d6b657900557365722d4167656e74007461726765742e" - "6e616d65007461726765742e73657276696365003a626f6f6c2d6b657900627974" - "65732d6b657900646f75626c652d6b6579006475726174696f6e2d6b657900696e" - "742d6b657900737472696e672d6b657900737472696e672d6d61702d6b65790049" - "662d4d617463680074696d652d6b657900"); + "string-map-key00User-Agent00target.name00target.service00:bool-" + "key00bytes-key00double-key00duration-key00int-key00string-" + "key00string-map-key00If-Match00time-key00"); } TEST(ReferencedTest, FillFail1Test) { @@ -253,14 +251,13 @@ TEST(ReferencedTest, OKSignature1Test) { std::string signature; EXPECT_TRUE(referenced.Signature(attributes, "extra", &signature)); - EXPECT_EQ( - utils::ConcatHash::DebugString(signature), - "626f6f6c2d6b657900010062797465732d6b657900746869732069732061206279746573" - "2076616c756500646f75626c652d6b6579009a99999999f95840006475726174696f6e2d" - "6b6579000500000000000000000000000000696e742d6b65790023000000000000000073" - "7472696e672d6b65790074686973206973206120737472696e672076616c756500737472" - "696e672d6d61702d6b65790049662d4d617463680076616c756531000074696d652d6b65" - "790000000000000000000000000000006578747261"); + EXPECT_EQ(utils::ConcatHash::DebugString(signature), + "bool-key000100bytes-key00this is a bytes " + "value00double-key009a99999999f9X@00duration-" + "key000500000000000000000000000000int-key00#0000000000000000string-" + "key00this is a string " + "value00string-map-key00If-Match00value10000time-" + "key000000000000000000000000000000extra"); } TEST(ReferencedTest, StringMapReferencedTest) { @@ -282,9 +279,8 @@ TEST(ReferencedTest, StringMapReferencedTest) { std::string signature; EXPECT_TRUE(referenced.Signature(attrs, "extra", &signature)); EXPECT_EQ(utils::ConcatHash::DebugString(signature), - "6d61702d6b6579310076616c756531006d61702d6b6579320065786163742d7375" - "626b6579340073756276616c7565340065786163742d7375626b65793500737562" - "76616c75653500006578747261"); + "map-key100value100map-key200exact-subkey400subvalue400exact-" + "subkey500subvalue50000extra"); // negative test: map-key3 must absence ::istio::mixer::v1::Attributes attr1(attrs);