diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 698b5af..170e698 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/bench/README.md b/bench/README.md index 9f18bd2..5faa056 100644 --- a/bench/README.md +++ b/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. diff --git a/bench/scratch_scopes.rb b/bench/scratch_scopes.rb new file mode 100644 index 0000000..0915b4c --- /dev/null +++ b/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' diff --git a/m_state.mere b/m_state.mere index adb1f54..e94e64a 100644 --- a/m_state.mere +++ b/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) -> diff --git a/main.mere b/main.mere index 3a4aec6..05180ee 100644 --- a/main.mere +++ b/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 diff --git a/unittest/scratch_scope_check.py b/unittest/scratch_scope_check.py new file mode 100644 index 0000000..3a02f75 --- /dev/null +++ b/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}')