Split the filter popularity counting into a separate module (#26884)

* split hpack encoder filter into a separate class

* chores

* fix

* Automated change: Fix sanity tests

* include guards

* add suppressions

* Update popularity_count_test.cc

Co-authored-by: ctiller <ctiller@users.noreply.github.com>
pull/26851/head^2
Craig Tiller 3 years ago committed by GitHub
parent 3e122fd050
commit 58d3161aac
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 12
      BUILD
  2. 37
      CMakeLists.txt
  3. 2
      Makefile
  4. 13
      build_autogenerated.yaml
  5. 2
      gRPC-C++.podspec
  6. 2
      gRPC-Core.podspec
  7. 1
      grpc.gemspec
  8. 1
      package.xml
  9. 46
      src/core/ext/transport/chttp2/transport/hpack_encoder.cc
  10. 4
      src/core/ext/transport/chttp2/transport/hpack_encoder.h
  11. 62
      src/core/ext/transport/chttp2/transport/popularity_count.h
  12. 13
      test/core/transport/chttp2/BUILD
  13. 73
      test/core/transport/chttp2/popularity_count_test.cc
  14. 1
      tools/doxygen/Doxyfile.c++.internal
  15. 1
      tools/doxygen/Doxyfile.core.internal
  16. 24
      tools/run_tests/generated/tests.json

12
BUILD

@ -2516,6 +2516,17 @@ grpc_cc_library(
],
)
grpc_cc_library(
name = "popularity_count",
hdrs = [
"src/core/ext/transport/chttp2/transport/popularity_count.h",
],
language = "c++",
deps = [
"gpr_platform",
],
)
grpc_cc_library(
name = "grpc_transport_chttp2",
srcs = [
@ -2583,6 +2594,7 @@ grpc_cc_library(
"grpc_http_filters",
"grpc_trace",
"grpc_transport_chttp2_alpn",
"popularity_count",
],
)

37
CMakeLists.txt generated

@ -911,6 +911,7 @@ if(gRPC_BUILD_TESTS)
add_dependencies(buildtests_cxx overload_test)
add_dependencies(buildtests_cxx pid_controller_test)
add_dependencies(buildtests_cxx poll_test)
add_dependencies(buildtests_cxx popularity_count_test)
add_dependencies(buildtests_cxx port_sharing_end2end_test)
add_dependencies(buildtests_cxx proto_server_reflection_test)
add_dependencies(buildtests_cxx proto_utils_test)
@ -2151,6 +2152,7 @@ foreach(_hdr
include/grpc/grpc_posix.h
include/grpc/grpc_security.h
include/grpc/grpc_security_constants.h
include/grpc/impl/codegen/port_platform.h
include/grpc/load_reporting.h
include/grpc/slice.h
include/grpc/slice_buffer.h
@ -2704,6 +2706,7 @@ foreach(_hdr
include/grpc/grpc.h
include/grpc/grpc_posix.h
include/grpc/grpc_security_constants.h
include/grpc/impl/codegen/port_platform.h
include/grpc/load_reporting.h
include/grpc/slice.h
include/grpc/slice_buffer.h
@ -12850,6 +12853,40 @@ target_link_libraries(poll_test
)
endif()
if(gRPC_BUILD_TESTS)
add_executable(popularity_count_test
test/core/transport/chttp2/popularity_count_test.cc
third_party/googletest/googletest/src/gtest-all.cc
third_party/googletest/googlemock/src/gmock-all.cc
)
target_include_directories(popularity_count_test
PRIVATE
${CMAKE_CURRENT_SOURCE_DIR}
${CMAKE_CURRENT_SOURCE_DIR}/include
${_gRPC_ADDRESS_SORTING_INCLUDE_DIR}
${_gRPC_RE2_INCLUDE_DIR}
${_gRPC_SSL_INCLUDE_DIR}
${_gRPC_UPB_GENERATED_DIR}
${_gRPC_UPB_GRPC_GENERATED_DIR}
${_gRPC_UPB_INCLUDE_DIR}
${_gRPC_XXHASH_INCLUDE_DIR}
${_gRPC_ZLIB_INCLUDE_DIR}
third_party/googletest/googletest/include
third_party/googletest/googletest
third_party/googletest/googlemock/include
third_party/googletest/googlemock
${_gRPC_PROTO_GENS_DIR}
)
target_link_libraries(popularity_count_test
${_gRPC_PROTOBUF_LIBRARIES}
${_gRPC_ALLTARGETS_LIBRARIES}
)
endif()
if(gRPC_BUILD_TESTS)

2
Makefile generated

@ -1629,6 +1629,7 @@ PUBLIC_HEADERS_C += \
include/grpc/grpc_posix.h \
include/grpc/grpc_security.h \
include/grpc/grpc_security_constants.h \
include/grpc/impl/codegen/port_platform.h \
include/grpc/load_reporting.h \
include/grpc/slice.h \
include/grpc/slice_buffer.h \
@ -2034,6 +2035,7 @@ PUBLIC_HEADERS_C += \
include/grpc/grpc.h \
include/grpc/grpc_posix.h \
include/grpc/grpc_security_constants.h \
include/grpc/impl/codegen/port_platform.h \
include/grpc/load_reporting.h \
include/grpc/slice.h \
include/grpc/slice_buffer.h \

@ -421,6 +421,7 @@ libs:
- include/grpc/grpc_posix.h
- include/grpc/grpc_security.h
- include/grpc/grpc_security_constants.h
- include/grpc/impl/codegen/port_platform.h
- include/grpc/load_reporting.h
- include/grpc/slice.h
- include/grpc/slice_buffer.h
@ -512,6 +513,7 @@ libs:
- src/core/ext/transport/chttp2/transport/huffsyms.h
- src/core/ext/transport/chttp2/transport/incoming_metadata.h
- src/core/ext/transport/chttp2/transport/internal.h
- src/core/ext/transport/chttp2/transport/popularity_count.h
- src/core/ext/transport/chttp2/transport/stream_map.h
- src/core/ext/transport/chttp2/transport/varint.h
- src/core/ext/transport/inproc/inproc_transport.h
@ -1651,6 +1653,7 @@ libs:
- include/grpc/grpc.h
- include/grpc/grpc_posix.h
- include/grpc/grpc_security_constants.h
- include/grpc/impl/codegen/port_platform.h
- include/grpc/load_reporting.h
- include/grpc/slice.h
- include/grpc/slice_buffer.h
@ -1739,6 +1742,7 @@ libs:
- src/core/ext/transport/chttp2/transport/huffsyms.h
- src/core/ext/transport/chttp2/transport/incoming_metadata.h
- src/core/ext/transport/chttp2/transport/internal.h
- src/core/ext/transport/chttp2/transport/popularity_count.h
- src/core/ext/transport/chttp2/transport/stream_map.h
- src/core/ext/transport/chttp2/transport/varint.h
- src/core/ext/transport/inproc/inproc_transport.h
@ -6008,6 +6012,15 @@ targets:
deps:
- absl/types:variant
uses_polling: false
- name: popularity_count_test
gtest: true
build: test
language: c++
headers:
- src/core/ext/transport/chttp2/transport/popularity_count.h
src:
- test/core/transport/chttp2/popularity_count_test.cc
deps: []
- name: port_sharing_end2end_test
gtest: true
build: test

2
gRPC-C++.podspec generated

@ -288,6 +288,7 @@ Pod::Spec.new do |s|
'src/core/ext/transport/chttp2/transport/huffsyms.h',
'src/core/ext/transport/chttp2/transport/incoming_metadata.h',
'src/core/ext/transport/chttp2/transport/internal.h',
'src/core/ext/transport/chttp2/transport/popularity_count.h',
'src/core/ext/transport/chttp2/transport/stream_map.h',
'src/core/ext/transport/chttp2/transport/varint.h',
'src/core/ext/transport/inproc/inproc_transport.h',
@ -951,6 +952,7 @@ Pod::Spec.new do |s|
'src/core/ext/transport/chttp2/transport/huffsyms.h',
'src/core/ext/transport/chttp2/transport/incoming_metadata.h',
'src/core/ext/transport/chttp2/transport/internal.h',
'src/core/ext/transport/chttp2/transport/popularity_count.h',
'src/core/ext/transport/chttp2/transport/stream_map.h',
'src/core/ext/transport/chttp2/transport/varint.h',
'src/core/ext/transport/inproc/inproc_transport.h',

2
gRPC-Core.podspec generated

@ -386,6 +386,7 @@ Pod::Spec.new do |s|
'src/core/ext/transport/chttp2/transport/incoming_metadata.h',
'src/core/ext/transport/chttp2/transport/internal.h',
'src/core/ext/transport/chttp2/transport/parsing.cc',
'src/core/ext/transport/chttp2/transport/popularity_count.h',
'src/core/ext/transport/chttp2/transport/stream_lists.cc',
'src/core/ext/transport/chttp2/transport/stream_map.cc',
'src/core/ext/transport/chttp2/transport/stream_map.h',
@ -1539,6 +1540,7 @@ Pod::Spec.new do |s|
'src/core/ext/transport/chttp2/transport/huffsyms.h',
'src/core/ext/transport/chttp2/transport/incoming_metadata.h',
'src/core/ext/transport/chttp2/transport/internal.h',
'src/core/ext/transport/chttp2/transport/popularity_count.h',
'src/core/ext/transport/chttp2/transport/stream_map.h',
'src/core/ext/transport/chttp2/transport/varint.h',
'src/core/ext/transport/inproc/inproc_transport.h',

1
grpc.gemspec generated

@ -300,6 +300,7 @@ Gem::Specification.new do |s|
s.files += %w( src/core/ext/transport/chttp2/transport/incoming_metadata.h )
s.files += %w( src/core/ext/transport/chttp2/transport/internal.h )
s.files += %w( src/core/ext/transport/chttp2/transport/parsing.cc )
s.files += %w( src/core/ext/transport/chttp2/transport/popularity_count.h )
s.files += %w( src/core/ext/transport/chttp2/transport/stream_lists.cc )
s.files += %w( src/core/ext/transport/chttp2/transport/stream_map.cc )
s.files += %w( src/core/ext/transport/chttp2/transport/stream_map.h )

1
package.xml generated

@ -280,6 +280,7 @@
<file baseinstalldir="/" name="src/core/ext/transport/chttp2/transport/incoming_metadata.h" role="src" />
<file baseinstalldir="/" name="src/core/ext/transport/chttp2/transport/internal.h" role="src" />
<file baseinstalldir="/" name="src/core/ext/transport/chttp2/transport/parsing.cc" role="src" />
<file baseinstalldir="/" name="src/core/ext/transport/chttp2/transport/popularity_count.h" role="src" />
<file baseinstalldir="/" name="src/core/ext/transport/chttp2/transport/stream_lists.cc" role="src" />
<file baseinstalldir="/" name="src/core/ext/transport/chttp2/transport/stream_map.cc" role="src" />
<file baseinstalldir="/" name="src/core/ext/transport/chttp2/transport/stream_map.h" role="src" />

@ -68,11 +68,7 @@ namespace {
/* don't consider adding anything bigger than this to the hpack table */
constexpr size_t kMaxDecoderSpaceUsage = 512;
constexpr size_t kDataFrameHeaderSize = 9;
constexpr uint8_t kMaxFilterValue = 255;
/* if the probability of this item being seen again is < 1/x then don't add
it to the table */
#define ONE_ON_ADD_PROBABILITY (GRPC_CHTTP2_HPACKC_NUM_VALUES >> 1)
/* The hpack index we encode over the wire. Meaningful to the hpack encoder and
parser on the remote end as well as HTTP2. *Not* the same as
HpackEncoderSlotHash, which is only meaningful to the hpack encoder
@ -223,40 +219,6 @@ static void UpdateAddOrEvict(Hashtable hashtable, const ValueType& value,
#endif
}
/* halve all counts because an element reached max */
static void HalveFilter(uint8_t /*idx*/, uint32_t* sum, uint8_t* elems) {
*sum = 0;
for (int i = 0; i < GRPC_CHTTP2_HPACKC_NUM_VALUES; i++) {
elems[i] /= 2;
(*sum) += elems[i];
}
}
/* increment a filter count, halve all counts if one element reaches max */
static void IncrementFilter(uint8_t idx, uint32_t* sum, uint8_t* elems) {
elems[idx]++;
if (GPR_LIKELY(elems[idx] < kMaxFilterValue)) {
(*sum)++;
} else {
HalveFilter(idx, sum, elems);
}
}
static uint32_t UpdateHashtablePopularity(
grpc_chttp2_hpack_compressor* hpack_compressor, uint32_t elem_hash) {
const uint32_t popularity_hash = HASH_FRAGMENT_1(elem_hash);
IncrementFilter(popularity_hash, &hpack_compressor->filter_elems_sum,
hpack_compressor->filter_elems);
return popularity_hash;
}
static bool CanAddToHashtable(grpc_chttp2_hpack_compressor* hpack_compressor,
uint32_t popularity_hash) {
const bool can_add =
hpack_compressor->filter_elems[popularity_hash] >=
hpack_compressor->filter_elems_sum / ONE_ON_ADD_PROBABILITY;
return can_add;
}
} /* namespace */
struct framer_state {
@ -647,8 +609,9 @@ static EmitIndexedStatus maybe_emit_indexed(grpc_chttp2_hpack_compressor* c,
->hash()
: reinterpret_cast<grpc_core::StaticMetadata*>(GRPC_MDELEM_DATA(elem))
->hash();
/* Update filter to see if we can perhaps add this elem. */
const uint32_t popularity_hash = UpdateHashtablePopularity(c, elem_hash);
// Update filter to see if we can perhaps add this elem.
bool can_add_to_hashtable =
c->filter_elems.AddElement(HASH_FRAGMENT_1(elem_hash));
/* is this elem currently in the decoders table? */
HpackEncoderIndex indices_key;
if (GetMatchingIndex<MetadataComparator>(c->elem_table.entries, elem,
@ -658,8 +621,7 @@ static EmitIndexedStatus maybe_emit_indexed(grpc_chttp2_hpack_compressor* c,
return EmitIndexedStatus(elem_hash, true, false);
}
/* Didn't hit either cuckoo index, so no emit. */
return EmitIndexedStatus(elem_hash, false,
CanAddToHashtable(c, popularity_hash));
return EmitIndexedStatus(elem_hash, false, can_add_to_hashtable);
}
static void emit_maybe_add(grpc_chttp2_hpack_compressor* c, grpc_mdelem elem,

@ -24,6 +24,7 @@
#include <grpc/slice.h>
#include <grpc/slice_buffer.h>
#include "src/core/ext/transport/chttp2/transport/frame.h"
#include "src/core/ext/transport/chttp2/transport/popularity_count.h"
#include "src/core/lib/transport/metadata.h"
#include "src/core/lib/transport/metadata_batch.h"
#include "src/core/lib/transport/transport.h"
@ -59,8 +60,7 @@ struct grpc_chttp2_hpack_compressor {
a new literal should be added to the compression table or not.
They track a single integer that counts how often a particular value has
been seen. When that count reaches max (255), all values are halved. */
uint32_t filter_elems_sum;
uint8_t filter_elems[GRPC_CHTTP2_HPACKC_NUM_VALUES];
grpc_core::PopularityCount<GRPC_CHTTP2_HPACKC_NUM_VALUES> filter_elems;
/* entry tables for keys & elems: these tables track values that have been
seen and *may* be in the decompressor table */

@ -0,0 +1,62 @@
// Copyright 2021 gRPC 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.
#ifndef GRPC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_POPULARITY_COUNT_H
#define GRPC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_POPULARITY_COUNT_H
#include <grpc/impl/codegen/port_platform.h>
#include "include/grpc/impl/codegen/port_platform.h"
namespace grpc_core {
// filter tables for elems: this tables provides an approximate
// popularity count for particular hashes, and are used to determine whether
// a new literal should be added to the compression table or not.
// They track a single integer that counts how often a particular value has
// been seen. When that count reaches max (255), all values are halved. */
template <uint8_t kElems>
class PopularityCount {
public:
PopularityCount() : sum_{0}, elems_{} {}
// increment a filter count, halve all counts if one element reaches max
// return true if this element seems to be popular, false otherwise
bool AddElement(uint8_t idx) {
elems_[idx]++;
if (GPR_LIKELY(elems_[idx] < 255)) {
sum_++;
} else {
HalveFilter();
}
return elems_[idx] >= 2 * sum_ / kElems;
}
private:
// halve all counts because an element reached max
void HalveFilter() {
sum_ = 0;
for (int i = 0; i < kElems; i++) {
elems_[i] /= 2;
sum_ += elems_[i];
}
}
uint32_t sum_;
uint8_t elems_[kElems];
};
} // namespace grpc_core
#endif /* GRPC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_POPULARITY_COUNT_H */

@ -210,3 +210,16 @@ grpc_cc_test(
"//test/core/util:grpc_test_util",
],
)
grpc_cc_test(
name = "popularity_count_test",
srcs = ["popularity_count_test.cc"],
external_deps = [
"gtest",
],
deps = [
"//:gpr_platform",
"//:popularity_count",
"//test/core/util:grpc_suppressions",
],
)

@ -0,0 +1,73 @@
/*
*
* Copyright 2020 gRPC 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 "include/grpc/impl/codegen/port_platform.h"
#include <gtest/gtest.h>
#include <array>
#include "src/core/ext/transport/chttp2/transport/popularity_count.h"
namespace grpc_core {
namespace testing {
static constexpr uint8_t kTestSize = 4;
struct Scenario {
std::array<uint8_t, kTestSize> initial_values;
uint8_t final_add;
bool expectation;
};
std::ostream& operator<<(std::ostream& out, Scenario s) {
out << "init:";
for (size_t i = 0; i < kTestSize; i++) {
if (i != 0) {
out << ",";
}
out << static_cast<int>(s.initial_values[i]);
}
out << " final:" << static_cast<int>(s.final_add);
out << " expect:" << (s.expectation ? "true" : "false");
return out;
}
struct PopularityCountTest : public ::testing::TestWithParam<Scenario> {};
TEST_P(PopularityCountTest, Test) {
Scenario s = GetParam();
PopularityCount<kTestSize> pop;
for (size_t i = 0; i < kTestSize; i++) {
for (size_t j = 0; j < s.initial_values[i]; j++) {
pop.AddElement(i);
}
}
EXPECT_EQ(pop.AddElement(s.final_add), s.expectation);
}
INSTANTIATE_TEST_SUITE_P(InterestingTests, PopularityCountTest,
::testing::Values(Scenario{{0, 0, 0, 0}, 0, true},
Scenario{{64, 0, 0, 0}, 0, true},
Scenario{{64, 0, 0, 0}, 1, false}));
} // namespace testing
} // namespace grpc_core
int main(int argc, char** argv) {
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
}

@ -1237,6 +1237,7 @@ src/core/ext/transport/chttp2/transport/incoming_metadata.cc \
src/core/ext/transport/chttp2/transport/incoming_metadata.h \
src/core/ext/transport/chttp2/transport/internal.h \
src/core/ext/transport/chttp2/transport/parsing.cc \
src/core/ext/transport/chttp2/transport/popularity_count.h \
src/core/ext/transport/chttp2/transport/stream_lists.cc \
src/core/ext/transport/chttp2/transport/stream_map.cc \
src/core/ext/transport/chttp2/transport/stream_map.h \

@ -1072,6 +1072,7 @@ src/core/ext/transport/chttp2/transport/incoming_metadata.cc \
src/core/ext/transport/chttp2/transport/incoming_metadata.h \
src/core/ext/transport/chttp2/transport/internal.h \
src/core/ext/transport/chttp2/transport/parsing.cc \
src/core/ext/transport/chttp2/transport/popularity_count.h \
src/core/ext/transport/chttp2/transport/stream_lists.cc \
src/core/ext/transport/chttp2/transport/stream_map.cc \
src/core/ext/transport/chttp2/transport/stream_map.h \

@ -5561,6 +5561,30 @@
],
"uses_polling": false
},
{
"args": [],
"benchmark": false,
"ci_platforms": [
"linux",
"mac",
"posix",
"windows"
],
"cpu_cost": 1.0,
"exclude_configs": [],
"exclude_iomgrs": [],
"flaky": false,
"gtest": true,
"language": "c++",
"name": "popularity_count_test",
"platforms": [
"linux",
"mac",
"posix",
"windows"
],
"uses_polling": true
},
{
"args": [],
"benchmark": false,

Loading…
Cancel
Save