Three performance improvements (#60)

* Small optimization to avoid a frequent call to C\lastx, which is slow for some reason when running in local Docker. * Small optimization to avoid a frequent call to C\first, which is slow for some reason when running in local Docker. * Implement short-circuit behavior for AND and OR clauses: when possible, return without evaluating $right.
slackhq · Jan 5, 2021 · c2e22dc · c2e22dc
1 parent bf8d27c
commit c2e22dc
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 18 deletions.
diff --git a/src/Expressions/BinaryOperatorExpression.php b/src/Expressions/BinaryOperatorExpression.php
@@ -113,9 +113,6 @@ public function evaluateImpl(row $row, AsyncMysqlConnection $conn): mixed {
       throw new SQLFakeRuntimeException('Attempted to evaluate BinaryOperatorExpression with no right operand');
     }
 
-    $l_value = $left->evaluate($row, $conn);
-    $r_value = $right->evaluate($row, $conn);
-
     $as_string = $left->getType() == TokenType::STRING_CONSTANT || $right->getType() == TokenType::STRING_CONSTANT;
 
     $op = $this->operator;
@@ -124,17 +121,36 @@ public function evaluateImpl(row $row, AsyncMysqlConnection $conn): mixed {
       throw new SQLFakeRuntimeException('Attempted to evaluate BinaryOperatorExpression with empty operator');
     }
 
+    // special handling for AND/OR - when possible, return without evaluating $right
+    if ($op === Operator::AND) {
+      $l_value = $left->evaluate($row, $conn);
+      if (!$l_value) {
+        return $this->negated;
+      }
+      $r_value = $right->evaluate($row, $conn);
+      if (!$r_value) {
+        return $this->negated;
+      }
+      return !$this->negated;
+    } else if ($op === Operator::OR) {
+      $l_value = $left->evaluate($row, $conn);
+      if ($l_value) {
+        return !$this->negated;
+      }
+      $r_value = $right->evaluate($row, $conn);
+      if ($r_value) {
+        return !$this->negated;
+      }
+      return $this->negated;
+    }
+
+    $l_value = $left->evaluate($row, $conn);
+    $r_value = $right->evaluate($row, $conn);
+
     switch ($op) {
       case Operator::AND:
-        if ((bool)$l_value && (bool)$r_value) {
-          return !$this->negated;
-        }
-        return $this->negated;
       case Operator::OR:
-        if ((bool)$l_value || (bool)$r_value) {
-          return !$this->negated;
-        }
-        return $this->negated;
+        invariant(false, 'impossible to arrive here');
       case Operator::EQUALS:
         // maybe do some stuff with data types here
         // comparing strings: gotta think about collation and case sensitivity!

diff --git a/src/Expressions/ColumnExpression.php b/src/Expressions/ColumnExpression.php
@@ -58,7 +58,18 @@ public function evaluateImpl(row $row, AsyncMysqlConnection $_conn): mixed {
       // but only if the column expression didn't have an explicit table name on it
       // OR if we are explicitly allowing fallthrough to the full row, which we do in the ORDER BY clause
       foreach ($row as $key => $col) {
-        if (C\lastx(Str\split($key, '.')) === $this->columnName) {
+        $parts = Str\split($key, '.');
+
+        // find the last part of the string - as an optimization, we deliberately don't call C\lastx
+        $did_iterate = false;
+        $last_part = null;
+        foreach ($parts as $part) {
+          $did_iterate = true;
+          $last_part = $part;
+        }
+        invariant($did_iterate, 'Column name was empty');
+
+        if ($last_part === $this->columnName) {
           return $col;
         }
       }

diff --git a/src/Expressions/Expression.php b/src/Expressions/Expression.php
@@ -2,8 +2,6 @@
 
 namespace Slack\SQLFake;
 
-use namespace HH\Lib\C;
-
 type ExpressionEvaluationOpts = shape(
   ?'encode_json' => bool,
   ?'bool_as_int' => bool,
@@ -115,10 +113,13 @@ public function addRecursiveExpression(token_list $_tokens, int $_pointer, bool
    * this helper lets them extract the first value from the grouping set
    */
   protected function maybeUnrollGroupedDataset(row $rows): row {
-    $first = C\first($rows);
-    if ($first is dict<_, _>) {
-      /* HH_FIXME[4110] generics can't be specified here yet */
-      return $first;
+    // as an optimization, we deliberately don't call C\first
+    foreach ($rows as $row) {
+      if ($row is dict<_, _>) {
+        /* HH_FIXME[4110] generics can't be specified here yet */
+        return $row;
+      }
+      break;
     }
 
     return $rows;