core: improve expr.toString performance (#1706)

Creating a string from the expression can be a common operation when creating messages. This change reduces the overhead by allowing it to be written to an existing builder instance and reduce the amount of internal resizing and copies. **Before** ``` Benchmark Mode Cnt Score Error Units withEscapes thrpt 10 581,213.514 ± 4708.897 ops/s withNoEscapes thrpt 10 643,706.596 ± 9144.301 ops/s ``` **After** ``` Benchmark Mode Cnt Score Error Units withEscapes thrpt 10 956,439.595 ± 5243.582 ops/s withNoEscapes thrpt 10 1,031,542.340 ± 25261.261 ops/s ```
Netflix · Oct 15, 2024 · 0dea833 · 0dea833
1 parent 3df773f
commit 0dea833
Show file tree

Hide file tree

Showing 13 changed files with 403 additions and 82 deletions.
diff --git a/atlas-core/src/main/scala/com/netflix/atlas/core/model/DataExpr.scala b/atlas-core/src/main/scala/com/netflix/atlas/core/model/DataExpr.scala
@@ -60,8 +60,10 @@ sealed trait DataExpr extends TimeSeriesExpr with Product {
     ResultSet(this, data.getOrElse(this, Nil), context.state)
   }
 
-  override def toString: String = {
-    if (offset.isZero) exprString else s"$exprString,$offset,:offset"
+  override def append(builder: java.lang.StringBuilder): Unit = {
+    builder.append(exprString)
+    if (!offset.isZero)
+      builder.append(',').append(offset).append(",:offset")
   }
 
   /**

diff --git a/atlas-core/src/main/scala/com/netflix/atlas/core/model/EventExpr.scala b/atlas-core/src/main/scala/com/netflix/atlas/core/model/EventExpr.scala
@@ -34,7 +34,9 @@ object EventExpr {
     */
   case class Raw(query: Query) extends EventExpr {
 
-    override def toString: String = query.toString
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, query)
+    }
   }
 
   /**
@@ -49,6 +51,8 @@ object EventExpr {
 
     require(columns.nonEmpty, "set of columns cannot be empty")
 
-    override def toString: String = Interpreter.toString(query, columns, ":table")
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, query, columns, ":table")
+    }
   }
 }
diff --git a/atlas-core/src/main/scala/com/netflix/atlas/core/model/Expr.scala b/atlas-core/src/main/scala/com/netflix/atlas/core/model/Expr.scala
@@ -15,7 +15,18 @@
  */
 package com.netflix.atlas.core.model
 
-trait Expr extends Product {
+import com.netflix.atlas.core.stacklang.StackItem
+
+trait Expr extends Product with StackItem {
+
+  /** Use builder for encoding as a string. Sub-classes should override the append method. */
+  override def toString: String = {
+    // 256 is a rough size that isn't too large to waste a lot, but also limit the
+    // need for growing the buffer with typical expressions
+    val builder = new java.lang.StringBuilder(256)
+    append(builder)
+    builder.toString
+  }
 
   /**
     * Returns a string that can be executed with the stack interpreter to create this expression.

diff --git a/atlas-core/src/main/scala/com/netflix/atlas/core/model/FilterExpr.scala b/atlas-core/src/main/scala/com/netflix/atlas/core/model/FilterExpr.scala
@@ -39,7 +39,12 @@ object FilterExpr {
   case class Stat(expr: TimeSeriesExpr, stat: String, str: Option[String] = None)
       extends FilterExpr {
 
-    override def toString: String = str.getOrElse(s"$expr,$stat,:stat")
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      str match {
+        case Some(s) => builder.append(s)
+        case None    => Interpreter.append(builder, expr, stat, ":stat")
+      }
+    }
 
     def dataExprs: List[DataExpr] = expr.dataExprs
 
@@ -64,7 +69,9 @@ object FilterExpr {
 
     def name: String
 
-    override def toString: String = s":stat-$name"
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, s":stat-$name")
+    }
 
     def dataExprs: List[DataExpr] = Nil
 
@@ -113,7 +120,9 @@ object FilterExpr {
 
     if (!expr1.isGrouped) require(!expr2.isGrouped, "filter grouping must match expr grouping")
 
-    override def toString: String = Interpreter.toString(expr1, expr2, ":filter")
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, expr1, expr2, ":filter")
+    }
 
     def dataExprs: List[DataExpr] = expr1.dataExprs ::: expr2.dataExprs
 
@@ -183,7 +192,9 @@ object FilterExpr {
 
     require(k > 0, s"k must be positive ($k <= 0)")
 
-    override def toString: String = Interpreter.toString(expr, stat, k, s":$opName")
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, expr, stat, k, s":$opName")
+    }
 
     def dataExprs: List[DataExpr] = expr.dataExprs
 

diff --git a/atlas-core/src/main/scala/com/netflix/atlas/core/model/MathExpr.scala b/atlas-core/src/main/scala/com/netflix/atlas/core/model/MathExpr.scala
@@ -30,6 +30,8 @@ import com.netflix.atlas.core.util.Math
 import com.netflix.atlas.core.util.Strings
 import com.netflix.spectator.api.histogram.PercentileBuckets
 
+import scala.collection.immutable.ArraySeq
+
 trait MathExpr extends TimeSeriesExpr
 
 object MathExpr {
@@ -49,7 +51,9 @@ object MathExpr {
 
     def dataExprs: List[DataExpr] = expr.dataExprs
 
-    override def toString: String = Interpreter.toString(expr, original, replacement, ":as")
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, expr, original, replacement, ":as")
+    }
 
     def isGrouped: Boolean = expr.isGrouped
 
@@ -83,7 +87,9 @@ object MathExpr {
 
     def dataExprs: List[DataExpr] = Nil
 
-    override def toString: String = Interpreter.toString(v, ":const")
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, v, ":const")
+    }
 
     def isGrouped: Boolean = false
 
@@ -107,7 +113,9 @@ object MathExpr {
 
     def dataExprs: List[DataExpr] = Nil
 
-    override def toString: String = ":random"
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, ":random")
+    }
 
     def isGrouped: Boolean = false
 
@@ -135,7 +143,9 @@ object MathExpr {
 
     def dataExprs: List[DataExpr] = Nil
 
-    override def toString: String = Interpreter.toString(seed, ":srandom")
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, seed, ":srandom")
+    }
 
     def isGrouped: Boolean = false
 
@@ -196,7 +206,9 @@ object MathExpr {
 
     def dataExprs: List[DataExpr] = Nil
 
-    override def toString: String = s"$mode,:time"
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, mode, ":time")
+    }
 
     def isGrouped: Boolean = false
 
@@ -215,7 +227,9 @@ object MathExpr {
 
     def dataExprs: List[DataExpr] = Nil
 
-    override def toString: String = Interpreter.toString(s, e, ":time-span")
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, s, e, ":time-span")
+    }
 
     def isGrouped: Boolean = false
 
@@ -286,7 +300,9 @@ object MathExpr {
 
     def dataExprs: List[DataExpr] = expr.dataExprs
 
-    override def toString: String = Interpreter.toString(expr, min, s":$name")
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, expr, min, s":$name")
+    }
 
     def isGrouped: Boolean = expr.isGrouped
 
@@ -316,6 +332,10 @@ object MathExpr {
 
     override def toString: String = Interpreter.toString(expr, max, s":$name")
 
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, expr, max, s":$name")
+    }
+
     def isGrouped: Boolean = expr.isGrouped
 
     def groupByKey(tags: Map[String, String]): Option[String] = expr.groupByKey(tags)
@@ -344,7 +364,16 @@ object MathExpr {
 
     def dataExprs: List[DataExpr] = expr.dataExprs
 
-    override def toString: String = Interpreter.toString(expr, s":$name")
+    override def toString: String = {
+      // Needs to be overridden here or it will get the copy from the based Function type
+      val builder = new java.lang.StringBuilder()
+      append(builder)
+      builder.toString
+    }
+
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, expr, s":$name")
+    }
 
     def isGrouped: Boolean = expr.isGrouped
 
@@ -456,7 +485,16 @@ object MathExpr {
 
     def dataExprs: List[DataExpr] = expr1.dataExprs ::: expr2.dataExprs
 
-    override def toString: String = Interpreter.toString(expr1, expr2, s":$name")
+    override def toString: String = {
+      // Needs to be overridden here or it will get the copy from the based Function type
+      val builder = new java.lang.StringBuilder()
+      append(builder)
+      builder.toString
+    }
+
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, expr1, expr2, s":$name")
+    }
 
     def isGrouped: Boolean = expr1.isGrouped || expr2.isGrouped
 
@@ -680,7 +718,9 @@ object MathExpr {
 
     def dataExprs: List[DataExpr] = expr.dataExprs
 
-    override def toString: String = Interpreter.toString(expr, s":$name")
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, expr, s":$name")
+    }
 
     def isGrouped: Boolean = false
 
@@ -766,7 +806,9 @@ object MathExpr {
       }
     }
 
-    override def toString: String = Interpreter.toString(expr, keys, ":by")
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      Interpreter.append(builder, expr, keys, ":by")
+    }
 
     def dataExprs: List[DataExpr] = expr.dataExprs
 
@@ -828,16 +870,20 @@ object MathExpr {
     private val evalGroupKeys = expr.keys.filter(_ != TagKey.percentile)
     private val pcts = percentiles.distinct.sortWith(_ < _).toArray
 
-    override def toString: String = {
-      val baseExpr =
-        if (evalGroupKeys.isEmpty)
-          expr.af.query.toString
-        else
-          Interpreter.toString(expr.af.query, evalGroupKeys, ":by")
-      if (expr.offset.isZero)
-        s"$baseExpr,(,${pcts.mkString(",")},),:percentiles"
-      else
-        s"$baseExpr,(,${pcts.mkString(",")},),:percentiles,${expr.offset},:offset"
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      // Base expr
+      Interpreter.append(builder, expr.af.query)
+      if (evalGroupKeys.nonEmpty)
+        Interpreter.append(builder, expr.af.query, evalGroupKeys, ":by")
+
+      // Percentiles
+      builder.append(",(,")
+      Interpreter.append(builder, ArraySeq.unsafeWrapArray(pcts)*)
+      builder.append(",),:percentiles")
+
+      // Offset
+      if (!expr.offset.isZero)
+        builder.append(',').append(expr.offset).append(",:offset")
     }
 
     override def dataExprs: List[DataExpr] = List(expr)
@@ -995,9 +1041,11 @@ object MathExpr {
 
     def dataExprs: List[DataExpr] = evalExpr.dataExprs
 
-    override def toString: String = toString(displayExpr)
+    override def append(builder: java.lang.StringBuilder): Unit = {
+      append(builder, displayExpr)
+    }
 
-    private def toString(expr: Expr): String = {
+    private def append(builder: java.lang.StringBuilder, expr: Expr): Unit = {
       val op =
         if (displayParams.isEmpty)
           s":$name"
@@ -1009,33 +1057,41 @@ object MathExpr {
           // aggregate function. Modifications to the aggregate need to be represented
           // after the operation as part of the expression string. There are two
           // categories: offsets applied to the data function and group by.
-          val buffer = new java.lang.StringBuilder
-          buffer.append(s"$q,$op")
-          getOffset(evalExpr).foreach(d => buffer.append(s",$d,:offset"))
+          builder.append(s"$q,$op")
+          getOffset(evalExpr).foreach(d => builder.append(s",$d,:offset"))
 
           val grouping = evalExpr.finalGrouping
           if (grouping.nonEmpty) {
-            buffer.append(grouping.mkString(",(,", ",", ",),:by"))
+            builder.append(grouping.mkString(",(,", ",", ",),:by"))
           }
-
-          buffer.toString
         case t: TimeSeriesExpr if groupingMatches =>
           // The passed in expression maybe the result of a rewrite to the display expression
           // that was not applied to the eval expression. If it changes the grouping, then it
           // would alter the toString behavior. So the grouping match check is based on the
           // original display expression.
           val evalOffset = getOffset(evalExpr)
-          evalOffset.fold(s"$t,$op") { d =>
+          val str = evalOffset.fold(s"$t,$op") { d =>
             val displayOffset = getOffset(t).getOrElse(Duration.ZERO)
             if (d != displayOffset) s"${t.withOffset(d)},$op" else s"$t,$op"
           }
+          builder.append(str)
         case _ =>
+          Interpreter.append(builder, expr, op)
           val grouping = evalExpr.finalGrouping
-          val by = if (grouping.nonEmpty) grouping.mkString(",(,", ",", ",),:by") else ""
-          s"$expr,$op$by"
+          if (grouping.nonEmpty) {
+            builder.append(",(,")
+            Interpreter.append(builder, grouping*)
+            builder.append(",),:by")
+          }
       }
     }
 
+    private def toString(expr: Expr): String = {
+      val builder = new java.lang.StringBuilder()
+      append(builder, expr)
+      builder.toString
+    }
+
     private def groupingMatches: Boolean = {
       displayExpr match {
         case t: TimeSeriesExpr => t.finalGrouping == evalExpr.finalGrouping