Recycle temporary expression scopes on success and failure

.github/workflows/ci.yml

@@ -163,6 +163,8 @@ jobs:
           python3 unittest/tcp_connect_check.py
       - name: Compare every corpus program
         run: sh run_corpus.sh
+      - name: Check temporary scope reuse on success and exceptions
+        run: python3 unittest/scratch_scope_check.py
       - name: Check YAML store round trips
         run: sh tools/yaml_store_check.sh
       - name: Check large zlib and gzip interoperability
@@ -573,6 +575,10 @@ jobs:
         run: sh tools/build.sh
       - name: Check C nesting headroom
         run: MR_C=mr.c sh tools/bracket_depth_check.sh
+      - name: Check temporary scope reuse on success and exceptions
+        run: |
+          ulimit -s 524288
+          python3 unittest/scratch_scope_check.py
       - name: Check deferred string-page compaction
         run: python3 unittest/string_page_compact_check.py
       - name: Smoke test

bench/README.md

@@ -511,3 +511,26 @@ The `file_read_checks.rb` benchmark above also exercises this path through
 28.12 seconds to 24.47 seconds with identical output. Other validation ran
 concurrently; this is indicative, not an isolated benchmark or a CI duration
 claim. Per-line interpreter work still dominates much of this workload.
+
+## Temporary expression scopes
+
+```sh
+MERE_RUBY_STORE_STATS=1 ./mere-ruby bench/scratch_scopes.rb 10000
+python3 unittest/scratch_scope_check.py
+```
+
+The workload keeps a closure over the surrounding scope, performs comparisons
+and attribute/index updates, and repeats accessors and comparisons that raise.
+Its output checks values, the retained closure and the original exceptions.
+The checker compares fresh frame counts at 100 and 10,000 iterations, with
+hash indexing ON and OFF; it does not rely on an RSS threshold.
+
+On main `e45622a`, the indexed runs allocated 602 and 60,002 fresh frames.
+Clearing the parent's capture marker from each detached temporary and releasing
+it on both success and failure reduced those counts to 3 and 3 in both modes.
+A real capture of the temporary still keeps it alive and registers it for GC.
+
+With the pinned O2 compiler `991f299b`, Ruby 4.0.9's
+`TestHashOnly#test_broken_hash_value` previously exceeded the 2 GiB family RSS
+limit (2,103,536 KiB observed). After this change it passed both assertions in
+31.9 seconds with 269,344 KiB peak family RSS. The inputs and limits are unchanged.

bench/scratch_scopes.rb

@@ -0,0 +1,48 @@
+# Run with MERE_RUBY_STORE_STATS=1 to compare pool_news as count grows.
+count = Integer(ARGV.fetch(0, '20000'))
+marker = :kept
+keep = proc { marker }
+cell = Struct.new(:value).new(0)
+items = [0]
+count.times do |i|
+  raise 'comparison' if i + 1 != 1 + i
+  cell.value += 1
+  items[0] += 1
+end
+
+class ScratchFailure
+  def value
+    raise 'getter'
+  end
+  def [](_index)
+    raise 'index'
+  end
+  def ==(_other)
+    raise 'comparison'
+  end
+end
+bad = ScratchFailure.new
+errors = 0
+count.times do
+  begin
+    bad.value += 1
+  rescue RuntimeError => error
+    raise unless error.message == 'getter'
+    errors += 1
+  end
+  begin
+    bad[0] += 1
+  rescue RuntimeError => error
+    raise unless error.message == 'index'
+    errors += 1
+  end
+  begin
+    bad == Object.new
+  rescue RuntimeError => error
+    raise unless error.message == 'comparison'
+    errors += 1
+  end
+end
+raise 'values' unless cell.value == count && items[0] == count && errors == count * 3
+raise 'capture' unless keep.call == :kept
+puts 'scratch scopes: values, captures and exceptions preserved'

m_state.mere

@@ -2875,9 +2875,21 @@ let lv_flat_pooled = fn env ->
   let d = frame_pool_get () in
   let _ = lv_flat_into env d in
   let _ = if map_has d "__eid" then map_delete d "__eid" else () in
+  // This is a new detached frame. A closure holding the parent does not
+  // hold this copy; inheriting its mark prevented every scratch frame from
+  // returning to the pool. A capture of the copy sets its own mark and id.
+  let _ = if map_has d "__captured" then map_delete d "__captured" else () in
   d;
 let scratch_env_done = fn d ->
-  if map_has d "__captured" then () else frame_pool_put d;
+  blk_env_put d;
+// Accessors and comparison methods may raise before the expression returns.
+// Release the temporary on that path too, preserving the original failure.
+let with_scratch_env = fn env -> fn f ->
+  let d = lv_flat_pooled env in
+  let out = try_or_msg (fn (u: unit) -> f d)
+                      (fn (msg: str) -> let _ = scratch_env_done d in fail msg) in
+  let _ = scratch_env_done d in
+  out;
 // read a map whose keys are "0".."n-1" (insertion order) back into a list;
 // used to collect map_iter results (which can only side-effect) into an array.
 let rec idx_to_list = fn tmp -> fn (i: int) -> fn (n: int) ->

main.mere

@@ -4008,7 +4008,7 @@ let rec eval_e = fn env -> fn world -> fn blk -> fn (e: Expr) ->
     // (a pooled scratch env -- see lv_flat_pooled -- so the value returned is
     //  the one computed here, never one read back out of it: Ruby's value of
     //  `recv.attr op= v` is the value assigned, not what the setter returns)
-    let e2 = lv_flat_pooled env in
+    with_scratch_env env (fn e2 ->
     let _ = map_set e2 "__opr" r in
     // self-receiver keeps ESelf so private accessors stay reachable (self is
     // side-effect-free, so evaluating it twice is harmless).
@@ -4023,14 +4023,13 @@ let rec eval_e = fn env -> fn world -> fn blk -> fn (e: Expr) ->
        else
          (let v = eval_e env world blk rhs_e in
           set9 (apply_bin_vals world blk op cur v))) in
-    let _ = scratch_env_done e2 in
-    res9
+    res9)
   // recv[idx..] op= rhs — evaluate recv and the indices once (bound to temps).
   | EIdxOp (recv_e, idx_es, op, rhs_e) ->
     let r = eval_e env world blk recv_e in
     let idxs = eval_args env world blk idx_es in
     // (a pooled scratch env, as for EAttrOp above)
-    let e2 = lv_flat_pooled env in
+    with_scratch_env env (fn e2 ->
     let _ = map_set e2 "__opr" r in
     let rx = (match recv_e with ESelf -> ESelf | _ -> EVar "__opr") in
     let ies = idx_temp_exprs e2 idxs 0 in
@@ -4044,8 +4043,7 @@ let rec eval_e = fn env -> fn world -> fn blk -> fn (e: Expr) ->
        else
          (let v = eval_e env world blk rhs_e in
           set9 (apply_bin_vals world blk op cur v))) in
-    let _ = scratch_env_done e2 in
-    res9
+    res9)
   // `self.using M` is `using M`: a private method called through `self.`
   // (only the literal `self`: `recv = self; recv.using M` is refused, as
   // ruby refuses a private call with any other receiver)
@@ -4510,12 +4508,11 @@ let rec eval_e = fn env -> fn world -> fn blk -> fn (e: Expr) ->
     //   comparison is run over two temporaries, which read twice harmlessly.
     else if (str_eq op "==" || str_eq op "!=") && ebin_rhs_effects b then
       (let bv9 = eval_e env world blk b in
-       let e2 = lv_flat_pooled env in
+       with_scratch_env env (fn e2 ->
        let _ = map_set e2 "__opa" va0 in
        let _ = map_set e2 "__opb" bv9 in
        let r9 = eval_e e2 world blk (EBin (op, EVar "__opa", EVar "__opb")) in
-       let _ = scratch_env_done e2 in
-       r9)
+       r9))
     else
     // ⚠ TWO INTEGERS AND A PLAIN OPERATOR, before even the chain of operator
     //   names below (`<<`, `==`, `!=` ...): `i < n` and `i += 1` paid a dozen

unittest/scratch_scope_check.py

@@ -0,0 +1,26 @@
+#!/usr/bin/env python3
+"""Temporary expression scopes must return to the frame pool as a loop grows."""
+import os
+from pathlib import Path
+import re
+import subprocess
+
+ROOT = Path(__file__).resolve().parent.parent
+binary = os.environ.get('MR_BIN', str(ROOT / 'mere-ruby'))
+for indexing in ('1', '0'):
+    news = []
+    for count in (100, 10000):
+        env = dict(os.environ, MERE_RUBY_STORE_STATS='1')
+        if indexing == '0':
+            env['MERE_RUBY_NO_HASH_INDEX'] = '1'
+        else:
+            env.pop('MERE_RUBY_NO_HASH_INDEX', None)
+        result = subprocess.run([binary, str(ROOT / 'bench/scratch_scopes.rb'), str(count)],
+                                env=env, capture_output=True, text=True, timeout=60, check=True)
+        assert result.stdout == 'scratch scopes: values, captures and exceptions preserved\n', result.stdout
+        match = re.search(r'^pool_news\t(\d+)$', result.stderr, re.M)
+        assert match, result.stderr
+        news.append(int(match[1]))
+    # This measures allocation counts, not a platform-dependent RSS threshold.
+    assert news[1] <= news[0] + 32, (indexing, news)
+    print(f'hash indexing {indexing}: fresh frames at 100/10000 iterations = {news}')