Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove use_fast_protobuf_hash runtime flag #36645

Merged
merged 2 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 1 addition & 12 deletions source/common/protobuf/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -214,18 +214,7 @@ void ProtoExceptionUtil::throwProtoValidationException(const std::string& valida

size_t MessageUtil::hash(const Protobuf::Message& message) {
#if defined(ENVOY_ENABLE_FULL_PROTOS)
if (Runtime::runtimeFeatureEnabled("envoy.restart_features.use_fast_protobuf_hash")) {
return DeterministicProtoHash::hash(message);
} else {
std::string text_format;
Protobuf::TextFormat::Printer printer;
printer.SetExpandAny(true);
printer.SetUseFieldNumber(true);
printer.SetSingleLineMode(true);
printer.SetHideUnknownFields(true);
printer.PrintToString(message, &text_format);
return HashUtil::xxHash64(text_format);
}
return DeterministicProtoHash::hash(message);
#else
return HashUtil::xxHash64(message.SerializeAsString());
#endif
Expand Down
1 change: 0 additions & 1 deletion source/common/runtime/runtime_features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ RUNTIME_GUARD(envoy_restart_features_allow_slot_destroy_on_worker_threads);
RUNTIME_GUARD(envoy_restart_features_fix_dispatcher_approximate_now);
RUNTIME_GUARD(envoy_restart_features_quic_handle_certs_with_shared_tls_code);
RUNTIME_GUARD(envoy_restart_features_use_eds_cache_for_ads);
RUNTIME_GUARD(envoy_restart_features_use_fast_protobuf_hash);

// Begin false flags. Most of them should come with a TODO to flip true.

Expand Down
8 changes: 1 addition & 7 deletions source/extensions/filters/http/cache/http_cache.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,7 @@ LookupRequest::LookupRequest(const Http::RequestHeaderMap& request_headers, Syst
// Unless this API is still alpha, calls to stableHashKey() must always return
// the same result, or a way must be provided to deal with a complete cache
// flush.
size_t stableHashKey(const Key& key) {
if (Runtime::runtimeFeatureEnabled("envoy.restart_features.use_fast_protobuf_hash")) {
return DeterministicProtoHash::hash(key);
} else {
return MessageUtil::hash(key);
}
}
size_t stableHashKey(const Key& key) { return DeterministicProtoHash::hash(key); }

void LookupRequest::initializeRequestCacheControl(const Http::RequestHeaderMap& request_headers) {
const absl::string_view cache_control =
Expand Down
17 changes: 0 additions & 17 deletions test/common/protobuf/utility_speed_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
#include "source/common/protobuf/utility.h"

#include "test/common/protobuf/deterministic_hash_test.pb.h"
#include "test/test_common/test_runtime.h"

#include "benchmark/benchmark.h"

Expand Down Expand Up @@ -53,21 +52,8 @@ static std::unique_ptr<Protobuf::Message> testProtoWithRepeatedFields() {
return msg;
}

static void bmHashByTextFormat(benchmark::State& state, std::unique_ptr<Protobuf::Message> msg) {
TestScopedRuntime runtime;
runtime.mergeValues({{"envoy.restart_features.use_fast_protobuf_hash", "false"}});
uint64_t hash = 0;
for (auto _ : state) {
UNREFERENCED_PARAMETER(_);
hash += MessageUtil::hash(*msg);
}
benchmark::DoNotOptimize(hash);
}

static void bmHashByDeterministicHash(benchmark::State& state,
std::unique_ptr<Protobuf::Message> msg) {
TestScopedRuntime runtime;
runtime.mergeValues({{"envoy.restart_features.use_fast_protobuf_hash", "true"}});
uint64_t hash = 0;
for (auto _ : state) {
UNREFERENCED_PARAMETER(_);
Expand All @@ -76,10 +62,7 @@ static void bmHashByDeterministicHash(benchmark::State& state,
benchmark::DoNotOptimize(hash);
}
BENCHMARK_CAPTURE(bmHashByDeterministicHash, map, testProtoWithMaps());
BENCHMARK_CAPTURE(bmHashByTextFormat, map, testProtoWithMaps());
BENCHMARK_CAPTURE(bmHashByDeterministicHash, recursion, testProtoWithRecursion());
BENCHMARK_CAPTURE(bmHashByTextFormat, recursion, testProtoWithRecursion());
BENCHMARK_CAPTURE(bmHashByDeterministicHash, repeatedFields, testProtoWithRepeatedFields());
BENCHMARK_CAPTURE(bmHashByTextFormat, repeatedFields, testProtoWithRepeatedFields());

} // namespace Envoy
26 changes: 10 additions & 16 deletions test/common/protobuf/utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -194,22 +194,16 @@ TEST_F(ProtobufUtilityTest, MessageUtilHash) {
a4.PackFrom(s2);
a5.PackFrom(s3);

TestScopedRuntime runtime_;
for (std::string runtime_value : {"true", "false"}) {
// TODO(ravenblack): when the runtime flag is removed, keep the expects
// but remove the loop around them and the extra output.
runtime_.mergeValues({{"envoy.restart_features.use_fast_protobuf_hash", runtime_value}});
EXPECT_EQ(MessageUtil::hash(a1), MessageUtil::hash(a2)) << runtime_value;
EXPECT_EQ(MessageUtil::hash(a2), MessageUtil::hash(a3)) << runtime_value;
EXPECT_NE(0, MessageUtil::hash(a1)) << runtime_value;
// Same keys and values but with the values in a different order should not have
// the same hash.
EXPECT_NE(MessageUtil::hash(a1), MessageUtil::hash(a4)) << runtime_value;
// Different keys with the values in the same order should not have the same hash.
EXPECT_NE(MessageUtil::hash(a1), MessageUtil::hash(a5)) << runtime_value;
// Struct without 'any' around it should not hash the same as struct inside 'any'.
EXPECT_NE(MessageUtil::hash(s), MessageUtil::hash(a1)) << runtime_value;
}
EXPECT_EQ(MessageUtil::hash(a1), MessageUtil::hash(a2));
EXPECT_EQ(MessageUtil::hash(a2), MessageUtil::hash(a3));
EXPECT_NE(0, MessageUtil::hash(a1));
// Same keys and values but with the values in a different order should not have
// the same hash.
EXPECT_NE(MessageUtil::hash(a1), MessageUtil::hash(a4));
// Different keys with the values in the same order should not have the same hash.
EXPECT_NE(MessageUtil::hash(a1), MessageUtil::hash(a5));
// Struct without 'any' around it should not hash the same as struct inside 'any'.
EXPECT_NE(MessageUtil::hash(s), MessageUtil::hash(a1));
}

TEST_F(ProtobufUtilityTest, RepeatedPtrUtilDebugString) {
Expand Down
12 changes: 0 additions & 12 deletions test/extensions/filters/http/cache/http_cache_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
#include "test/mocks/http/mocks.h"
#include "test/mocks/server/server_factory_context.h"
#include "test/test_common/simulated_time_system.h"
#include "test/test_common/test_runtime.h"
#include "test/test_common/utility.h"

#include "gtest/gtest.h"
Expand Down Expand Up @@ -336,22 +335,11 @@ TEST_F(LookupRequestTest, PragmaNoFallback) {
}

TEST(HttpCacheTest, StableHashKey) {
TestScopedRuntime runtime;
runtime.mergeValues({{"envoy.restart_features.use_fast_protobuf_hash", "true"}});
Key key;
key.set_host("example.com");
ASSERT_EQ(stableHashKey(key), 6153940628716543519u);
}

TEST(HttpCacheTest, StableHashKeyWithSlowHash) {
// TODO(ravenblack): This test should be removed when the runtime guard is removed.
TestScopedRuntime runtime;
runtime.mergeValues({{"envoy.restart_features.use_fast_protobuf_hash", "false"}});
Key key;
key.set_host("example.com");
ASSERT_EQ(stableHashKey(key), 9582653837550152292u);
}

TEST_P(LookupRequestTest, ResultWithBodyAndTrailersMatchesExpectation) {
request_headers_.setReferenceKey(Http::CustomHeaders::get().CacheControl,
GetParam().request_cache_control);
Expand Down