Removed --use-fake-symbol-table option. (envoyproxy#14178)

Signed-off-by: Manish Kumar <[email protected]>

Author: Manish Kumar

docs/root/operations/admin.rst

@@ -511,9 +511,6 @@ modify different aspects of the server:
   but in response to user requests on high core-count machines, this
   can cause performance issues due to mutex contention.
 
-  This admin endpoint requires Envoy to be started with option
-  `--use-fake-symbol-table 0`.
-
   See :repo:`source/docs/stats.md` for more details.
 
   Note also that actual mutex contention can be tracked via :http:get:`/contention`.

docs/root/version_history/current.rst

@@ -85,3 +85,4 @@ Deprecated
 * gzip: :ref:`HTTP Gzip filter <config_http_filters_gzip>` is rejected now unless explicitly allowed with :ref:`runtime override <config_runtime_deprecation>` `envoy.deprecated_features.allow_deprecated_gzip_http_filter` set to `true`.
 * logging: the `--log-format-prefix-with-location` option is removed.
 * ratelimit: the :ref:`dynamic metadata <envoy_v3_api_field_config.route.v3.RateLimit.Action.dynamic_metadata>` action is deprecated in favor of the more generic :ref:`metadata <envoy_v3_api_field_config.route.v3.RateLimit.Action.metadata>` action.
+* stats: the `--use-fake-symbol-table` option is removed.

include/envoy/server/options.h

@@ -234,11 +234,6 @@ class Options {
    */
   virtual bool mutexTracingEnabled() const PURE;
 
-  /**
-   * @return whether to use the fake symbol table implementation.
-   */
-  virtual bool fakeSymbolTableEnabled() const PURE;
-
   /**
    * @return bool indicating whether cpuset size should determine the number of worker threads.
    */

include/envoy/stats/symbol_table.h

@@ -136,25 +136,6 @@ class SymbolTable {
   virtual void debugPrint() const PURE;
 #endif
 
-  /**
-   * Calls the provided function with a string-view representation of the
-   * elaborated name. This is useful during the interim period when we
-   * are using FakeSymbolTableImpl, to avoid an extra allocation. Once
-   * we migrate to using SymbolTableImpl, this interface will no longer
-   * be helpful and can be removed. The reason it's useful now is that
-   * it makes up, in part, for some extra runtime overhead that is spent
-   * on the SymbolTable abstraction and API, without getting full benefit
-   * from the improved representation.
-   *
-   * TODO(#6307): Remove this when the transition from FakeSymbolTableImpl to
-   * SymbolTableImpl is complete.
-   *
-   * @param stat_name The stat name.
-   * @param fn The function to call with the elaborated stat name as a string_view.
-   */
-  virtual void callWithStringView(StatName stat_name,
-                                  const std::function<void(absl::string_view)>& fn) const PURE;
-
   using RecentLookupsFn = std::function<void(absl::string_view, uint64_t)>;
 
   /**

source/common/stats/symbol_table_impl.cc

@@ -236,11 +236,6 @@ std::string SymbolTableImpl::toString(const StatName& stat_name) const {
   return absl::StrJoin(decodeStrings(stat_name.data(), stat_name.dataSize()), ".");
 }
 
-void SymbolTableImpl::callWithStringView(StatName stat_name,
-                                         const std::function<void(absl::string_view)>& fn) const {
-  fn(toString(stat_name));
-}
-
 void SymbolTableImpl::incRefCount(const StatName& stat_name) {
   // Before taking the lock, decode the array of symbols from the SymbolTable::Storage.
   const SymbolVec symbols = Encoding::decodeSymbols(stat_name.data(), stat_name.dataSize());
@@ -615,7 +610,7 @@ void StatNameList::clear(SymbolTable& symbol_table) {
 }
 
 StatNameSet::StatNameSet(SymbolTable& symbol_table, absl::string_view name)
-    : name_(std::string(name)), symbol_table_(symbol_table), pool_(symbol_table) {
+    : name_(std::string(name)), pool_(symbol_table) {
   builtin_stat_names_[""] = StatName();
 }
 

source/common/stats/symbol_table_impl.h

@@ -186,8 +186,6 @@ class SymbolTableImpl : public SymbolTable {
   void populateList(const StatName* names, uint32_t num_names, StatNameList& list) override;
   StoragePtr encode(absl::string_view name) override;
   StoragePtr makeDynamicStorage(absl::string_view name) override;
-  void callWithStringView(StatName stat_name,
-                          const std::function<void(absl::string_view)>& fn) const override;
 
 #ifndef ENVOY_CONFIG_COVERAGE
   void debugPrint() const override;
@@ -585,7 +583,7 @@ class StatNamePool {
  * SymbolTable lock, but tokens are not shared across StatNames.
  *
  * The SymbolTable is required as a constructor argument to assist in encoding
- * the stat-names, which differs between FakeSymbolTableImpl and SymbolTableImpl.
+ * the stat-names.
  *
  * Example usage:
  *   StatNameDynamicPool pool(symbol_table);
@@ -652,7 +650,6 @@ class StatNameList {
   void clear(SymbolTable& symbol_table);
 
 private:
-  friend class FakeSymbolTableImpl;
   friend class SymbolTableImpl;
 
   /**
@@ -666,10 +663,8 @@ class StatNameList {
    * ...
    *
    *
-   * For FakeSymbolTableImpl, each symbol is a single char, casted into a
-   * uint8_t. For SymbolTableImpl, each symbol is 1 or more bytes, in a
-   * variable-length encoding. See SymbolTableImpl::Encoding::addSymbol for
-   * details.
+   * For SymbolTableImpl, each symbol is 1 or more bytes, in a variable-length
+   * encoding. See SymbolTableImpl::Encoding::addSymbol for details.
    */
   void moveStorageIntoList(SymbolTable::StoragePtr&& storage) { storage_ = std::move(storage); }
 
@@ -841,13 +836,11 @@ class StatNameSet {
   }
 
 private:
-  friend class FakeSymbolTableImpl;
   friend class SymbolTableImpl;
 
   StatNameSet(SymbolTable& symbol_table, absl::string_view name);
 
   const std::string name_;
-  Stats::SymbolTable& symbol_table_;
   Stats::StatNamePool pool_ ABSL_GUARDED_BY(mutex_);
   mutable absl::Mutex mutex_;
   using StringStatNameMap = absl::flat_hash_map<std::string, Stats::StatName>;

source/common/stats/thread_local_store.cc

@@ -349,9 +349,8 @@ class StatNameTagHelper {
       : pool_(tls.symbolTable()), stat_name_tags_(stat_name_tags.value_or(StatNameTagVector())) {
     if (!stat_name_tags) {
       TagVector tags;
-      tls.symbolTable().callWithStringView(name, [&tags, &tls, this](absl::string_view name_str) {
-        tag_extracted_name_ = pool_.add(tls.tagProducer().produceTags(name_str, tags));
-      });
+      tag_extracted_name_ =
+          pool_.add(tls.tagProducer().produceTags(tls.symbolTable().toString(name), tags));
       StatName empty;
       for (const auto& tag : tags) {
         StatName tag_name = tls.wellKnownTags().getBuiltin(tag.name_, empty);
@@ -603,10 +602,7 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatNameWithTags(
     StatNameTagHelper tag_helper(parent_, joiner.tagExtractedName(), stat_name_tags);
 
     ConstSupportedBuckets* buckets = nullptr;
-    symbolTable().callWithStringView(final_stat_name,
-                                     [&buckets, this](absl::string_view stat_name) {
-                                       buckets = &parent_.histogram_settings_->buckets(stat_name);
-                                     });
+    buckets = &parent_.histogram_settings_->buckets(symbolTable().toString(final_stat_name));
 
     RefcountPtr<ParentHistogramImpl> stat;
     {

source/docs/stats.md

@@ -193,19 +193,14 @@ with a format-check, but we can determine whether symbol-table lookups are
 occurring during via an admin endpoint that shows 20 recent lookups by name, at
 `ENVOY_HOST:ADMIN_PORT/stats?recentlookups`.
 
-As of October 6, 2020, the "fake" symbol table implementation has been removed
-from the system, and the "--use-fake-symbol-table" option is now a no-op,
-triggering a warning if set to "1". The option will be removed in a later
-release.
-
 ### Symbol Table Class Overview
 
 Class | Superclass | Description
 -----| ---------- | ---------
 SymbolTable | | Abstract class providing an interface for symbol tables
 SymbolTableImpl | SymbolTable | Implementation of SymbolTable API where StatName share symbols held in a table
 SymbolTableImpl::Encoding | | Helper class for incrementally encoding strings into symbols
-StatName | | Provides an API and a view into a StatName (dynamic orsymbolized). Like absl::string_view, the backing store must be separately maintained.
+StatName | | Provides an API and a view into a StatName (dynamic or symbolized). Like absl::string_view, the backing store must be separately maintained.
 StatNameStorageBase | | Holds storage (an array of bytes) for a dynamic or symbolized StatName
 StatNameStorage  | StatNameStorageBase | Holds storage for a symbolized StatName. Must be explicitly freed (not just destructed).
 StatNameManagedStorage | StatNameStorage | Like StatNameStorage, but is 8 bytes larger, and can be destructed without free(). 

source/server/options_impl.cc

@@ -147,10 +147,6 @@ OptionsImpl::OptionsImpl(std::vector<std::string> args,
   TCLAP::SwitchArg cpuset_threads(
       "", "cpuset-threads", "Get the default # of worker threads from cpuset size", cmd, false);
 
-  TCLAP::ValueArg<bool> use_fake_symbol_table("", "use-fake-symbol-table",
-                                              "Use fake symbol table implementation", false, false,
-                                              "bool", cmd);
-
   TCLAP::ValueArg<std::string> disable_extensions("", "disable-extensions",
                                                   "Comma-separated list of extensions to disable",
                                                   false, "", "string", cmd);
@@ -181,11 +177,6 @@ OptionsImpl::OptionsImpl(std::vector<std::string> args,
 
   hot_restart_disabled_ = disable_hot_restart.getValue();
   mutex_tracing_enabled_ = enable_mutex_tracing.getValue();
-  fake_symbol_table_enabled_ = use_fake_symbol_table.getValue();
-  if (fake_symbol_table_enabled_) {
-    ENVOY_LOG(warn, "Fake symbol tables have been removed. Please remove references to "
-                    "--use-fake-symbol-table");
-  }
 
   cpuset_threads_ = cpuset_threads.getValue();
 
@@ -423,8 +414,8 @@ OptionsImpl::OptionsImpl(const std::string& service_cluster, const std::string&
       service_zone_(service_zone), file_flush_interval_msec_(10000), drain_time_(600),
       parent_shutdown_time_(900), drain_strategy_(Server::DrainStrategy::Gradual),
       mode_(Server::Mode::Serve), hot_restart_disabled_(false), signal_handling_enabled_(true),
-      mutex_tracing_enabled_(false), cpuset_threads_(false), fake_symbol_table_enabled_(false),
-      socket_path_("@envoy_domain_socket"), socket_mode_(0) {}
+      mutex_tracing_enabled_(false), cpuset_threads_(false), socket_path_("@envoy_domain_socket"),
+      socket_mode_(0) {}
 
 void OptionsImpl::disableExtensions(const std::vector<std::string>& names) {
   for (const auto& name : names) {

source/server/options_impl.h

@@ -100,10 +100,6 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable<Logger::I
     ignore_unknown_dynamic_fields_ = ignore_unknown_dynamic_fields;
   }
 
-  void setFakeSymbolTableEnabled(bool fake_symbol_table_enabled) {
-    fake_symbol_table_enabled_ = fake_symbol_table_enabled;
-  }
-
   void setSocketPath(const std::string& socket_path) { socket_path_ = socket_path; }
 
   void setSocketMode(mode_t socket_mode) { socket_mode_ = socket_mode; }
@@ -150,7 +146,6 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable<Logger::I
   bool hotRestartDisabled() const override { return hot_restart_disabled_; }
   bool signalHandlingEnabled() const override { return signal_handling_enabled_; }
   bool mutexTracingEnabled() const override { return mutex_tracing_enabled_; }
-  bool fakeSymbolTableEnabled() const override { return fake_symbol_table_enabled_; }
   Server::CommandLineOptionsPtr toCommandLineOptions() const override;
   void parseComponentLogLevels(const std::string& component_log_levels);
   bool cpusetThreadsEnabled() const override { return cpuset_threads_; }
@@ -205,7 +200,6 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable<Logger::I
   bool signal_handling_enabled_;
   bool mutex_tracing_enabled_;
   bool cpuset_threads_;
-  bool fake_symbol_table_enabled_;
   std::vector<std::string> disabled_extensions_;
   uint32_t count_;
 

test/common/stats/symbol_table_impl_test.cc

@@ -704,8 +704,7 @@ TEST_F(StatNameTest, SupportsAbslHash) {
 
 // Tests the memory savings realized from using symbol tables with 1k
 // clusters. This test shows the memory drops from almost 8M to less than
-// 2M. Note that only SymbolTableImpl is tested for memory consumption,
-// and not FakeSymbolTableImpl.
+// 2M.
 TEST(SymbolTableTest, Memory) {
   // Tests a stat-name allocation strategy.
   auto test_memory_usage = [](std::function<void(absl::string_view)> fn) -> size_t {

test/integration/hotrestart_test.sh

@@ -78,18 +78,18 @@ echo "Hot restart test using dynamic base id"
 
 TEST_INDEX=0
 function run_testsuite() {
-  local BASE_ID BASE_ID_PATH HOT_RESTART_JSON="$1" FAKE_SYMBOL_TABLE="$2"
+  local BASE_ID BASE_ID_PATH HOT_RESTART_JSON="$1"
   local SOCKET_PATH=@envoy_domain_socket
   local SOCKET_MODE=0
-  if [ -n "$3" ] &&  [ -n "$4" ]
+  if [ -n "$2" ] &&  [ -n "$3" ]
   then
-     SOCKET_PATH="$3"
-     SOCKET_MODE="$4"
+     SOCKET_PATH="$2"
+     SOCKET_MODE="$3"
   fi
 
   start_test validation
   check "${ENVOY_BIN}" -c "${HOT_RESTART_JSON}" --mode validate --service-cluster cluster \
-      --use-fake-symbol-table "$FAKE_SYMBOL_TABLE" --service-node node --disable-hot-restart
+      --service-node node --disable-hot-restart
 
   BASE_ID_PATH=$(mktemp 'envoy_test_base_id.XXXXXX')
   echo "Selected dynamic base id path ${BASE_ID_PATH}"
@@ -101,8 +101,7 @@ function run_testsuite() {
   ADMIN_ADDRESS_PATH_0="${TEST_TMPDIR}"/admin.0."${TEST_INDEX}".address
   run_in_background_saving_pid "${ENVOY_BIN}" -c "${HOT_RESTART_JSON}" \
       --restart-epoch 0  --use-dynamic-base-id --base-id-path "${BASE_ID_PATH}" \
-      --service-cluster cluster --service-node node --use-fake-symbol-table "$FAKE_SYMBOL_TABLE" \
-      --admin-address-path "${ADMIN_ADDRESS_PATH_0}" \
+      --service-cluster cluster --service-node node --admin-address-path "${ADMIN_ADDRESS_PATH_0}" \
       --socket-path "${SOCKET_PATH}" --socket-mode "${SOCKET_MODE}"
 
   BASE_ID=$(cat "${BASE_ID_PATH}")
@@ -140,22 +139,13 @@ function run_testsuite() {
   echo "The Envoy's hot restart version is ${CLI_HOT_RESTART_VERSION}"
   echo "Now checking that the above version is what we expected."
   check [ "${CLI_HOT_RESTART_VERSION}" = "${EXPECTED_CLI_HOT_RESTART_VERSION}" ]
-
-  start_test "Checking for consistency of /hot_restart_version with --use-fake-symbol-table ${FAKE_SYMBOL_TABLE}"
-  CLI_HOT_RESTART_VERSION=$("${ENVOY_BIN}" --hot-restart-version --base-id "${BASE_ID}" \
-    --use-fake-symbol-table "$FAKE_SYMBOL_TABLE" 2>&1)
-  CLI_HOT_RESTART_VERSION=$(strip_fake_symbol_table_warning "$CLI_HOT_RESTART_VERSION" "$FAKE_SYMBOL_TABLE")
-  EXPECTED_CLI_HOT_RESTART_VERSION="11.${SHARED_MEMORY_SIZE}"
-  check [ "${CLI_HOT_RESTART_VERSION}" = "${EXPECTED_CLI_HOT_RESTART_VERSION}" ]
-
+ 
   start_test "Checking for match of --hot-restart-version and admin /hot_restart_version"
   ADMIN_ADDRESS_0=$(cat "${ADMIN_ADDRESS_PATH_0}")
   echo "fetching hot restart version from http://${ADMIN_ADDRESS_0}/hot_restart_version ..."
   ADMIN_HOT_RESTART_VERSION=$(curl -sg "http://${ADMIN_ADDRESS_0}/hot_restart_version")
   echo "Fetched ADMIN_HOT_RESTART_VERSION is ${ADMIN_HOT_RESTART_VERSION}"
-  CLI_HOT_RESTART_VERSION=$("${ENVOY_BIN}" --hot-restart-version --base-id "${BASE_ID}" \
-    --use-fake-symbol-table "$FAKE_SYMBOL_TABLE" 2>&1)
-  CLI_HOT_RESTART_VERSION=$(strip_fake_symbol_table_warning "$CLI_HOT_RESTART_VERSION" "$FAKE_SYMBOL_TABLE")
+  CLI_HOT_RESTART_VERSION=$("${ENVOY_BIN}" --hot-restart-version --base-id "${BASE_ID}" 2>&1)
   check [ "${ADMIN_HOT_RESTART_VERSION}" = "${CLI_HOT_RESTART_VERSION}" ]
 
   start_test "Checking server.hot_restart_generation 1"
@@ -175,7 +165,7 @@ function run_testsuite() {
   ADMIN_ADDRESS_PATH_1="${TEST_TMPDIR}"/admin.1."${TEST_INDEX}".address
   run_in_background_saving_pid "${ENVOY_BIN}" -c "${UPDATED_HOT_RESTART_JSON}" \
       --restart-epoch 1 --base-id "${BASE_ID}" --service-cluster cluster --service-node node \
-      --use-fake-symbol-table "$FAKE_SYMBOL_TABLE" --admin-address-path "${ADMIN_ADDRESS_PATH_1}" \
+      --admin-address-path "${ADMIN_ADDRESS_PATH_1}" \
       --socket-path "${SOCKET_PATH}" --socket-mode "${SOCKET_MODE}"
 
   SERVER_1_PID=$BACKGROUND_PID
@@ -216,7 +206,7 @@ function run_testsuite() {
   start_test "Starting epoch 2"
   run_in_background_saving_pid "${ENVOY_BIN}" -c "${UPDATED_HOT_RESTART_JSON}" \
       --restart-epoch 2  --base-id "${BASE_ID}" --service-cluster cluster --service-node node \
-      --use-fake-symbol-table "$FAKE_SYMBOL_TABLE" --admin-address-path "${ADMIN_ADDRESS_PATH_2}" \
+      --admin-address-path "${ADMIN_ADDRESS_PATH_2}" \
       --parent-shutdown-time-s 3 \
       --socket-path "${SOCKET_PATH}" --socket-mode "${SOCKET_MODE}"
 
@@ -267,37 +257,14 @@ function run_testsuite() {
   wait "${SERVER_2_PID}"
 }
 
-# TODO(#13399): remove this helper function and the references to it, as long as
-# the references to $FAKE_SYMBOL_TABLE.
-function strip_fake_symbol_table_warning() {
-  local INPUT="$1"
-  local FAKE_SYMBOL_TABLE="$2"
-  if [ "$FAKE_SYMBOL_TABLE" = "1" ]; then
-    echo "$INPUT" | grep -v "Fake symbol tables have been removed"
-  else
-    echo "$INPUT"
-  fi
-}
-
 # Hotrestart in abstract namespace
 for HOT_RESTART_JSON in "${JSON_TEST_ARRAY[@]}"
 do
-  # Run one of the tests with real symbol tables. No need to do all of them.
-  if [ "$TEST_INDEX" = "0" ]; then
-    run_testsuite "$HOT_RESTART_JSON" "0" || exit 1
-  fi
-
-  run_testsuite "$HOT_RESTART_JSON" "1" || exit 1
+  run_testsuite "$HOT_RESTART_JSON" || exit 1
 done
 
 # Hotrestart in specified UDS
-# Real symbol tables are the default, so I had run just one with fake symbol tables
-# (Switch the "0" and "1" in the second arg in the two run_testsuite calls below).
-if [ "$TEST_INDEX" = "0" ]; then
-  run_testsuite "${HOT_RESTART_JSON_V4}" "0" "${SOCKET_DIR}/envoy_domain_socket" "600" || exit 1
-fi
-
-run_testsuite "${HOT_RESTART_JSON_V4}" "1" "${SOCKET_DIR}/envoy_domain_socket" "600" || exit 1
+run_testsuite "${HOT_RESTART_JSON_V4}" "${SOCKET_DIR}/envoy_domain_socket" "600" || exit 1
 
 start_test "disabling hot_restart by command line."
 CLI_HOT_RESTART_VERSION=$("${ENVOY_BIN}" --hot-restart-version --disable-hot-restart 2>&1)
@@ -308,8 +275,7 @@ start_test socket-mode for socket path
 run_in_background_saving_pid "${ENVOY_BIN}" -c "${HOT_RESTART_JSON}" \
       --restart-epoch 0  --base-id 0 --base-id-path "${BASE_ID_PATH}" \
       --socket-path "${SOCKET_DIR}"/envoy_domain_socket --socket-mode 644 \
-      --service-cluster cluster --service-node node --use-fake-symbol-table "$FAKE_SYMBOL_TABLE" \
-      --admin-address-path "${ADMIN_ADDRESS_PATH_0}"
+      --service-cluster cluster --service-node node --admin-address-path "${ADMIN_ADDRESS_PATH_0}"
 sleep 3
 EXPECTED_SOCKET_MODE=$(stat -c '%a' "${SOCKET_DIR}"/envoy_domain_socket_parent_0)
 check [ "644" = "${EXPECTED_SOCKET_MODE}" ]

test/mocks/server/options.h

@@ -47,7 +47,6 @@ class MockOptions : public Options {
   MOCK_METHOD(bool, hotRestartDisabled, (), (const));
   MOCK_METHOD(bool, signalHandlingEnabled, (), (const));
   MOCK_METHOD(bool, mutexTracingEnabled, (), (const));
-  MOCK_METHOD(bool, fakeSymbolTableEnabled, (), (const));
   MOCK_METHOD(bool, cpusetThreadsEnabled, (), (const));
   MOCK_METHOD(const std::vector<std::string>&, disabledExtensions, (), (const));
   MOCK_METHOD(Server::CommandLineOptionsPtr, toCommandLineOptions, (), (const));