support: add environment controllable feature flags

RFC via PR! This is a mechanism that we have explicitly built into some
of the work for the next release, so I decided to come with a fully
concrete proposal.

Here we provide the macro "KATANA_EXPERIMENTAL_FEATURE(FeatureName)"
where FeatureName is some feature-specific string. Other code can then
then check if the flag using the macro
KATANA_EXPERIMENTAL_ENABLED(FeatureName).

Those wishing to use the experimental feature simply put "FeatureName"
into a comma delimited list provided via the environment variable
"KATANA_ENABLE_EXPERIMENTAL". If "FeatureName" is part of that string
the above function will return true.

The mechanism is very simple on purpose; rather than allow flags that
are "True" by default I thought it was a good idea to encourage
developers to eliminate feature flags once the features they guard are
stable or turn feature flags into actual user facing configuration
options if the ability to toggle a flag is actually useful.

The main thing I like about this design:
 * flags can be defined close to to features they guard
 * related to the above: adding/changing a flag does not require a
   cross-repo commit
 * flags can be as private or as public as they need to be

This comes with the drawback that technically flags can conflict,
meaning a developer could accidentally enable some far flung feature
when they only meant to test their own. I don't think that's a huge
concern as the macro is easy to grep for.

Signed-off-by: Tyler Hunt <[email protected]>

libgraph/src/SharedMemSys.cpp

@@ -20,10 +20,12 @@
 #include "katana/SharedMemSys.h"
 
 #include "katana/CommBackend.h"
+#include "katana/Experimental.h"
 #include "katana/Galois.h"
 #include "katana/GaloisRuntime.h"
 #include "katana/Logging.h"
 #include "katana/Plugin.h"
+#include "katana/Strings.h"
 #include "katana/TextTracer.h"
 #include "tsuba/FileStorage.h"
 #include "tsuba/tsuba.h"
@@ -44,6 +46,23 @@ katana::SharedMemSys::SharedMemSys(std::unique_ptr<ProgressTracer> tracer)
   if (auto init_good = tsuba::Init(&comm_backend); !init_good) {
     KATANA_LOG_FATAL("tsuba::Init: {}", init_good.error());
   }
+
+  auto features_on = katana::internal::ExperimentalFeature::ReportEnabled();
+  if (!features_on.empty()) {
+    auto feature_string = katana::Join(features_on, ",");
+    tracer->GetActiveSpan().SetTags(
+        {{"experimental_features_enabled", feature_string}});
+  }
+
+  auto unrecognized =
+      katana::internal::ExperimentalFeature::ReportUnrecognized();
+  if (!unrecognized.empty()) {
+    KATANA_LOG_WARN(
+        "these values from KATANA_ENABLE_EXPERIMENTAL did not match any "
+        "features:\n\t{}",
+        katana::Join(unrecognized, " "));
+  }
+
   katana::ProgressTracer::Set(std::move(tracer));
 }
 

libsupport/CMakeLists.txt

@@ -19,16 +19,17 @@ set(sources
         src/EntityTypeManager.cpp
         src/Env.cpp
         src/ErrorCode.cpp
-        src/HostAllocator.cpp
+        src/Experimental.cpp
         src/HTTP.cpp
+        src/HostAllocator.cpp
         src/JSON.cpp
         src/JSONTracer.cpp
         src/Logging.cpp
         src/NoopTracer.cpp
-        src/Random.cpp
-        src/Result.cpp
         src/Plugin.cpp
         src/ProgressTracer.cpp
+        src/Random.cpp
+        src/Result.cpp
         src/Signals.cpp
         src/Strings.cpp
         src/TextTracer.cpp

libsupport/include/katana/Experimental.h

@@ -0,0 +1,122 @@
+#ifndef KATANA_LIBSUPPORT_KATANA_EXPERIMENTAL_H_
+#define KATANA_LIBSUPPORT_KATANA_EXPERIMENTAL_H_
+
+#include <memory>
+#include <mutex>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "katana/config.h"
+
+namespace katana {
+namespace internal {
+
+/// ExperimentalFeature tracks the state of feature flags set in the environment;
+/// It is not intended to be used directly; please see the macro KATANA_EXPERIMENTAL_FEATURE
+/// below
+class KATANA_EXPORT ExperimentalFeature {
+public:
+  static ExperimentalFeature* Register(
+      const std::string& feature_name, const std::string& filename,
+      int line_number);
+
+  /// report the feature flags that were checked on codepaths that were executed and
+  /// the flag was set to true
+  static std::vector<std::string> ReportEnabled();
+
+  /// report the feature flags that were used but stayed false
+  static std::vector<std::string> ReportDisabled();
+
+  /// report the feature flags that were provided but did not match any registered flag
+  static std::vector<std::string> ReportUnrecognized();
+
+  bool IsEnabled() { return is_enabled_; }
+
+  ExperimentalFeature(const ExperimentalFeature& no_copy) = delete;
+  ExperimentalFeature& operator=(const ExperimentalFeature& no_copy) = delete;
+  ExperimentalFeature(ExperimentalFeature&& no_move) = delete;
+  ExperimentalFeature& operator=(ExperimentalFeature&& no_move) = delete;
+  ~ExperimentalFeature() = default;
+
+  const std::string& name() const { return name_; }
+  const std::string& filename() const { return filename_; }
+  int line_number() const { return line_number_; }
+
+private:
+  ExperimentalFeature(std::string name, std::string filename, int line_number)
+      : name_(std::move(name)),
+        filename_(std::move(filename)),
+        line_number_(line_number) {
+    CheckEnv();
+  }
+
+  void CheckEnv();
+
+  std::string name_;
+  std::string filename_;
+  int line_number_{};
+  bool is_enabled_{};
+
+  static std::mutex registered_features_mutex_;
+  static std::unordered_map<std::string, std::unique_ptr<ExperimentalFeature>>
+      registered_features_;
+};
+
+}  // namespace internal
+}  // namespace katana
+
+/// KATANA_EXPERIMENTAL_FEATURE creates a flag that can be set from the environment.
+/// The macro takes a feature_name which should be an unquoted, unique string that
+/// looks like a function name. Developers can then use the macro
+/// KATANA_EXPERIMENTAL_ENABLED using the same string to detect if the flag was set.
+///
+/// Flags are set using the environment variable KATANA_ENABLE_EXPERIMENTAL. Users
+/// pass the same string passed to KATANA_EXPERIMENTAL_FEATURE to set a particular
+/// flag. Multiple flags may be set by providing a comma delimited list of feature
+/// names.
+///
+/// NB: an implication of the above is that these flags are only useful for runtime
+/// configuration. If the desire is to control compile-time changes, a different
+/// mechanism is required.
+///
+/// Example:
+///
+/// Program env:
+///   KATANA_ENABLE_EXPERIMENTAL="UnstableButUseful"
+///
+/// active_development.cpp:
+///    KATANA_EXPERIMENTAL_FEATURE(UnstableButUseful);
+///
+///    void important_function() {
+///      if (KATANA_EXPERIMENTAL_ENABLED(UnstableButUseful)) {
+///        // do something useful
+///      } else {
+///        // be conservative
+///      }
+///    }
+///
+/// Flags declared in different parts of the codebase can conflict. A good
+/// practice is to choose good name for your feature and grep for this macro
+/// to be sure it does not collide with another.
+///
+/// Another best practice is to heavily comment where the macro is defined,
+/// detailing what the feature does and the state of tests to avoid regressions.
+#define KATANA_EXPERIMENTAL_FEATURE(feature_name)                              \
+  namespace katana::internal {                                                 \
+  class ExperimentalFeature;                                                   \
+  static auto* katana_experimental_feature_ptr_##feature_name =                \
+      ::katana::internal::ExperimentalFeature::Register(                       \
+          #feature_name, __FILE__, __LINE__);                                  \
+  }                                                                            \
+  static_assert(                                                               \
+      std::is_same<                                                            \
+          ::katana::internal::ExperimentalFeature,                             \
+          katana::internal::ExperimentalFeature>::value,                       \
+      "KATANA_EXPERIMENTAL_FEATURE must not be inside a namespace block")
+
+#define KATANA_EXPERIMENTAL_ENABLED(feature_name)                              \
+  (::katana::internal::katana_experimental_feature_ptr_##feature_name          \
+       ->IsEnabled())
+
+#endif

libsupport/src/Experimental.cpp

@@ -0,0 +1,132 @@
+#include "katana/Experimental.h"
+
+#include <iomanip>
+#include <memory>
+
+#include "katana/Env.h"
+#include "katana/Logging.h"
+#include "katana/Strings.h"
+
+namespace {
+
+class ExperimentalFeatureEnvState {
+public:
+  static ExperimentalFeatureEnvState* Get() {
+    std::call_once(init_flag_, [&]() {
+      state_ = std::unique_ptr<ExperimentalFeatureEnvState>(
+          new ExperimentalFeatureEnvState());
+    });
+    return state_.get();
+  }
+
+  bool WasInEnv(const std::string& feature) {
+    auto it = features_used_.find(feature);
+    if (it == features_used_.end()) {
+      return false;
+    }
+    it->second = true;
+    return true;
+  }
+
+  const std::unordered_map<std::string, bool>& features_used() {
+    return features_used_;
+  }
+
+private:
+  ExperimentalFeatureEnvState() {
+    std::string val;
+    if (!katana::GetEnv("KATANA_ENABLE_EXPERIMENTAL", &val)) {
+      return;
+    }
+
+    auto strings = katana::SplitView(val, ",");
+    for (const auto& feature : strings) {
+      features_used_.emplace(feature, false);
+    }
+  }
+
+  std::unordered_map<std::string, bool> features_used_;
+
+  static std::once_flag init_flag_;
+  static std::unique_ptr<ExperimentalFeatureEnvState> state_;
+};
+
+std::once_flag ExperimentalFeatureEnvState::init_flag_;
+
+std::unique_ptr<ExperimentalFeatureEnvState>
+    ExperimentalFeatureEnvState::state_;
+
+}  // namespace
+
+katana::internal::ExperimentalFeature*
+katana::internal::ExperimentalFeature::Register(
+    const std::string& feature_name, const std::string& filename,
+    int line_number) {
+  std::lock_guard<std::mutex> lock(registered_features_mutex_);
+
+  auto [it, was_created] = registered_features_.emplace(
+      feature_name,
+      std::unique_ptr<ExperimentalFeature>(
+          new ExperimentalFeature(feature_name, filename, line_number)));
+  const auto& flag = it->second;
+  if (!was_created &&
+      (flag->filename() != filename || flag->line_number() != line_number)) {
+    KATANA_LOG_WARN(
+        "{} declared in multiple places:\n\there: {}:{}\n\tand here: {}:{}",
+        feature_name, flag->filename(), flag->line_number(), filename,
+        line_number);
+  }
+  return it->second.get();
+}
+
+void
+katana::internal::ExperimentalFeature::CheckEnv() {
+  is_enabled_ = ExperimentalFeatureEnvState::Get()->WasInEnv(name_);
+}
+
+std::vector<std::string>
+katana::internal::ExperimentalFeature::ReportEnabled() {
+  std::lock_guard<std::mutex> lock(registered_features_mutex_);
+
+  std::vector<std::string> enabled;
+
+  for (const auto& [name, ptr] : registered_features_) {
+    if (ptr->IsEnabled()) {
+      enabled.emplace_back(name);
+    }
+  }
+  return enabled;
+}
+
+std::vector<std::string>
+katana::internal::ExperimentalFeature::ReportDisabled() {
+  std::lock_guard<std::mutex> lock(registered_features_mutex_);
+
+  std::vector<std::string> disabled;
+
+  for (const auto& [name, ptr] : registered_features_) {
+    if (!ptr->IsEnabled()) {
+      disabled.emplace_back(name);
+    }
+  }
+  return disabled;
+}
+
+std::vector<std::string>
+katana::internal::ExperimentalFeature::ReportUnrecognized() {
+  std::vector<std::string> unused;
+
+  for (const auto& [name, was_used] :
+       ExperimentalFeatureEnvState::Get()->features_used()) {
+    if (!was_used) {
+      unused.emplace_back(name);
+    }
+  }
+  return unused;
+}
+
+std::mutex katana::internal::ExperimentalFeature::registered_features_mutex_;
+
+std::unordered_map<
+    std::string, std::unique_ptr<katana::internal::ExperimentalFeature>>
+    katana::internal::ExperimentalFeature::registered_features_;

libsupport/test/CMakeLists.txt

@@ -22,6 +22,7 @@ add_unit_test(bitmath)
 add_unit_test(cache)
 add_unit_test(disjoint_range_iterator)
 add_unit_test(env)
+add_unit_test(experimental)
 add_unit_test(logging)
 add_unit_test(opaque-id)
 add_unit_test(random)
@@ -33,6 +34,9 @@ add_unit_test(type-manager)
 add_unit_test(uri)
 add_unit_test(zip_iterator)
 
+set_tests_properties(experimental-test PROPERTIES
+  ENVIRONMENT KATANA_ENABLE_EXPERIMENTAL=TestOn,TestSecond,DefinedButUnused,EnvironmentOnly)
+
 add_executable(arrow-bench arrow-bench.cpp)
 target_link_libraries(arrow-bench katana_support benchmark::benchmark)
 add_test(NAME arrow-bench COMMAND arrow-bench --benchmark_filter=/1024)

libsupport/test/experimental.cpp

@@ -0,0 +1,44 @@
+#include "katana/Experimental.h"
+
+#include <algorithm>
+
+#include "katana/Logging.h"
+
+KATANA_EXPERIMENTAL_FEATURE(TestOn);
+KATANA_EXPERIMENTAL_FEATURE(TestOff);
+KATANA_EXPERIMENTAL_FEATURE(TestSecond);
+
+KATANA_EXPERIMENTAL_FEATURE(DefinedButUnused);
+
+// this causes the compiler to print:
+//   error: static assertion failed: KATANA_EXPERIMENTAL_FEATURE must not be
+//      inside a namespace block
+//
+// namespace test {
+//
+// KATANA_EXPERIMENTAL_FEATURE(ShouldNotCompile);
+//
+// } // namespace test
+
+int
+main() {
+  KATANA_LOG_ASSERT(KATANA_EXPERIMENTAL_ENABLED(TestOn));
+  KATANA_LOG_ASSERT(!KATANA_EXPERIMENTAL_ENABLED(TestOff));
+  KATANA_LOG_ASSERT(KATANA_EXPERIMENTAL_ENABLED(TestSecond));
+
+  auto unused_in_env =
+      katana::internal::ExperimentalFeature::ReportUnrecognized();
+  KATANA_LOG_ASSERT(unused_in_env.size() == 1);
+  KATANA_LOG_ASSERT(unused_in_env[0] == "EnvironmentOnly");
+
+  auto enabled = katana::internal::ExperimentalFeature::ReportEnabled();
+  KATANA_LOG_ASSERT(enabled.size() == 3);
+  std::sort(enabled.begin(), enabled.end());
+  KATANA_LOG_ASSERT(enabled[0] == "DefinedButUnused");
+  KATANA_LOG_ASSERT(enabled[1] == "TestOn");
+  KATANA_LOG_ASSERT(enabled[2] == "TestSecond");
+
+  auto disabled = katana::internal::ExperimentalFeature::ReportDisabled();
+  KATANA_LOG_ASSERT(disabled.size() == 1);
+  KATANA_LOG_ASSERT(disabled[0] == "TestOff");
+}