collapse TreeBuilderV2 features

Summary: Currently there are two features between CodeGen v2 (TypeGraph) and TreeBuilder v2. These are TypedDataSegment and TreeBuilderTypeChecking. Each of these features currently has a full set of tests run in the CI and each have specific exclusions. Collapse these features into TreeBuilder v2. This allows for significantly simplified testing as any OIL tests run under TreeBuilder v2 and any OID tests run under TreeBuilder v1. The reasoning behind this is I no longer intend to partially roll out this feature. Full TreeBuilder v2 applies different conditions to containers than the intermediate states, and writing these only to have them never deployed is a waste of time. Test Plan: - it builds - CI
facebookexperimental · Nov 13, 2023 · 3871d92 · 3871d92
1 parent 2542612
commit 3871d92
Show file tree

Hide file tree

Showing 13 changed files with 39 additions and 238 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -21,20 +21,6 @@ workflows:
           oid_test_args: "-ftype-graph"
           tests_regex: "OidIntegration\\..*"
           exclude_regex: ".*inheritance_polymorphic.*|.*arrays_member_int0"
-      - test:
-          name: test-typed-data-segment-gcc
-          requires:
-            - build-gcc
-          oid_test_args: "-ftyped-data-segment"
-          tests_regex: "OidIntegration\\..*"
-          exclude_regex: ".*inheritance_polymorphic.*|.*pointers.*|.*arrays_member_int0|.*cycles_.*"
-      - test:
-          name: test-tree-builder-type-checking-gcc
-          requires:
-            - build-gcc
-          oid_test_args: "-ftree-builder-type-checking"
-          tests_regex: "OidIntegration\\..*"
-          exclude_regex: ".*inheritance_polymorphic.*|.*pointers.*|.*arrays_member_int0|.*cycles_.*"
       - coverage:
           name: coverage
           requires:
@@ -43,14 +29,6 @@ workflows:
           name: coverage-type-graph
           requires:
             - test-type-graph-gcc
-      - coverage:
-          name: coverage-typed-data-segment
-          requires:
-            - test-typed-data-segment-gcc
-      - coverage:
-          name: coverage-tree-builder-type-checking
-          requires:
-            - test-tree-builder-type-checking-gcc
 
       - build:
           name: build-clang

diff --git a/oi/CodeGen.cpp b/oi/CodeGen.cpp
@@ -119,17 +119,14 @@ void addIncludes(const TypeGraph& typeGraph,
                  FeatureSet features,
                  std::string& code) {
   std::set<std::string_view> includes{"cstddef"};
-  if (features[Feature::TypedDataSegment]) {
+  if (features[Feature::TreeBuilderV2]) {
+    code += "#define DEFINE_DESCRIBE 1\n";  // added before all includes
+
     includes.emplace("functional");
-    includes.emplace("oi/types/st.h");
-  }
-  if (features[Feature::TreeBuilderTypeChecking]) {
+    includes.emplace("oi/exporters/inst.h");
     includes.emplace("oi/types/dy.h");
-
-    code += "#define DEFINE_DESCRIBE 1\n";  // added before all includes
+    includes.emplace("oi/types/st.h");
   }
-  if (features[Feature::TreeBuilderV2])
-    includes.emplace("oi/exporters/inst.h");
   if (features[Feature::Library]) {
     includes.emplace("vector");
     includes.emplace("oi/IntrospectionResult.h");
@@ -832,35 +829,20 @@ void CodeGen::genClassTypeHandler(const Class& c, std::string& code) {
   code += "  using type = ";
   genClassStaticType(c, code);
   code += ";\n";
-  if (config_.features[Feature::TreeBuilderV2])
-    genClassTreeBuilderInstructions(c, code);
+  genClassTreeBuilderInstructions(c, code);
   genClassTraversalFunction(c, code);
   code += "};\n";
 }
 
 namespace {
 
-void genContainerTypeHandler(FeatureSet features,
-                             std::unordered_set<const ContainerInfo*>& used,
+void genContainerTypeHandler(std::unordered_set<const ContainerInfo*>& used,
                              const ContainerInfo& c,
                              std::span<const TemplateParam> templateParams,
                              std::string& code) {
   if (!used.insert(&c).second)
     return;
 
-  if (!features[Feature::TreeBuilderV2]) {
-    const auto& handler = c.codegen.handler;
-    if (handler.empty()) {
-      LOG(ERROR) << "`codegen.handler` must be specified for all containers "
-                    "under \"-ftyped-data-segment\", not specified for \"" +
-                        c.typeName + "\"";
-      throw std::runtime_error("missing `codegen.handler`");
-    }
-    auto fmt = boost::format(c.codegen.handler) % c.typeName;
-    code += fmt.str();
-    return;
-  }
-
   code += c.codegen.extra;
 
   // TODO: Move this check into the ContainerInfo parsing once always enabled.
@@ -1088,7 +1070,7 @@ constexpr inst::Field make_field(std::string_view name) {
                     "0"},
   };
   genContainerTypeHandler(
-      features, used, FuncGen::GetOiArrayContainerInfo(), arrayParams, code);
+      used, FuncGen::GetOiArrayContainerInfo(), arrayParams, code);
 }
 
 }  // namespace
@@ -1098,14 +1080,10 @@ void CodeGen::addTypeHandlers(const TypeGraph& typeGraph, std::string& code) {
     if (const auto* c = dynamic_cast<const Class*>(&t)) {
       genClassTypeHandler(*c, code);
     } else if (const auto* con = dynamic_cast<const Container*>(&t)) {
-      genContainerTypeHandler(config_.features,
-                              definedContainers_,
-                              con->containerInfo_,
-                              con->templateParams,
-                              code);
+      genContainerTypeHandler(
+          definedContainers_, con->containerInfo_, con->templateParams, code);
     } else if (const auto* cap = dynamic_cast<const CaptureKeys*>(&t)) {
-      genContainerTypeHandler(config_.features,
-                              definedContainers_,
+      genContainerTypeHandler(definedContainers_,
                               cap->containerInfo(),
                               cap->container().templateParams,
                               code);
@@ -1227,25 +1205,23 @@ void CodeGen::generate(
   if (!config_.features[Feature::Library]) {
     FuncGen::DeclareExterns(code);
   }
-  if (!config_.features[Feature::TypedDataSegment]) {
+  if (!config_.features[Feature::TreeBuilderV2]) {
     defineMacros(code);
   }
   addIncludes(typeGraph, config_.features, code);
   defineInternalTypes(code);
   FuncGen::DefineJitLog(code, config_.features);
 
-  if (config_.features[Feature::TypedDataSegment]) {
+  if (config_.features[Feature::TreeBuilderV2]) {
     if (config_.features[Feature::Library]) {
       FuncGen::DefineBackInserterDataBuffer(code);
     } else {
       FuncGen::DefineDataSegmentDataBuffer(code);
     }
     code += "using namespace oi;\n";
     code += "using namespace oi::detail;\n";
-    if (config_.features[Feature::TreeBuilderV2]) {
-      code += "using oi::exporters::ParsedData;\n";
-      code += "using namespace oi::exporters;\n";
-    }
+    code += "using oi::exporters::ParsedData;\n";
+    code += "using namespace oi::exporters;\n";
     code += "namespace OIInternal {\nnamespace {\n";
     FuncGen::DefineBasicTypeHandlers(code, config_.features);
     code += "} // namespace\n} // namespace OIInternal\n";
@@ -1265,7 +1241,7 @@ void CodeGen::generate(
    * process faster.
    */
   code += "namespace OIInternal {\nnamespace {\n";
-  if (!config_.features[Feature::TypedDataSegment]) {
+  if (!config_.features[Feature::TreeBuilderV2]) {
     FuncGen::DefineEncodeData(code);
     FuncGen::DefineEncodeDataSize(code);
     FuncGen::DefineStoreData(code);
@@ -1280,7 +1256,7 @@ void CodeGen::generate(
     genExclusiveSizes(typeGraph, code);
   }
 
-  if (config_.features[Feature::TypedDataSegment]) {
+  if (config_.features[Feature::TreeBuilderV2]) {
     addStandardTypeHandlers(typeGraph, config_.features, code);
     addTypeHandlers(typeGraph, code);
   } else {
@@ -1297,10 +1273,8 @@ void CodeGen::generate(
   code += "} // namespace\n} // namespace OIInternal\n";
 
   const auto typeName = SymbolService::getTypeName(drgnType);
-  if (config_.features[Feature::Library]) {
+  if (config_.features[Feature::TreeBuilderV2]) {
     FuncGen::DefineTopLevelIntrospect(code, typeName);
-  } else if (config_.features[Feature::TypedDataSegment]) {
-    FuncGen::DefineTopLevelGetSizeRefTyped(code, typeName, config_.features);
   } else {
     FuncGen::DefineTopLevelGetSizeRef(code, typeName, config_.features);
   }
@@ -1310,8 +1284,6 @@ void CodeGen::generate(
                                            typeName,
                                            calculateExclusiveSize(rootType),
                                            enumerateTypeNames(rootType));
-  } else if (config_.features[Feature::TreeBuilderTypeChecking]) {
-    FuncGen::DefineOutputType(code, typeName);
   }
 
   if (!linkageName_.empty())

diff --git a/oi/ContainerInfo.cpp b/oi/ContainerInfo.cpp
@@ -274,10 +274,6 @@ ContainerInfo::ContainerInfo(const fs::path& path) {
   } else {
     throw ContainerInfoError(path, "`codegen.decl` is a required field");
   }
-  if (std::optional<std::string> str =
-          codegenToml["handler"].value<std::string>()) {
-    codegen.handler = std::move(*str);
-  }
   if (std::optional<std::string> str =
           codegenToml["traversal_func"].value<std::string>()) {
     codegen.traversalFunc = std::move(*str);
@@ -323,8 +319,6 @@ ContainerInfo::ContainerInfo(std::string typeName_,
       matcher(getMatcher(typeName)),
       ctype(ctype_),
       header(std::move(header_)),
-      codegen(Codegen{"// DummyDecl %1%\n",
-                      "// DummyFunc %1%\n",
-                      "// DummyHandler %1%\n",
-                      "// DummyFunc\n"}) {
+      codegen(Codegen{
+          "// DummyDecl %1%\n", "// DummyFunc %1%\n", "// DummyFunc\n"}) {
 }
diff --git a/oi/ContainerInfo.h b/oi/ContainerInfo.h
@@ -36,7 +36,6 @@ struct ContainerInfo {
   struct Codegen {
     std::string decl;
     std::string func;
-    std::string handler = "";
     std::string traversalFunc = "";
     std::string extra = "";
     std::vector<Processor> processors{};

diff --git a/oi/Features.cpp b/oi/Features.cpp
@@ -40,11 +40,6 @@ std::optional<std::string_view> featureHelp(Feature f) {
       return "Use Type Graph for code generation (CodeGen v2).";
     case Feature::PruneTypeGraph:
       return "Prune unreachable nodes from the type graph";
-    case Feature::TypedDataSegment:
-      return "Use Typed Data Segment in generated code.";
-    case Feature::TreeBuilderTypeChecking:
-      return "Use Typed Data Segment to perform runtime Type Checking in "
-             "TreeBuilder.";
     case Feature::Library:
       return std::nullopt;  // Hide in OID help
     case Feature::TreeBuilderV2:
@@ -65,14 +60,8 @@ std::optional<std::string_view> featureHelp(Feature f) {
 
 std::span<const Feature> requirements(Feature f) {
   switch (f) {
-    case Feature::TypedDataSegment:
-      static constexpr std::array tds = {Feature::TypeGraph};
-      return tds;
-    case Feature::TreeBuilderTypeChecking:
-      static constexpr std::array tc = {Feature::TypedDataSegment};
-      return tc;
     case Feature::TreeBuilderV2:
-      static constexpr std::array tb2 = {Feature::TreeBuilderTypeChecking};
+      static constexpr std::array tb2 = {Feature::TypeGraph};
       return tb2;
     case Feature::Library:
       static constexpr std::array lib = {Feature::TreeBuilderV2};

diff --git a/oi/Features.h b/oi/Features.h
@@ -22,20 +22,18 @@
 
 #include "oi/EnumBitset.h"
 
-#define OI_FEATURE_LIST                                    \
-  X(ChaseRawPointers, "chase-raw-pointers")                \
-  X(PackStructs, "pack-structs")                           \
-  X(GenPaddingStats, "gen-padding-stats")                  \
-  X(CaptureThriftIsset, "capture-thrift-isset")            \
-  X(TypeGraph, "type-graph")                               \
-  X(PruneTypeGraph, "prune-type-graph")                    \
-  X(TypedDataSegment, "typed-data-segment")                \
-  X(TreeBuilderTypeChecking, "tree-builder-type-checking") \
-  X(Library, "library")                                    \
-  X(TreeBuilderV2, "tree-builder-v2")                      \
-  X(GenJitDebug, "gen-jit-debug")                          \
-  X(JitLogging, "jit-logging")                             \
-  X(JitTiming, "jit-timing")                               \
+#define OI_FEATURE_LIST                         \
+  X(ChaseRawPointers, "chase-raw-pointers")     \
+  X(PackStructs, "pack-structs")                \
+  X(GenPaddingStats, "gen-padding-stats")       \
+  X(CaptureThriftIsset, "capture-thrift-isset") \
+  X(TypeGraph, "type-graph")                    \
+  X(PruneTypeGraph, "prune-type-graph")         \
+  X(Library, "library")                         \
+  X(TreeBuilderV2, "tree-builder-v2")           \
+  X(GenJitDebug, "gen-jit-debug")               \
+  X(JitLogging, "jit-logging")                  \
+  X(JitTiming, "jit-timing")                    \
   X(PolymorphicInheritance, "polymorphic-inheritance")
 
 namespace oi::detail {