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

Reflection-based deterministic message hashing #30761

Merged
merged 44 commits into from
Dec 13, 2023

Merge branch 'main' into hash

fbed541
Select commit
Loading
Failed to load commit list.
Merged

Reflection-based deterministic message hashing #30761

Merge branch 'main' into hash
fbed541
Select commit
Loading
Failed to load commit list.
CI (Envoy) / Envoy/Publish and verify succeeded Dec 13, 2023 in 8h 35m 1s

Envoy/Publish and verify (success)

Check has finished

Details

Check run finished (success ✔️)

The check run can be viewed here:

Envoy/Publish and verify (pr/30761/main@fbed541)

Check started by

Request (pr/30761/main@fbed541)

ravenblackx @ravenblackx fbed541 #30761 merge main@afdc660

Reflection-based deterministic message hashing

Commit Message: Reflection-based deterministic message hashing
Additional Description: Comparing config used to be done with "deterministic" serialization and a hash of that, but it turned out deterministic serialization was not in fact deterministic enough (Any messages could contain reordered maps or even other fields). The recommendation in protobuf docs is "if you want a deterministic order do it yourself". (Note that the linked comment also suggests that SetSerializationDeterministic should work for our purposes here, but protocolbuffers/protobuf#5731 is the relevant unaddressed bug about Any fields.)

The prior method of achieving determinism by expanding Any via TextFormat serialization is slow enough that it shows up on performance graphs as a significant cost (about 1/4 of our total startup time).
image

This change is quite a lot of code, but should give us a deterministic hash with a much faster runtime than transforming to and then hashing a human-readable string.

Risk Level: Some; if it's broken it might cause config updates to not apply, or cache entries to collide.
Testing: Many unit tests, and a bit extra in the existing hash test. Benchmark results from bazel run -c opt test/common/protobuf:utility_speed_test:

-----------------------------------------------------------------------------------
Benchmark                                         Time             CPU   Iterations
-----------------------------------------------------------------------------------
bmHashByDeterministicHash/map                 35663 ns        35662 ns        20288
bmHashByTextFormat/map                       376608 ns       376503 ns         1892
bmHashByDeterministicHash/recursion           13682 ns        13681 ns        51381
bmHashByTextFormat/recursion                  30239 ns        30239 ns        23463
bmHashByDeterministicHash/repeatedFields      21024 ns        21023 ns        33263
bmHashByTextFormat/repeatedFields            150444 ns       150443 ns         4645

Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
Runtime Guard: envoy.restart_features.use_fast_protobuf_hash

Environment

Request variables

Key Value
ref 279f0dabf74f1aa71574c38e0924961e368ae9ad
sha fbed541
pr 30761
base-sha afdc660
actor ravenblackx @ravenblackx
message Reflection-based deterministic message hashing...
started 1702396314.098278
target-branch main
trusted false
Build image

Container image/s (as used in this CI run)

Key Value
default envoyproxy/envoy-build-ubuntu:7467652575122d8d54e767a68f141598bd855383
mobile envoyproxy/envoy-build-ubuntu:mobile-7467652575122d8d54e767a68f141598bd855383
Version

Envoy version (as used in this CI run)

Key Value
major 1
minor 29
patch 0
dev true