[perf fix] Do _LeakTick() before if and case, not after

Related to issue #2123. Because control flow raises C++ exceptions, we can skip a _LeafTick() after, which would lead to the same perf bug: if (true) { continue # exception, interpreter doesn't reach the end of 'if' }
oils-for-unix · Nov 17, 2024 · 5bcc9f6 · 5bcc9f6
1 parent 0634190
commit 5bcc9f6
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 22 deletions.
diff --git a/osh/cmd_eval.py b/osh/cmd_eval.py
@@ -1720,28 +1720,28 @@ def _Dispatch(self, node, cmd_st):
             elif case(command_e.If):
                 node = cast(command.If, UP_node)
 
+                # Perf bug fix: loops might only execute 'if', but we still
+                # need to GC
+                self._LeafTick()
+
                 # No SetTokenForLine() because
                 # - $LINENO can't appear directly in 'if'
                 # - 'if' doesn't directly cause errors
                 # It will be taken care of by command.Simple, condition, etc.
                 status = self._DoIf(node)
 
-                # Perf bug fix: loops might only execute 'if', but we still
-                # need to GC
-                self._LeafTick()
-
             elif case(command_e.Case):
                 node = cast(command.Case, UP_node)
 
+                # Perf bug fix: loops might only execute 'case', but we still
+                # need to GC
+                self._LeafTick()
+
                 # Must set location for 'case $LINENO'
                 self.mem.SetTokenForLine(node.case_kw)
                 self._MaybeRunDebugTrap()
                 status = self._DoCase(node)
 
-                # Perf bug fix: loops might only execute 'case', but we still
-                # need to GC
-                self._LeafTick()
-
             elif case(command_e.WhileUntil):
                 node = cast(command.WhileUntil, UP_node)
 

diff --git a/test/bug-2123.sh b/test/bug-2123.sh
@@ -4,8 +4,8 @@ demo() {
   local ysh=_bin/cxx-opt/ysh
   ninja $ysh
 
-  #OILS_GC_STATS=1 _OILS_GC_VERBOSE=1 $ysh test/bug-2123.ysh
-  time OILS_GC_STATS=1 $ysh test/bug-2123.ysh
+  OILS_GC_STATS=1 _OILS_GC_VERBOSE=1 $ysh test/bug-2123.ysh
+  #time OILS_GC_STATS=1 $ysh test/bug-2123.ysh
 
   # max RSS
   # 246

diff --git a/test/bug-2123.ysh b/test/bug-2123.ysh
@@ -1,16 +1,20 @@
 # Run with _bin/cxx-opt/ysh
 
 proc filter (; predicate) {
-  #var i = 0
-	for line in (io.stdin) {
-		if (io->evalExpr(predicate, vars={_val: fromJson8(line)})) {
-			write -- $line
-		}
-    #setvar i += 1
-    #if (i % 1000 === 0) {
-    #  echo "i $i"
-    #}
-	}
+  for line in (io.stdin) {
+    if (io->evalExpr(predicate, vars={_val: fromJson8(line)})) {
+      write -- $line
+    }
+  }
+}
+
+proc filter-continue (; predicate) {
+  for line in (io.stdin) {
+    # BUG FIX: we should GC before 'continue' raises an exception
+    if (not io->evalExpr(predicate, vars={_val: fromJson8(line)})) {
+      continue
+    }
+  }
 }
 
 #
@@ -20,7 +24,7 @@ proc filter (; predicate) {
 var f = '_tmp/b1'
 
 for i in (0 ..= 100_000) {
-	json write ({}, space=0)
+  json write ({}, space=0)
 } > $f
 
 #
@@ -74,7 +78,8 @@ proc without-default {
   #time filter [get(_val, 'missing-key', 0) === 0] < $f >/dev/null
 
   echo '****** BEGIN filter'
-  time filter [get(_val, 'missing-key')] < $f #>/dev/null
+  #time filter [get(_val, 'missing-key')] < $f #>/dev/null
+  time filter-continue [get(_val, 'missing-key')] < $f #>/dev/null
   echo '****** END filter'
 
   write -- 'AFTER -------------------------------------------------------'