Action metrics (#85)

**New Features**
- A new `REGOCPP_ACTION_METRICS` option is available. When turned on, it will compile rego-cpp with
  instrumentation to measure the number of times a Trieste action is executed and how long is spent
  in each.
- A new internal `join` method.

**Improvements**
- `ValueDef` now caches its `str` and `json` representations instead of computing them each time.
  This has had a significant impact on unification (~5x speedup)
- Use of the new `join` method reduces allocations and copies for common string operations
- The unifier can now act over `Object`, `Set`, and `Array` nodes directly.
- The `functions` pass has been refactored to produce fewer temporary variables and to perform
  fewer rewrites, reducing its runtime by half.
- The local variable fixing that used to happen in `implicit_enums` now happens in its own pass, `enum_locals` resulting in a speedup for both.

---------

Signed-off-by: Matthew A Johnson <[email protected]>

Author: matajoh

CHANGELOG

@@ -1,5 +1,22 @@
 # Changelog
 
+## 2023-09-21 - Version 0.3.10
+Instrumentation and optimization. 
+
+**New Features**
+- A new `REGOCPP_ACTION_METRICS` option is available. When turned on, it will compile rego-cpp with
+  instrumentation to measure the number of times a Trieste action is executed and how long is spent
+  in each.
+- A new internal `join` method.
+
+**Improvements**
+- `ValueDef` now caches its `str` and `json` representations instead of computing them each time.
+  This has had a significant impact on unification (~5x speedup)
+- Use of the new `join` method reduces allocations and copies for common string operations
+- The unifier can now act over `Object`, `Set`, and `Array` nodes directly.
+- The `functions` pass has been refactored to produce fewer temporary variables and to perform
+  fewer rewrites, reducing its runtime by half.
+
 ## 2023-09-20 - Version 0.3.9
 Fixing several impactful bugs.
 

CMakeLists.txt

@@ -66,10 +66,11 @@ option(REGOCPP_BUILD_DOCS "Specifies whether to build the documentation" OFF)
 option(REGOCPP_OPA_TESTS "Specifies whether to load and run the OPA built-ins tests" OFF)
 option(REGOCPP_SPECULATIVE_LOAD_HARDENING "Specifies whether to enable speculative load hardening, only supported with Clang" OFF)
 option(REGOCPP_COPY_EXAMPLES "Specifies whether to copy the examples to the install directory" OFF)
-option(REGOCPP_USE_CXX_17 "Specifies whether to use C++17" OFF)
+option(REGOCPP_USE_CXX17 "Specifies whether to use C++17" OFF)
+option(REGOCPP_ACTION_METRICS "Specifies whether to metricate Trieste Actions" OFF)
 set(REGOCPP_SANITIZE "" CACHE STRING "Argument to pass to sanitize (disabled by default)")
 
-if(REGOCPP_USE_CXX_17)
+if(REGOCPP_USE_CXX17)
   set(CMAKE_CXX_STANDARD 17)
 else()
   set(CMAKE_CXX_STANDARD 20)
@@ -87,7 +88,7 @@ FetchContent_Declare(
 
 set(CMAKE_POLICY_DEFAULT_CMP0077 NEW) 
 
-set(TRIESTE_USE_CXX_17 ${REGOCPP_USE_CXX_17})
+set(TRIESTE_USE_CXX17 ${REGOCPP_USE_CXX17})
 set(TRIESTE_BUILD_SAMPLES OFF)
 
 if (REGOCPP_SPECULATIVE_LOAD_HARDENING)

VERSION

@@ -1 +1 @@
-0.3.9
+0.3.10

examples/rust/Cargo.toml

@@ -6,5 +6,5 @@ edition = "2021"
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
-regorust = "0.3.9"
+regorust = "0.3.10"
 clap = { version = "4.0", features = ["derive"] }

include/rego/rego.hh

@@ -713,12 +713,13 @@ namespace rego
   // clang-format on
 
   inline const auto wf_pass_implicit_enums = wf_pass_init;
+  inline const auto wf_pass_enum_locals = wf_pass_implicit_enums;
 
   inline const auto wf_rulebody_exprs = (wf_assign_exprs - AssignInfix);
 
   // clang-format off
   inline const auto wf_pass_rulebody =
-    wf_pass_implicit_enums
+    wf_pass_enum_locals
     | (Module <<= (Import | RuleComp | RuleFunc | RuleSet | RuleObj | Submodule)++)
     | (UnifyExpr <<= Var * (Val >>= Expr))
     | (Expr <<= wf_rulebody_exprs)
@@ -750,9 +751,9 @@ namespace rego
   // clang-format off
   inline const auto wf_pass_functions =
     wf_pass_lift_to_rule
-    | (UnifyExpr <<= Var * (Val >>= Var | Scalar | Function))
+    | (UnifyExpr <<= Var * (Val >>= Var | Scalar | Array | Set | Object | Function))
     | (Function <<= JSONString * ArgSeq)
-    | (ArgSeq <<= (Scalar | Var | wf_arith_op | wf_bin_op | wf_bool_op | NestedBody | VarSeq)++)
+    | (ArgSeq <<= (Scalar | Object | Array | Set | Var | wf_arith_op | wf_bin_op | wf_bool_op | NestedBody | VarSeq)++)
     | (Input <<= Key * (Val >>= Term | Undefined))[Key]
     | (Array <<= Term++)
     | (Set <<= Term++)

src/CMakeLists.txt

@@ -97,13 +97,17 @@ if (REGOCPP_SANITIZE)
   target_link_libraries(rego PUBLIC -fsanitize=${REGOCPP_SANITIZE})
 endif()
 
-if(REGOCPP_USE_CXX_17)
+if(REGOCPP_USE_CXX17)
   target_compile_features(rego PUBLIC cxx_std_17)
-  target_compile_definitions(rego PUBLIC REGOCPP_USE_CXX_17)
+  target_compile_definitions(rego PUBLIC REGOCPP_USE_CXX17)
 else()
   target_compile_features(rego PUBLIC cxx_std_20)
 endif()
 
+if(REGOCPP_ACTION_METRICS)
+  target_compile_definitions(rego PUBLIC REGOCPP_ACTION_METRICS)
+endif()
+
 target_include_directories( rego
   PUBLIC
     $<INSTALL_INTERFACE:include>

src/builtins.cc

@@ -16,19 +16,24 @@ namespace
 
   Node print(const Nodes& args)
   {
-    std::ostringstream buf;
-    std::string sep = "";
     for (auto arg : args)
     {
       if (arg->type() == Undefined)
       {
         return Resolver::scalar(false);
       }
-      buf << sep << to_json(arg);
-      sep = " ";
     }
-    buf << std::endl;
-    std::cout << buf.str();
+
+    join(
+      std::cout,
+      args.begin(),
+      args.end(),
+      " ",
+      [](std::ostream& stream, const Node& n) {
+        stream << to_json(n, true);
+        return true;
+      })
+      << std::endl;
     return Resolver::scalar(true);
   }
 

src/builtins/aggregates.cc

@@ -76,7 +76,12 @@ namespace
       return collection;
     }
 
-    Nodes items(collection->begin(), collection->end());
+    Nodes items;
+    for (const Node& item : *collection)
+    {
+      items.push_back(item->clone());
+    }
+
     std::sort(items.begin(), items.end(), [](const Node& a, const Node& b) {
       return to_json(a) < to_json(b);
     });

src/builtins/strings.cc

@@ -51,11 +51,13 @@ namespace
     }
 
     std::ostringstream result_str;
-    std::string sep = "";
-    for (auto& item : items)
+    for (auto it = items.begin(); it != items.end(); ++it)
     {
-      result_str << sep << item;
-      sep = delim_str;
+      if (it != items.begin())
+      {
+        result_str << delim_str;
+      }
+      result_str << *it;
     }
 
     return Resolver::scalar(result_str.str());

src/encoding.cc

@@ -176,12 +176,15 @@ namespace rego
       }
 
       buf << "[";
-      std::string sep = "";
-      for (const auto& item : items)
-      {
-        buf << sep << item;
-        sep = ", ";
-      }
+      join(
+        buf,
+        items.begin(),
+        items.end(),
+        ", ",
+        [](std::ostream& stream, const std::string& item) {
+          stream << item;
+          return true;
+        });
       buf << "]";
     }
     else if (node->type() == Set || node->type() == DataSet)
@@ -202,13 +205,17 @@ namespace rego
       {
         buf << "<";
       }
-      std::string sep = "";
-      for (const auto& node_key : node_keys)
-      {
-        std::string key_str = to_json(node_key.node, set_as_array, sort_arrays);
-        buf << sep << key_str;
-        sep = ", ";
-      }
+
+      join(
+        buf,
+        node_keys.begin(),
+        node_keys.end(),
+        ", ",
+        [set_as_array,
+         sort_arrays](std::ostream& stream, const NodeKey& node_key) {
+          stream << to_json(node_key.node, set_as_array, sort_arrays);
+          return true;
+        });
 
       if (set_as_array)
       {
@@ -222,7 +229,6 @@ namespace rego
     else if (node->type() == Object || node->type() == DataObject)
     {
       std::map<std::string, std::string> items;
-      std::vector<NodeKey> keys;
       for (const auto& child : *node)
       {
         auto key = child / Key;
@@ -236,13 +242,15 @@ namespace rego
       }
 
       buf << "{";
-      std::string sep = "";
-      for (const auto& [key, value] : items)
-      {
-        buf << sep << key << ":" << value;
-        sep = ", ";
-      }
-
+      join(
+        buf,
+        items.begin(),
+        items.end(),
+        ", ",
+        [](std::ostream& stream, const auto& item) {
+          stream << item.first << ":" << item.second;
+          return true;
+        });
       buf << "}";
     }
     else if (
@@ -259,12 +267,15 @@ namespace rego
     else if (node->type() == TermSet)
     {
       buf << "termset{";
-      std::string sep = "";
-      for (const auto& child : *node)
-      {
-        buf << sep << to_json(child, set_as_array, sort_arrays);
-        sep = ", ";
-      }
+      join(
+        buf,
+        node->begin(),
+        node->end(),
+        ", ",
+        [set_as_array, sort_arrays](std::ostream& stream, const Node& n) {
+          stream << to_json(n, set_as_array, sort_arrays);
+          return true;
+        });
       buf << "}";
     }
     else if (node->type() == BuiltInHook)

src/internal.cc

@@ -206,26 +206,32 @@ namespace rego
 
   std::ostream& operator<<(std::ostream& os, const std::set<Location>& locs)
   {
-    std::string sep = "";
     os << "{";
-    for (const auto& loc : locs)
-    {
-      os << sep << loc.view();
-      sep = ", ";
-    }
+    join(
+      os,
+      locs.begin(),
+      locs.end(),
+      ", ",
+      [](std::ostream& stream, const Location& loc) {
+        stream << loc.view();
+        return true;
+      });
     os << "}";
     return os;
   }
 
   std::ostream& operator<<(std::ostream& os, const std::vector<Location>& locs)
   {
-    std::string sep = "";
     os << "[";
-    for (const auto& loc : locs)
-    {
-      os << sep << loc.source->origin() << ":" << loc.view();
-      sep = ", ";
-    }
+    join(
+      os,
+      locs.begin(),
+      locs.end(),
+      ", ",
+      [](std::ostream& stream, const Location& loc) {
+        stream << loc.view();
+        return true;
+      });
     os << "]";
     return os;
   }
@@ -564,12 +570,15 @@ namespace rego
       if (m_types.size() > 1)
       {
         error << "must be one of {";
-        std::string sep = "";
-        for (auto& type : m_types)
-        {
-          error << sep << type_name(type, m_specify_number);
-          sep = ", ";
-        }
+        join(
+          error,
+          m_types.begin(),
+          m_types.end(),
+          ", ",
+          [&](std::ostream& stream, const Token& type) {
+            stream << type_name(type, m_specify_number);
+            return true;
+          });
         error << "}";
       }
       else if (m_types.size() == 1)
@@ -784,4 +793,45 @@ namespace rego
   {
     return var->type().in({Submodule, DataItem, Data});
   }
+
+  ActionMetrics::ActionMetrics(const char* file, std::size_t line) :
+    m_key{file, line}, m_start(ActionMetrics::clock::now())
+  {}
+
+  ActionMetrics::~ActionMetrics()
+  {
+    if (!contains(ActionMetrics::s_action_info, m_key))
+    {
+      ActionMetrics::s_action_info.insert(
+        {m_key, {0, ActionMetrics::duration::zero()}});
+    }
+
+    auto elapsed = ActionMetrics::clock::now() - m_start;
+    ActionMetrics::s_action_info[m_key].count++;
+    ActionMetrics::s_action_info[m_key].time_spent += elapsed;
+  }
+
+  std::map<ActionMetrics::key_t, ActionMetrics::info_t>
+    ActionMetrics::s_action_info = {};
+
+  void ActionMetrics::print()
+  {
+    std::cout << "Action\tCount\tTime(ms)" << std::endl;
+    for (auto& [key, info] : ActionMetrics::s_action_info)
+    {
+      std::chrono::duration<double, std::milli> fp_ms = info.time_spent;
+      std::cout << key.file << ":" << key.line << "\t" << info.count << "\t"
+                << fp_ms.count() << std::endl;
+    }
+  }
+
+  bool ActionMetrics::key_t::operator<(const ActionMetrics::key_t& other) const
+  {
+    if (file != other.file)
+    {
+      return file < other.file;
+    }
+
+    return line < other.line;
+  }
 }