Unify garbage collector

[frostney]

Unifies Goccia and Souffle garbage collectors into a single generic GC to fix Windows OOM and access violation errors in the benchmark runner.

The Windows 32-bit BenchmarkRunner was hitting "Out of memory" because the Goccia GC never collected between benchmark files, leading to memory exhaustion. This PR unifies the Goccia and Souffle GCs, enabling explicit collection between files and resolving cross-GC dangling pointer issues, which also fixes pre-existing Invalid type cast errors in the typed-arrays.js benchmark.

---

  

Summary by CodeRabbit

• New Features

  • Unified, runtime-wide garbage collector with centralized object-based pinning and root management.
  • Shared prototypes and singletons are now auto-pinned via the unified collector.

• Bug Fixes

  • More reliable memory reclamation between script/module runs; runners invoke explicit collection to reduce leaks and variance.

• Documentation

  • Architecture, design and API docs updated to reflect the unified GC model and collection semantics.

[cursor]

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
_{Learn more about Cursor Agents}

[coderabbitai]

📝 Walkthrough

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Unify garbage collector' accurately and concisely summarizes the main objective of the changeset: consolidating multiple per-backend garbage collectors (Goccia GC and Souffle GC) into a single unified generic GC.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch cursor/generic-garbage-collector-9852

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

Benchmark Results

254 benchmarks

Interpreted: 🟢 112 improved · 🔴 10 regressed · 132 unchanged · avg +6.5%
Bytecode: 🟢 96 improved · 🔴 11 regressed · 🆕 110 new · 37 unchanged · avg +19.5%

arraybuffer.js — Interp: 🔴 2, 12 unch. · avg -1.1% · Bytecode: 🟢 2, 🔴 4, 8 unch. · avg -2.5%

Benchmark	Interpreted	Δ	Bytecode	Δ
create ArrayBuffer(0)	426,086 → 425,582	-0.1%	153,012 → 139,115	🔴 -9.1%
create ArrayBuffer(64)	418,436 → 425,552	+1.7%	149,701 → 139,804	-6.6%
create ArrayBuffer(1024)	327,375 → 316,639	-3.3%	136,034 → 127,140	-6.5%
create ArrayBuffer(8192)	144,772 → 129,743	🔴 -10.4%	92,505 → 81,510	🔴 -11.9%
slice full buffer (64 bytes)	499,453 → 517,573	+3.6%	358,313 → 358,016	-0.1%
slice half buffer (512 of 1024 bytes)	426,716 → 434,660	+1.9%	322,385 → 323,381	+0.3%
slice with negative indices	419,912 → 392,443	-6.5%	348,659 → 348,130	-0.2%
slice empty range	478,952 → 499,234	+4.2%	359,354 → 357,594	-0.5%
byteLength access	1,493,605 → 1,572,032	+5.3%	899,175 → 988,141	🟢 +9.9%
Symbol.toStringTag access	1,110,585 → 1,121,266	+1.0%	467,622 → 509,487	🟢 +9.0%
ArrayBuffer.isView	690,906 → 721,186	+4.4%	435,114 → 439,028	+0.9%
clone ArrayBuffer(64)	392,547 → 381,465	-2.8%	334,070 → 317,821	-4.9%
clone ArrayBuffer(1024)	302,017 → 283,873	-6.0%	269,362 → 250,228	🔴 -7.1%
clone ArrayBuffer inside object	274,660 → 253,697	🔴 -7.6%	167,743 → 152,751	🔴 -8.9%

arrays.js — Interp: 🟢 6, 13 unch. · avg +5.0% · Bytecode: 🟢 17, 2 unch. · avg +22.7%

Benchmark	Interpreted	Δ	Bytecode	Δ
Array.from length 100	13,902 → 13,844	-0.4%	10,023 → 12,151	🟢 +21.2%
Array.from 10 elements	225,301 → 232,582	+3.2%	143,929 → 157,097	🟢 +9.1%
Array.of 10 elements	313,487 → 306,299	-2.3%	204,638 → 218,647	+6.8%
spread into new array	324,541 → 335,948	+3.5%	409,594 → 564,372	🟢 +37.8%
map over 50 elements	26,450 → 27,729	+4.8%	16,986 → 20,408	🟢 +20.1%
filter over 50 elements	23,245 → 23,396	+0.6%	15,330 → 20,185	🟢 +31.7%
reduce sum 50 elements	26,469 → 26,689	+0.8%	14,684 → 17,881	🟢 +21.8%
forEach over 50 elements	22,600 → 22,567	-0.1%	17,115 → 21,825	🟢 +27.5%
find in 50 elements	34,238 → 36,066	+5.3%	21,435 → 24,837	🟢 +15.9%
sort 20 elements	11,851 → 12,100	+2.1%	2,505 → 2,873	🟢 +14.7%
flat nested array	116,717 → 119,632	+2.5%	214,393 → 269,699	🟢 +25.8%
flatMap	70,393 → 73,637	+4.6%	124,515 → 186,044	🟢 +49.4%
map inside map (5x5)	18,184 → 21,407	🟢 +17.7%	39,428 → 67,631	🟢 +71.5%
filter inside map (5x10)	14,212 → 15,921	🟢 +12.0%	10,911 → 12,774	🟢 +17.1%
reduce inside map (5x10)	18,104 → 19,401	🟢 +7.2%	11,167 → 12,775	🟢 +14.4%
forEach inside forEach (5x10)	15,310 → 16,402	🟢 +7.1%	12,203 → 12,995	+6.5%
find inside some (10x10)	12,825 → 14,016	🟢 +9.3%	8,650 → 9,686	🟢 +12.0%
map+filter chain nested (5x20)	5,352 → 5,397	+0.8%	3,545 → 4,143	🟢 +16.9%
reduce flatten (10x5)	36,285 → 42,165	🟢 +16.2%	5,051 → 5,611	🟢 +11.1%

async-await.js — Interp: 6 unch. · avg +2.5% · Bytecode: 🟢 1, 🔴 1, 4 unch. · avg +0.3%

Benchmark	Interpreted	Δ	Bytecode	Δ
single await	371,361 → 372,689	+0.4%	261,651 → 276,243	+5.6%
multiple awaits	166,437 → 170,510	+2.4%	117,952 → 120,888	+2.5%
await non-Promise value	842,036 → 845,373	+0.4%	857,278 → 946,259	🟢 +10.4%
await with try/catch	357,861 → 371,633	+3.8%	277,196 → 266,808	-3.7%
await Promise.all	50,256 → 52,871	+5.2%	44,779 → 40,009	🔴 -10.7%
nested async function call	185,611 → 190,478	+2.6%	209,785 → 204,665	-2.4%

classes.js — Interp: 🟢 4, 27 unch. · avg +3.4% · Bytecode: 🟢 15, 16 unch. · avg +7.8%

Benchmark	Interpreted	Δ	Bytecode	Δ
simple class new	112,309 → 115,930	+3.2%	297,465 → 333,197	🟢 +12.0%
class with defaults	90,639 → 93,091	+2.7%	196,354 → 231,696	🟢 +18.0%
50 instances via Array.from	5,404 → 5,179	-4.2%	5,684 → 6,151	🟢 +8.2%
instance method call	57,719 → 58,685	+1.7%	117,938 → 152,502	🟢 +29.3%
static method call	87,886 → 92,180	+4.9%	280,787 → 352,139	🟢 +25.4%
single-level inheritance	45,180 → 45,979	+1.8%	115,983 → 146,766	🟢 +26.5%
two-level inheritance	38,394 → 39,065	+1.7%	98,000 → 121,711	🟢 +24.2%
private field access	55,460 → 58,282	+5.1%	129,588 → 168,776	🟢 +30.2%
private methods	61,866 → 63,646	+2.9%	153,033 → 209,350	🟢 +36.8%
getter/setter access	63,419 → 65,271	+2.9%	138,114 → 167,663	🟢 +21.4%
class decorator (identity)	78,003 → 81,535	+4.5%	54,460 → 50,858	-6.6%
class decorator (wrapping)	45,633 → 47,499	+4.1%	36,880 → 36,170	-1.9%
identity method decorator	55,848 → 58,979	+5.6%	44,989 → 42,490	-5.6%
wrapping method decorator	45,854 → 48,933	+6.7%	40,045 → 39,042	-2.5%
stacked method decorators (x3)	32,955 → 34,790	+5.6%	28,967 → 27,410	-5.4%
identity field decorator	63,763 → 67,457	+5.8%	47,580 → 44,623	-6.2%
field initializer decorator	54,735 → 56,904	+4.0%	41,865 → 40,527	-3.2%
getter decorator (identity)	58,431 → 57,875	-1.0%	40,909 → 39,982	-2.3%
setter decorator (identity)	48,503 → 49,065	+1.2%	33,710 → 34,693	+2.9%
static method decorator	62,834 → 62,115	-1.1%	65,628 → 62,560	-4.7%
static field decorator	69,698 → 73,779	+5.9%	69,501 → 65,667	-5.5%
private method decorator	46,902 → 48,270	+2.9%	38,096 → 36,833	-3.3%
private field decorator	49,349 → 53,252	🟢 +7.9%	39,763 → 38,373	-3.5%
plain auto-accessor (no decorator)	91,907 → 90,457	-1.6%	55,493 → 52,300	-5.8%
auto-accessor with decorator	52,044 → 52,806	+1.5%	39,023 → 36,578	-6.3%
decorator writing metadata	43,785 → 44,261	+1.1%	42,421 → 41,030	-3.3%
static getter read	105,236 → 108,021	+2.6%	320,831 → 377,689	🟢 +17.7%
static getter/setter pair	81,355 → 80,573	-1.0%	170,216 → 204,179	🟢 +20.0%
inherited static getter	55,890 → 60,753	🟢 +8.7%	234,202 → 256,723	🟢 +9.6%
inherited static setter	60,559 → 65,760	🟢 +8.6%	182,207 → 201,615	🟢 +10.7%
inherited static getter with this binding	49,967 → 55,136	🟢 +10.3%	130,168 → 150,064	🟢 +15.3%

closures.js — Interp: 🟢 2, 9 unch. · avg +3.5% · Bytecode: 🟢 11 · avg +51.1%

Benchmark	Interpreted	Δ	Bytecode	Δ
closure over single variable	131,677 → 155,589	🟢 +18.2%	361,939 → 602,385	🟢 +66.4%
closure over multiple variables	119,030 → 146,525	🟢 +23.1%	244,426 → 387,771	🟢 +58.6%
nested closures	127,437 → 130,075	+2.1%	353,194 → 553,739	🟢 +56.8%
function as argument	95,765 → 98,351	+2.7%	276,893 → 492,718	🟢 +77.9%
function returning function	124,543 → 123,878	-0.5%	352,381 → 582,372	🟢 +65.3%
compose two functions	74,349 → 73,598	-1.0%	210,064 → 347,099	🟢 +65.2%
fn.call	152,974 → 159,057	+4.0%	116,169 → 128,401	🟢 +10.5%
fn.apply	119,709 → 118,236	-1.2%	76,874 → 88,281	🟢 +14.8%
fn.bind	145,684 → 141,694	-2.7%	123,953 → 138,745	🟢 +11.9%
recursive sum to 50	12,701 → 12,024	-5.3%	21,245 → 38,299	🟢 +80.3%
recursive tree traversal	20,242 → 20,154	-0.4%	37,596 → 57,885	🟢 +54.0%

collections.js — Interp: 🟢 4, 🔴 6, 2 unch. · avg -2.6% · Bytecode: 🟢 2, 🔴 5, 5 unch. · avg -21.8%

Benchmark	Interpreted	Δ	Bytecode	Δ
add 50 elements	7,655 → 7,045	🔴 -8.0%	5,503 → 5,527	+0.4%
has lookup (50 elements)	93,550 → 84,397	🔴 -9.8%	87,068 → 80,679	🔴 -7.3%
delete elements	50,381 → 44,965	🔴 -10.7%	37,885 → 35,382	-6.6%
forEach iteration	15,506 → 16,982	🟢 +9.5%	12,528 → 15,133	🟢 +20.8%
spread to array	32,361 → 39,738	🟢 +22.8%	125,959 → 118,206	-6.2%
deduplicate array	44,147 → 43,876	-0.6%	45,373 → 44,530	-1.9%
set 50 entries	5,447 → 5,076	-6.8%	5,371 → 5,288	-1.5%
get lookup (50 entries)	94,659 → 78,314	🔴 -17.3%	319,818 → 84,154	🔴 -73.7%
has check	142,637 → 118,745	🔴 -16.8%	383,935 → 134,430	🔴 -65.0%
delete entries	49,091 → 41,795	🔴 -14.9%	67,457 → 32,391	🔴 -52.0%
forEach iteration	14,652 → 16,606	🟢 +13.3%	12,888 → 15,176	🟢 +17.8%
keys/values/entries	8,798 → 9,519	🟢 +8.2%	136,204 → 18,423	🔴 -86.5%

destructuring.js — Interp: 🟢 9, 13 unch. · avg +5.2% · Bytecode: 🟢 22 · avg +41.7%

Benchmark	Interpreted	Δ	Bytecode	Δ
simple array destructuring	397,161 → 454,220	🟢 +14.4%	374,245 → 629,079	🟢 +68.1%
with rest element	286,094 → 298,278	+4.3%	290,556 → 490,022	🟢 +68.6%
with defaults	435,197 → 444,672	+2.2%	423,887 → 697,366	🟢 +64.5%
skip elements	421,808 → 457,503	🟢 +8.5%	446,174 → 701,292	🟢 +57.2%
nested array destructuring	183,183 → 196,513	🟢 +7.3%	220,706 → 353,841	🟢 +60.3%
swap variables	554,203 → 549,291	-0.9%	621,677 → 972,419	🟢 +56.4%
simple object destructuring	295,043 → 316,039	🟢 +7.1%	416,373 → 500,808	🟢 +20.3%
with defaults	373,013 → 368,070	-1.3%	283,404 → 319,521	🟢 +12.7%
with renaming	315,130 → 332,935	+5.7%	449,441 → 574,752	🟢 +27.9%
nested object destructuring	137,888 → 159,194	🟢 +15.5%	232,085 → 249,582	🟢 +7.5%
rest properties	187,362 → 191,898	+2.4%	202,504 → 238,701	🟢 +17.9%
object parameter	91,224 → 96,206	+5.5%	153,996 → 187,261	🟢 +21.6%
array parameter	128,837 → 130,249	+1.1%	200,450 → 316,475	🟢 +57.9%
mixed destructuring in map	36,075 → 35,607	-1.3%	27,212 → 33,105	🟢 +21.7%
forEach with array destructuring	66,311 → 71,497	🟢 +7.8%	84,693 → 136,911	🟢 +61.7%
map with array destructuring	66,120 → 72,972	🟢 +10.4%	99,666 → 166,598	🟢 +67.2%
filter with array destructuring	74,899 → 74,757	-0.2%	124,318 → 194,781	🟢 +56.7%
reduce with array destructuring	78,904 → 81,429	+3.2%	112,689 → 183,560	🟢 +62.9%
map with object destructuring	77,576 → 80,325	+3.5%	56,461 → 65,912	🟢 +16.7%
map with nested destructuring	65,673 → 65,653	-0.0%	46,958 → 55,767	🟢 +18.8%
map with rest in destructuring	36,544 → 41,015	🟢 +12.2%	30,632 → 44,147	🟢 +44.1%
map with defaults in destructuring	56,165 → 60,221	🟢 +7.2%	29,053 → 36,541	🟢 +25.8%

fibonacci.js — Interp: 🟢 6, 2 unch. · avg +10.0% · Bytecode: 🟢 7, 🔴 1 · avg +56.0%

Benchmark	Interpreted	Δ	Bytecode	Δ
recursive fib(15)	301 → 357	🟢 +18.8%	618 → 1,125	🟢 +82.0%
recursive fib(20)	28 → 30	🟢 +8.3%	56 → 101	🟢 +81.7%
recursive fib(15) typed	302 → 334	🟢 +10.6%	698 → 1,446	🟢 +107.2%
recursive fib(20) typed	27 → 30	🟢 +8.3%	63 → 130	🟢 +107.3%
iterative fib(20) via reduce	10,783 → 12,835	🟢 +19.0%	16,072 → 8,019	🔴 -50.1%
iterator fib(20)	9,410 → 9,927	+5.5%	10,001 → 15,025	🟢 +50.2%
iterator fib(20) via Iterator.from + take	14,935 → 15,239	+2.0%	12,248 → 16,774	🟢 +37.0%
iterator fib(20) last value via reduce	10,841 → 11,616	🟢 +7.1%	9,151 → 12,146	🟢 +32.7%

for-of.js — Interp: 🟢 7 · avg +18.5% · Bytecode: 🟢 1, 🆕 6 · avg +27.4%

Benchmark	Interpreted	Δ	Bytecode	Δ
for...of with 10-element array	44,538 → 54,002	🟢 +21.2%	154,861	🆕
for...of with 100-element array	5,002 → 6,071	🟢 +21.4%	21,201	🆕
for...of with string (10 chars)	33,818 → 37,267	🟢 +10.2%	126,547	🆕
for...of with Set (10 elements)	47,139 → 53,153	🟢 +12.8%	154,689	🆕
for...of with Map entries (10 entries)	26,322 → 33,897	🟢 +28.8%	51,292	🆕
for...of with destructuring	39,506 → 45,056	🟢 +14.0%	85,155	🆕
for-await-of with sync array	40,708 → 49,221	🟢 +20.9%	102,860 → 131,065	🟢 +27.4%

iterators.js — Interp: 🟢 17, 3 unch. · avg +12.8% · Bytecode: 🟢 18, 2 unch. · avg +24.1%

Benchmark	Interpreted	Δ	Bytecode	Δ
Iterator.from({next}).toArray() — 20 elements	13,264 → 16,988	🟢 +28.1%	11,325 → 18,339	🟢 +61.9%
Iterator.from({next}).toArray() — 50 elements	6,132 → 6,923	🟢 +12.9%	4,965 → 7,644	🟢 +54.0%
spread pre-wrapped iterator — 20 elements	11,090 → 12,491	🟢 +12.6%	10,814 → 15,550	🟢 +43.8%
Iterator.from({next}).forEach — 50 elements	4,363 → 4,790	🟢 +9.8%	3,755 → 5,197	🟢 +38.4%
Iterator.from({next}).reduce — 50 elements	4,512 → 4,784	+6.0%	3,656 → 4,939	🟢 +35.1%
wrap array iterator	170,416 → 182,578	🟢 +7.1%	105,000 → 119,433	🟢 +13.7%
wrap plain {next()} object	10,136 → 10,944	🟢 +8.0%	10,571 → 11,914	🟢 +12.7%
map + toArray (50 elements)	4,170 → 4,892	🟢 +17.3%	4,710 → 5,321	🟢 +13.0%
filter + toArray (50 elements)	4,152 → 4,731	🟢 +14.0%	4,372 → 5,279	🟢 +20.7%
take(10) + toArray (50 element source)	25,134 → 28,700	🟢 +14.2%	24,031 → 28,808	🟢 +19.9%
drop(40) + toArray (50 element source)	6,087 → 6,827	🟢 +12.2%	6,271 → 7,228	🟢 +15.3%
chained map + filter + take (100 element source)	7,614 → 8,790	🟢 +15.4%	7,650 → 9,352	🟢 +22.2%
some + every (50 elements)	2,374 → 2,768	🟢 +16.6%	2,615 → 3,053	🟢 +16.8%
find (50 elements)	5,537 → 5,995	🟢 +8.3%	5,422 → 6,612	🟢 +21.9%
array.values().map().filter().toArray()	8,686 → 9,448	🟢 +8.8%	7,691 → 9,345	🟢 +21.5%
array.values().take(5).toArray()	211,241 → 224,488	+6.3%	144,221 → 150,290	+4.2%
array.values().drop(45).toArray()	182,392 → 208,591	🟢 +14.4%	135,140 → 142,324	+5.3%
map.entries() chained helpers	9,192 → 11,311	🟢 +23.1%	4,000 → 4,898	🟢 +22.4%
set.values() chained helpers	18,117 → 19,286	+6.5%	16,008 → 19,702	🟢 +23.1%
string iterator map + toArray	12,788 → 14,712	🟢 +15.0%	17,370 → 20,256	🟢 +16.6%

json.js — Interp: 🟢 11, 9 unch. · avg +8.0% · Bytecode: 🆕 20

Benchmark	Interpreted	Δ	Bytecode	Δ
parse simple object	154,576 → 194,826	🟢 +26.0%	154,385	🆕
parse nested object	97,428 → 117,002	🟢 +20.1%	111,349	🆕
parse array of objects	53,969 → 61,188	🟢 +13.4%	61,062	🆕
parse large flat object	45,806 → 51,843	🟢 +13.2%	50,066	🆕
parse mixed types	75,718 → 76,598	+1.2%	72,058	🆕
stringify simple object	140,738 → 155,247	🟢 +10.3%	136,565	🆕
stringify nested object	75,605 → 83,905	🟢 +11.0%	74,519	🆕
stringify array of objects	39,367 → 39,227	-0.4%	34,699	🆕
stringify mixed types	64,785 → 71,007	🟢 +9.6%	63,174	🆕
reviver doubles numbers	37,924 → 47,653	🟢 +25.7%	45,356	🆕
reviver filters properties	35,979 → 39,387	🟢 +9.5%	42,184	🆕
reviver on nested object	50,464 → 53,767	+6.5%	52,400	🆕
reviver on array	27,029 → 30,188	🟢 +11.7%	28,248	🆕
replacer function doubles numbers	43,157 → 47,161	🟢 +9.3%	47,545	🆕
replacer function excludes properties	57,489 → 60,807	+5.8%	58,249	🆕
array replacer (allowlist)	110,593 → 108,209	-2.2%	90,052	🆕
stringify with 2-space indent	82,742 → 80,977	-2.1%	68,264	🆕
stringify with tab indent	79,832 → 81,613	+2.2%	69,369	🆕
parse then stringify	51,194 → 48,460	-5.3%	44,976	🆕
stringify then parse	30,570 → 28,954	-5.3%	27,057	🆕

jsx.jsx — Interp: 🟢 14, 7 unch. · avg +9.3% · Bytecode: 🆕 21

Benchmark	Interpreted	Δ	Bytecode	Δ
simple element	220,465 → 251,015	🟢 +13.9%	670,341	🆕
self-closing element	219,187 → 238,294	🟢 +8.7%	702,375	🆕
element with string attribute	173,049 → 196,283	🟢 +13.4%	420,292	🆕
element with multiple attributes	143,804 → 164,133	🟢 +14.1%	353,940	🆕
element with expression attribute	173,211 → 177,244	+2.3%	422,220	🆕
text child	193,736 → 226,190	🟢 +16.8%	589,478	🆕
expression child	202,221 → 220,879	🟢 +9.2%	586,985	🆕
mixed text and expression	193,441 → 206,373	+6.7%	536,109	🆕
nested elements (3 levels)	77,441 → 83,274	🟢 +7.5%	243,748	🆕
sibling children	54,375 → 61,264	🟢 +12.7%	178,239	🆕
component element	149,578 → 156,538	+4.7%	420,385	🆕
component with children	84,886 → 95,847	🟢 +12.9%	261,557	🆕
dotted component	118,057 → 133,029	🟢 +12.7%	319,618	🆕
empty fragment	192,634 → 229,254	🟢 +19.0%	566,089	🆕
fragment with children	55,186 → 60,700	🟢 +10.0%	170,745	🆕
spread attributes	102,572 → 113,986	🟢 +11.1%	109,781	🆕
spread with overrides	101,034 → 101,250	+0.2%	83,055	🆕
shorthand props	164,620 → 169,410	+2.9%	405,139	🆕
nav bar structure	25,633 → 26,190	+2.2%	78,639	🆕
card component tree	30,793 → 33,314	🟢 +8.2%	86,287	🆕
10 list items via Array.from	14,512 → 15,333	+5.7%	23,511	🆕

numbers.js — Interp: 🟢 1, 10 unch. · avg +0.3% · Bytecode: 🆕 11

Benchmark	Interpreted	Δ	Bytecode	Δ
integer arithmetic	546,686 → 636,574	🟢 +16.4%	1,497,578	🆕
floating point arithmetic	591,251 → 608,637	+2.9%	1,596,006	🆕
number coercion	187,564 → 183,374	-2.2%	132,615	🆕
toFixed	110,802 → 105,586	-4.7%	205,397	🆕
toString	170,979 → 169,343	-1.0%	666,957	🆕
valueOf	247,091 → 245,075	-0.8%	921,204	🆕
toPrecision	160,630 → 155,101	-3.4%	402,503	🆕
Number.isNaN	291,963 → 295,361	+1.2%	169,276	🆕
Number.isFinite	286,438 → 290,095	+1.3%	161,190	🆕
Number.isInteger	289,738 → 282,904	-2.4%	174,771	🆕
Number.parseInt and parseFloat	253,766 → 244,868	-3.5%	150,489	🆕

objects.js — Interp: 🟢 5, 2 unch. · avg +9.7% · Bytecode: 🆕 7

Benchmark	Interpreted	Δ	Bytecode	Δ
create simple object	448,275 → 528,098	🟢 +17.8%	865,179	🆕
create nested object	215,138 → 252,401	🟢 +17.3%	371,338	🆕
create 50 objects via Array.from	9,397 → 10,266	🟢 +9.2%	8,209	🆕
property read	642,692 → 638,013	-0.7%	631,056	🆕
Object.keys	308,051 → 308,566	+0.2%	225,156	🆕
Object.entries	101,082 → 115,085	🟢 +13.9%	77,968	🆕
spread operator	185,468 → 204,524	🟢 +10.3%	197,764	🆕

promises.js — Interp: 🟢 6, 🔴 1, 5 unch. · avg +6.7% · Bytecode: 🆕 12

Benchmark	Interpreted	Δ	Bytecode	Δ
Promise.resolve(value)	529,191 → 587,224	🟢 +11.0%	375,575	🆕
new Promise(resolve => resolve(value))	200,191 → 209,689	+4.7%	182,118	🆕
Promise.reject(reason)	520,923 → 577,206	🟢 +10.8%	355,099	🆕
resolve + then (1 handler)	179,043 → 182,563	+2.0%	147,190	🆕
resolve + then chain (3 deep)	75,989 → 69,653	🔴 -8.3%	67,937	🆕
resolve + then chain (10 deep)	24,093 → 23,993	-0.4%	23,254	🆕
reject + catch + then	112,932 → 109,953	-2.6%	94,823	🆕
resolve + finally + then	78,415 → 94,583	🟢 +20.6%	83,041	🆕
Promise.all (5 resolved)	32,787 → 35,367	🟢 +7.9%	27,900	🆕
Promise.race (5 resolved)	36,937 → 37,726	+2.1%	27,495	🆕
Promise.allSettled (5 mixed)	26,490 → 30,998	🟢 +17.0%	22,159	🆕
Promise.any (5 mixed)	30,498 → 35,357	🟢 +15.9%	27,319	🆕

strings.js — Interp: 🟢 5, 6 unch. · avg +7.1% · Bytecode: 🆕 11

Benchmark	Interpreted	Δ	Bytecode	Δ
string concatenation	404,591 → 539,908	🟢 +33.4%	393,396	🆕
template literal	445,544 → 419,558	-5.8%	619,490	🆕
string repeat	441,186 → 428,736	-2.8%	922,996	🆕
split and join	132,283 → 152,348	🟢 +15.2%	265,134	🆕
indexOf and includes	175,467 → 177,822	+1.3%	538,646	🆕
toUpperCase and toLowerCase	240,251 → 267,662	🟢 +11.4%	689,841	🆕
slice and substring	175,687 → 165,170	-6.0%	610,908	🆕
trim operations	174,668 → 199,174	🟢 +14.0%	713,668	🆕
replace and replaceAll	208,962 → 218,827	+4.7%	603,033	🆕
startsWith and endsWith	127,943 → 138,917	🟢 +8.6%	468,767	🆕
padStart and padEnd	202,026 → 210,371	+4.1%	539,491	🆕

typed-arrays.js — Interp: 🟢 15, 🔴 1, 6 unch. · avg +12.1% · Bytecode: 🆕 22

Benchmark	Interpreted	Δ	Bytecode	Δ
new Int32Array(0)	277,497 → 345,613	🟢 +24.5%	125,296	🆕
new Int32Array(100)	247,175 → 321,517	🟢 +30.1%	121,780	🆕
new Int32Array(1000)	155,991 → 161,987	+3.8%	87,007	🆕
new Float64Array(100)	250,378 → 271,049	🟢 +8.3%	111,758	🆕
Int32Array.from([...])	164,306 → 186,881	🟢 +13.7%	62,214	🆕
Int32Array.of(1, 2, 3, 4, 5)	279,425 → 332,151	🟢 +18.9%	249,230	🆕
sequential write 100 elements	3,287 → 3,982	🟢 +21.1%	12,402	🆕
sequential read 100 elements	3,349 → 4,168	🟢 +24.5%	9,950	🆕
Float64Array write 100 elements	3,037 → 3,643	🟢 +19.9%	11,819	🆕
fill(42)	44,320 → 44,956	+1.4%	43,273	🆕
slice()	200,059 → 215,258	🟢 +7.6%	186,933	🆕
map(x => x * 2)	8,884 → 8,898	+0.2%	7,123	🆕
filter(x => x > 50)	8,098 → 8,962	🟢 +10.7%	7,315	🆕
reduce (sum)	7,683 → 8,447	🟢 +9.9%	6,351	🆕
sort()	165,488 → 169,376	+2.3%	153,543	🆕
indexOf()	434,571 → 445,975	+2.6%	341,757	🆕
reverse()	336,549 → 336,968	+0.1%	263,277	🆕
create view over existing buffer	345,346 → 423,781	🟢 +22.7%	131,677	🆕
subarray()	415,824 → 474,783	🟢 +14.2%	342,645	🆕
set() from array	536,639 → 471,942	🔴 -12.1%	243,238	🆕
for-of loop	5,019 → 5,724	🟢 +14.0%	18,095	🆕
spread into array	16,345 → 20,836	🟢 +27.5%	62,156	🆕

_{Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant.}

[coderabbitai]

Actionable comments posted: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (15)

units/Goccia.Builtins.TestAssertions.pas (1)

1072-1104: ⚠️ Potential issue | 🟠 Major

The returned expectation still doesn't keep the unwrapped value alive.

The temp root only covers DrainQueue. After returning TGocciaExpectationValue.Create(Promise.PromiseResult, ...), the resolved/rejected value is referenced only from FActualValue, but TGocciaExpectationValue does not mark that field. A later collection can reclaim it before the chained matcher runs.

Suggested fix

   TGocciaExpectationValue = class(TGocciaObjectValue)
@@
     function GetResolves(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue;
     function GetRejects(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue;
+    procedure MarkReferences; override;
   end;

+procedure TGocciaExpectationValue.MarkReferences;
+begin
+  if GCMarked then Exit;
+  inherited;
+  if Assigned(FActualValue) then
+    FActualValue.MarkReferences;
+end;
+
 function TGocciaExpectationValue.GetResolves(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue;

As per coding guidelines: `Each value type must override MarkReferences to mark all TGocciaValue references it holds (prototype, closure, elements, property values, etc.).`

Also applies to: 1112-1144

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Builtins.TestAssertions.pas` around lines 1072 - 1104, The
returned TGocciaExpectationValue is not keeping the unwrapped Promise result
alive; instead of relying on a temp root around DrainQueue, implement/override
MarkReferences in TGocciaExpectationValue to mark the internal TGocciaValue
fields (the stored unwrapped value passed to TGocciaExpectationValue.Create,
plus any other TGocciaValue fields like FTestAssertions or prototype/closure
fields) so the GC will not reclaim the wrapped value before chained matchers
run; update TGocciaExpectationValue.MarkReferences to call Mark on that stored
value (and call inherited where appropriate) rather than trying to extend the
temp-root scope in the resolves code path.

units/Goccia.Evaluator.pas (1)

243-252: ⚠️ Potential issue | 🟠 Major

Guard these TGarbageCollector.Instance calls consistently.

The rest of this file still treats Instance as nullable, but these paths dereference it unconditionally. If evaluation can happen before GC initialization or during shutdown, they now crash instead of failing as JS code.

Also applies to: 742-761, 1425-1447, 1930-2240, 2521-2561, 3022-3056

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Evaluator.pas` around lines 243 - 252, The code unconditionally
dereferences TGarbageCollector.Instance (calling AddTempRoot/RemoveTempRoot)
which can be nil; change to grab a local variable (e.g. gc :=
TGarbageCollector.Instance) and call gc.AddTempRoot(Iterator) only if
Assigned(gc), then in the finally block call gc.RemoveTempRoot(Iterator) only if
Assigned(gc) so you don't crash when GC is uninitialized or during shutdown;
apply the same Assigned-guard pattern to all other occurrences mentioned (around
lines handling Iterator/AdvanceNext and the other ranges) so
AddTempRoot/RemoveTempRoot are always paired only when the GC was actually
added.

units/Goccia.MicrotaskQueue.pas (1)

149-161: ⚠️ Potential issue | 🟠 Major

Release the queue-owned temp root after a task is consumed.

queueMicrotask roots the callback before enqueueing it, but DrainQueue only removes the extra root it adds for execution. After FQueue.Clear, the original queue-owned root is still live, so processed callbacks never become collectible.

Based on learnings, values held only by Pascal code (not in any GocciaScript scope) must be protected with AddTempRoot/RemoveTempRoot for the duration they are needed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.MicrotaskQueue.pas` around lines 149 - 161, DrainQueue currently
removes only the extra execution roots but leaves the original queue-owned temp
root alive after FQueue.Clear, preventing collected callbacks from being freed;
update DrainQueue so that after clearing FQueue it also calls
TGarbageCollector.Instance.RemoveTempRoot for the temp root that was added by
queueMicrotask (the queue-owned root), ensuring any
Task.Handler/Task.Value/Promise roots held solely by the Pascal queue are
released; reference queueMicrotask (where the AddTempRoot is applied),
DrainQueue, FQueue.Clear and TGarbageCollector.Instance.RemoveTempRoot when
making the change.

units/Goccia.Values.TemporalPlainDate.pas (1)

452-461: ⚠️ Potential issue | 🟠 Major

Guard TGarbageCollector.Instance here.

This is now an unguarded singleton dereference in a codebase that otherwise treats Instance as nullable. If DateSubtract runs before GC initialization or during teardown, this path will raise an access violation.

Suggested fix

 function TGocciaTemporalPlainDateValue.DateSubtract(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue;
 var
   D: TGocciaTemporalPlainDateValue;
   Dur: TGocciaTemporalDurationValue;
   Arg: TGocciaValue;
   ObjArg: TGocciaObjectValue;
   DurRec: TTemporalDurationRecord;
   NegatedDur: TGocciaTemporalDurationValue;
   NewArgs: TGocciaArgumentsCollection;
+  GarbageCollector: TGarbageCollector;
 begin
+  GarbageCollector := TGarbageCollector.Instance;
   D := AsPlainDate(AThisValue, 'PlainDate.prototype.subtract');
   Arg := AArgs.GetElement(0);
@@
-  TGarbageCollector.Instance.AddTempRoot(NegatedDur);
+  if Assigned(GarbageCollector) then
+    GarbageCollector.AddTempRoot(NegatedDur);
   try
     NewArgs := TGocciaArgumentsCollection.Create([NegatedDur]);
     try
       Result := DateAdd(NewArgs, AThisValue);
     finally
       NewArgs.Free;
     end;
   finally
-    TGarbageCollector.Instance.RemoveTempRoot(NegatedDur);
+    if Assigned(GarbageCollector) then
+      GarbageCollector.RemoveTempRoot(NegatedDur);
   end;
 end;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Values.TemporalPlainDate.pas` around lines 452 - 461, The
TGarbageCollector.Instance dereference is unguarded (in the DateSubtract/DateAdd
path using NegatedDur), so wrap the AddTempRoot/RemoveTempRoot calls with a
nil-check: test Assigned(TGarbageCollector.Instance) before calling AddTempRoot
and record a boolean (e.g., GCAdded) to indicate you added the temp root, then
in the finally only call RemoveTempRoot if GCAdded is true; keep the NewArgs
creation and DateAdd call intact (refer to TGarbageCollector.Instance,
AddTempRoot, RemoveTempRoot, NewArgs and DateAdd) so you avoid an access
violation when Instance is nil during init/teardown.

units/Goccia.Builtins.GlobalArray.pas (1)

245-267: ⚠️ Potential issue | 🟠 Major

Temp-root ResultObj in every Array.from construction path.

Lines 245-267 only protect the iterator branch. The array, string, and array-like branches still keep ResultObj in a Pascal local while mapping callbacks or per-element allocations run; the string fast path is especially exposed because it allocates a new TGocciaStringLiteralValue before storing it. Non-iterator inputs can still lose the partially-built result under the unified collector.

As per coding guidelines, "Values held only by Pascal code (not in any GocciaScript scope) must be protected with AddTempRoot/RemoveTempRoot for the duration they are needed."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Builtins.GlobalArray.pas` around lines 245 - 267, The
partially-built ResultObj is only temp-rooted in the iterator branch; protect
ResultObj in all Array.from code paths (string fast path, array-like, and
standard array branches) by calling
TGarbageCollector.Instance.AddTempRoot(ResultObj) immediately after creating it
and ensuring a matching RemoveTempRoot(ResultObj) in the enclosing finally block
so mapping callbacks (MapCallback/MapArgs), per-element allocations (e.g.,
TGocciaStringLiteralValue) and CreateDataProperty calls cannot be collected;
mirror the iterator branch's temp-root placement and removal around any code
that invokes InvokeCallable or allocates element values.

units/Goccia.Builtins.Benchmark.pas (2)

432-493: ⚠️ Potential issue | 🟠 Major

SingleResult and the final ResultObj still need temp roots while they are being populated.

Rooting ResultsArray is not enough. Each TGocciaObjectValue.Create in Lines 445 and 476 lives only in a Pascal local until SetElement / Result := runs, and the AssignProperty(...Create(...)) calls in between can allocate enough to sweep those objects first.

As per coding guidelines, "Values held only by Pascal code (not in any GocciaScript scope) must be protected with AddTempRoot/RemoveTempRoot for the duration they are needed."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Builtins.Benchmark.pas` around lines 432 - 493, The code roots
ResultsArray but not the transient TGocciaObjectValue instances (SingleResult
and ResultObj) created while populating it, risking GC during AssignProperty
calls; after each TGocciaObjectValue.Create for SingleResult (the ones created
in the try and except blocks) call
TGarbageCollector.Instance.AddTempRoot(SingleResult) before doing
AssignProperty/SetElement and then call
TGarbageCollector.Instance.RemoveTempRoot(SingleResult) immediately after
ResultsArray.SetElement; likewise, after creating ResultObj call
AddTempRoot(ResultObj) before AssignProperty calls and RemoveTempRoot(ResultObj)
just before returning Result (and ensure all Add/Remove use
TGarbageCollector.Instance and mirror each other in the finally block).

---

280-295: ⚠️ Potential issue | 🟠 Major

Root SetupResult before DrainQueue.

Line 289 returns a value that only lives in SetupResult, but Line 290 can drain arbitrary microtasks before Line 295 roots it. That leaves setup payloads collectible before warmup/calibration captures them.

Suggested fix

       StartNanoseconds := GetNanoseconds;
       SetupResult := ABenchCase.SetupFunction.Call(EmptyArgs, TGocciaUndefinedLiteralValue.UndefinedValue);
+      if Assigned(SetupResult) and Assigned(GC) then
+        GC.AddTempRoot(SetupResult);
       if Assigned(TGocciaMicrotaskQueue.Instance) then
         TGocciaMicrotaskQueue.Instance.DrainQueue;
       Result.SetupMs := (GetNanoseconds - StartNanoseconds) / 1000000;
-
-      if Assigned(SetupResult) and Assigned(GC) then
-        GC.AddTempRoot(SetupResult);

As per coding guidelines, "Values held only by Pascal code (not in any GocciaScript scope) must be protected with AddTempRoot/RemoveTempRoot for the duration they are needed."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Builtins.Benchmark.pas` around lines 280 - 295, The SetupResult
returned by ABenchCase.SetupFunction.Call can be collected during
TGocciaMicrotaskQueue.Instance.DrainQueue because it isn’t rooted yet;
immediately after calling ABenchCase.SetupFunction.Call store and protect the
returned value by calling GC.AddTempRoot(SetupResult) (using GC :=
TGarbageCollector.Instance) before invoking
TGocciaMicrotaskQueue.Instance.DrainQueue, and keep that temp root until after
warmup/calibration when it is explicitly removed with RemoveTempRoot; reference
SetupResult, ABenchCase.SetupFunction.Call,
TGocciaMicrotaskQueue.Instance.DrainQueue, and TGarbageCollector.AddTempRoot to
locate and adjust the code.

units/Goccia.Engine.pas (1)

252-273: ⚠️ Potential issue | 🟠 Major

Pin the shared null singleton too.

PinSingletons now routes through PinObject, but it still never pins TGocciaNullLiteralValue.NullValue. Under the unified collector that leaves one of the language singletons collectible even though the runtime treats it as immortal.

Suggested fix

   PinIfAssigned(TGocciaUndefinedLiteralValue.UndefinedValue);
+  PinIfAssigned(TGocciaNullLiteralValue.NullValue);
   PinIfAssigned(TGocciaBooleanLiteralValue.TrueValue);

As per coding guidelines, "Singleton values (undefined, null, true, false, NaN, Infinity) are pinned via TGocciaGarbageCollector.Instance.PinValue during engine initialization in PinSingletons."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Engine.pas` around lines 252 - 273, Add pinning for the null
singleton in TGocciaEngine.PinSingletons: locate the PinIfAssigned call sequence
in TGocciaEngine.PinSingletons and add a call for
TGocciaNullLiteralValue.NullValue (e.g.,
PinIfAssigned(TGocciaNullLiteralValue.NullValue)) so the null singleton is
pinned by the garbage collector just like UndefinedValue, TrueValue, FalseValue,
etc.; use the existing PinIfAssigned helper (which delegates to
TGarbageCollector.Instance.PinObject) to keep the pattern consistent.

units/Goccia.Values.ClassValue.pas (1)

747-757: ⚠️ Potential issue | 🟠 Major

Keep Instance temp-rooted through the native-init path too.

Right now the temp root only covers ConstructorToCall.Call. Built-ins that skip a user constructor and go through InitializeNativeFromArguments at Lines 756-757 leave the freshly created object reachable only from Pascal locals during initialization, which is exactly the lifetime window the temp-root rule is meant to protect.

Proposed fix

-  if Assigned(ConstructorToCall) then
-  begin
-    TGarbageCollector.Instance.AddTempRoot(Instance);
-    try
-      ConstructorToCall.Call(AArguments, Instance);
-    finally
-      TGarbageCollector.Instance.RemoveTempRoot(Instance);
-    end;
-  end
-  else if Assigned(NativeInstance) and (NativeInstance is TGocciaInstanceValue) then
-    TGocciaInstanceValue(NativeInstance).InitializeNativeFromArguments(AArguments);
-
-  Result := Instance;
+  if Assigned(TGarbageCollector.Instance) then
+    TGarbageCollector.Instance.AddTempRoot(Instance);
+  try
+    if Assigned(ConstructorToCall) then
+      ConstructorToCall.Call(AArguments, Instance)
+    else if Assigned(NativeInstance) and (NativeInstance is TGocciaInstanceValue) then
+      TGocciaInstanceValue(NativeInstance).InitializeNativeFromArguments(AArguments);
+
+    Result := Instance;
+  finally
+    if Assigned(TGarbageCollector.Instance) then
+      TGarbageCollector.Instance.RemoveTempRoot(Instance);
+  end;

As per coding guidelines: Values held only by Pascal code (not in any GocciaScript scope) must be protected with AddTempRoot/RemoveTempRoot for the duration they are needed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Values.ClassValue.pas` around lines 747 - 757, The newly created
object Instance is temp-rooted only when ConstructorToCall is present but not
when the native-init path is taken; wrap the call to
TGocciaInstanceValue(NativeInstance).InitializeNativeFromArguments(AArguments)
with TGarbageCollector.Instance.AddTempRoot(Instance) before the call and
TGarbageCollector.Instance.RemoveTempRoot(Instance) in a finally block so
Instance is protected during InitializeNativeFromArguments; use the same
AddTempRoot/RemoveTempRoot pattern as around ConstructorToCall.Call and refer to
ConstructorToCall, NativeInstance,
TGocciaInstanceValue.InitializeNativeFromArguments and
TGarbageCollector.Instance.AddTempRoot/RemoveTempRoot.

TestRunner.dpr (1)

337-401: ⚠️ Potential issue | 🟠 Major

Use finally for both temp-root cleanup and per-file collection.

This block currently has two GC-lifetime holes: an exception between AddTempRoot and RemoveTempRoot leaks permanent roots, and the Continue on Line 369 skips the new per-file GC.Collect.

Suggested fix

   if Assigned(GC) then
   begin
     GC.AddTempRoot(AllTestResults);
     GC.AddTempRoot(AllFailedTests);
   end;
-
-  AllTestResults.AssignProperty('totalTests', TGocciaNumberLiteralValue.ZeroValue);
-  AllTestResults.AssignProperty('totalRunTests', TGocciaNumberLiteralValue.ZeroValue);
-  AllTestResults.AssignProperty('passed', TGocciaNumberLiteralValue.ZeroValue);
-  AllTestResults.AssignProperty('failed', TGocciaNumberLiteralValue.ZeroValue);
-  AllTestResults.AssignProperty('skipped', TGocciaNumberLiteralValue.ZeroValue);
-  AllTestResults.AssignProperty('assertions', TGocciaNumberLiteralValue.ZeroValue);
-  AllTestResults.AssignProperty('duration', TGocciaNumberLiteralValue.ZeroValue);
-  AllTestResults.AssignProperty('failedTests', AllFailedTests);
+  try
+    AllTestResults.AssignProperty('totalTests', TGocciaNumberLiteralValue.ZeroValue);
+    AllTestResults.AssignProperty('totalRunTests', TGocciaNumberLiteralValue.ZeroValue);
+    AllTestResults.AssignProperty('passed', TGocciaNumberLiteralValue.ZeroValue);
+    AllTestResults.AssignProperty('failed', TGocciaNumberLiteralValue.ZeroValue);
+    AllTestResults.AssignProperty('skipped', TGocciaNumberLiteralValue.ZeroValue);
+    AllTestResults.AssignProperty('assertions', TGocciaNumberLiteralValue.ZeroValue);
+    AllTestResults.AssignProperty('duration', TGocciaNumberLiteralValue.ZeroValue);
+    AllTestResults.AssignProperty('failedTests', AllFailedTests);

-  for I := 0 to AFiles.Count - 1 do
-  begin
-    ...
-    FileResult := RunScriptFromFile(AFiles[I]);
-    if FileResult.TestResult = nil then
-      Continue;
-    ...
-    if Assigned(GC) then
-      GC.Collect;
-  end;
+    for I := 0 to AFiles.Count - 1 do
+    begin
+      try
+        ...
+        FileResult := RunScriptFromFile(AFiles[I]);
+        if FileResult.TestResult = nil then
+          Continue;
+        ...
+      finally
+        if Assigned(GC) then
+          GC.Collect;
+      end;
+    end;

-  if Assigned(GC) then
-  begin
-    GC.RemoveTempRoot(AllTestResults);
-    GC.RemoveTempRoot(AllFailedTests);
-  end;
+  finally
+    if Assigned(GC) then
+    begin
+      GC.RemoveTempRoot(AllTestResults);
+      GC.RemoveTempRoot(AllFailedTests);
+    end;
+  end;

As per coding guidelines, values held only by Pascal code (not in any GocciaScript scope) must be protected with AddTempRoot/RemoveTempRoot for the duration they are needed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@TestRunner.dpr` around lines 337 - 401, Wrap the lifetime management of
AllTestResults and AllFailedTests in a try..finally so GC.RemoveTempRoot is
always called (move
GC.AddTempRoot(AllTestResults)/GC.AddTempRoot(AllFailedTests) before a try and
call GC.RemoveTempRoot(...) in the finally), and ensure the per-file GC.Collect
call in the loop cannot be skipped by the Continue (either call GC.Collect
before using Continue or restructure the per-file processing so GC.Collect is
executed in a small try..finally around RunScriptFromFile/FileResult handling);
update the block referencing AllTestResults, AllFailedTests,
GC.AddTempRoot/GC.RemoveTempRoot, the for I := 0 to AFiles.Count - 1 loop,
RunScriptFromFile and the existing Continue so temp roots are always removed and
per-file GC.Collect always runs.

units/Goccia.Values.IteratorValue.pas (1)

284-295: ⚠️ Potential issue | 🔴 Critical

Root the callback inputs too, not just the iterator/accumulator.

These loops now protect Iterator/Accumulator, but the current Value (and, for the same reason, Callback) is still only held in Pascal locals when you re-enter user code via InvokeIteratorCallback(...) / Call(...). A GC triggered inside that callback can collect the current element out from under the native helper.

Suggested pattern

   Callback := AArgs.GetElement(0);
+  TGarbageCollector.Instance.AddTempRoot(Callback);
   Index := 0;

   TGarbageCollector.Instance.AddTempRoot(Iterator);
   try
     Value := Iterator.DirectNext(Done);
     while not Done do
     begin
-      InvokeIteratorCallback(Callback, Value, Index);
+      TGarbageCollector.Instance.AddTempRoot(Value);
+      try
+        InvokeIteratorCallback(Callback, Value, Index);
+      finally
+        TGarbageCollector.Instance.RemoveTempRoot(Value);
+      end;
       Inc(Index);
       Value := Iterator.DirectNext(Done);
     end;
   finally
     TGarbageCollector.Instance.RemoveTempRoot(Iterator);
+    TGarbageCollector.Instance.RemoveTempRoot(Callback);
   end;

Apply the same pattern anywhere this unit passes a locally-held TGocciaValue back into user code (reduce, some, every, find).

Based on learnings: Values held only by Pascal code (not in any GocciaScript scope) must be protected with `AddTempRoot`/`RemoveTempRoot` for the duration they are needed

Also applies to: 317-350, 400-488

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Values.IteratorValue.pas` around lines 284 - 295, The loop roots
the Iterator but not the TGocciaValue instances passed into user code or the
Callback reference, so ensure any Value and Callback that are passed to
InvokeIteratorCallback (and similar methods like reduce/some/every/find) are
protected: call TGarbageCollector.Instance.AddTempRoot(Value) and
AddTempRoot(Callback) before InvokeIteratorCallback/Call and remove them in a
matching finally via RemoveTempRoot(Value) and RemoveTempRoot(Callback); mirror
this pattern around every place in this unit where a locally-held TGocciaValue
is handed back to user code (e.g., the sections noted around reduce, some,
every, find and the other ranges mentioned).

ScriptLoader.dpr (1)

205-217: ⚠️ Potential issue | 🔴 Critical

Per-file GC shutdown breaks prototype singleton cache—object pointers will be freed but reused in next cycle.

EmitBytecode and EmitWasm now initialize and tear down the garbage collector after each file. However, class-var prototype caches (e.g., FSharedArrayPrototype, FSharedMapPrototype, FPrototypeMethodHost) are never reset to nil between cycles.

On first file, InitializePrototype assigns FSharedXxxPrototype := TGocciaObjectValue.Create; and the new object is pinned in GC1. After shutdown, GC1 and all its pinned objects are freed.

On second file, a new GC2 is created, but the class var still holds the freed pointer. InitializePrototype has an early exit (if Assigned(FSharedArrayPrototype) then Exit;), so it never rebuilds—the method returns a dead object reference or the pointer is used in a different GC context, causing corruption or crash.

Either reset all class-var caches to nil in TGarbageCollector.Shutdown, or move GC lifetime outside the per-file loop to a single cycle per process.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ScriptLoader.dpr` around lines 205 - 217, The per-file GC shutdown leaves
class-var prototype caches pointing at freed objects; update
TGarbageCollector.Shutdown to nil-out the prototype/cache class variables (e.g.,
FSharedArrayPrototype, FSharedMapPrototype, FPrototypeMethodHost and any other
FShared* prototypes) so InitializePrototype’s guarded early-exit (if
Assigned(...)) will rebuild prototypes under the new GC, or alternatively move
GC lifetime out of EmitBytecode/EmitWasm to a single process-wide
initialization; pick the nil-reset approach and ensure Shutdown clears all
cached class vars before freeing GC state.

units/Goccia.Runtime.Operations.pas (2)

7925-7975: ⚠️ Potential issue | 🟠 Major

Release EnumObj once the wrapper owns it.

EnumObj is temp-rooted before WrapGocciaValue, but never unrooted afterward. Every finalized enum will stay pinned in the temp-root set even when nothing in JS can reach it anymore.

Suggested fix

-    if Assigned(TGarbageCollector.Instance) then
-      TGarbageCollector.Instance.AddTempRoot(EnumObj);
-
-    Result := WrapGocciaValue(EnumObj);
+    if Assigned(TGarbageCollector.Instance) then
+      TGarbageCollector.Instance.AddTempRoot(EnumObj);
+    try
+      Result := WrapGocciaValue(EnumObj);
+    finally
+      if Assigned(TGarbageCollector.Instance) then
+        TGarbageCollector.Instance.RemoveTempRoot(EnumObj);
+    end;

As per coding guidelines: Values held only by Pascal code (not in any GocciaScript scope) must be protected with `AddTempRoot`/`RemoveTempRoot` for the duration they are needed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Runtime.Operations.pas` around lines 7925 - 7975, The function
FinalizeEnum adds EnumObj to the GC temp roots via
TGarbageCollector.Instance.AddTempRoot but never removes it, leaking temp roots;
after successfully wrapping EnumObj with WrapGocciaValue (result of
WrapGocciaValue(EnumObj) assigned to Result) call
TGarbageCollector.Instance.RemoveTempRoot(EnumObj) (or remove it in a finally
block after wrapping) so the temporary root is released once ownership is
transferred to the wrapper; ensure you still RemoveTempRoot on all success paths
and do not remove it on exception paths where ownership wasn't transferred.

---

3585-3721: ⚠️ Potential issue | 🟠 Major

Balance the iterator temp-roots before returning.

The new AddTempRoot calls in this method never get paired with RemoveTempRoot after WrapGocciaValue takes ownership. That pins every bridged iterator for the lifetime of the runtime and will accumulate across long benchmark/test runs.

Suggested fix

function TGocciaRuntimeOperations.GetIterator(
  const AIterable: TSouffleValue;
  const ATryAsync: Boolean): TSouffleValue;
var
  GocciaVal: TGocciaValue;
  Iterator: TGocciaIteratorValue;
  IteratorMethod, IteratorObj, NextMethod: TGocciaValue;
  CallArgs: TGocciaArgumentsCollection;
+  GC: TGarbageCollector;
begin
+  GC := TGarbageCollector.Instance;
  {$IFDEF BRIDGE_METRICS}
  Inc(GBridgeMetrics.GetIteratorCount);
  {$ENDIF}
  try
    ...
        if GocciaVal is TGocciaMapValue then
        begin
          Iterator := TGocciaMapIteratorValue.Create(GocciaVal, mkEntries);
-          if Assigned(TGarbageCollector.Instance) then
-            TGarbageCollector.Instance.AddTempRoot(Iterator);
-          Result := WrapGocciaValue(Iterator);
+          if Assigned(GC) then
+            GC.AddTempRoot(Iterator);
+          try
+            Result := WrapGocciaValue(Iterator);
+          finally
+            if Assigned(GC) then
+              GC.RemoveTempRoot(Iterator);
+          end;
           Exit;
        end;
    ...
-    if Assigned(TGarbageCollector.Instance) then
-      TGarbageCollector.Instance.AddTempRoot(Iterator);
-    Result := WrapGocciaValue(Iterator);
+    if Assigned(GC) then
+      GC.AddTempRoot(Iterator);
+    try
+      Result := WrapGocciaValue(Iterator);
+    finally
+      if Assigned(GC) then
+        GC.RemoveTempRoot(Iterator);
+    end;

Apply the same pattern to the IteratorObj async path as well.
As per coding guidelines: Values held only by Pascal code (not in any GocciaScript scope) must be protected with AddTempRoot/RemoveTempRoot for the duration they are needed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Runtime.Operations.pas` around lines 3585 - 3721, GetIterator is
adding temporary GC roots (TGarbageCollector.Instance.AddTempRoot) for Iterator
and IteratorObj but never removing them after ownership transfers via
WrapGocciaValue, leaking pinned iterators; fix by pairing each AddTempRoot with
a RemoveTempRoot immediately after WrapGocciaValue (or use a try/finally that
calls RemoveTempRoot in the finally) so the temp root is cleared once Result is
produced, and apply the same pattern in both the async IteratorObj path and the
synchronous Iterator paths where AddTempRoot is called; reference
TGocciaRuntimeOperations.GetIterator, WrapGocciaValue, AddTempRoot and
RemoveTempRoot, and the local variables Iterator and IteratorObj to locate the
spots to update.

docs/souffle-vm.md (1)

695-705: ⚠️ Potential issue | 🟡 Minor

Update the remaining split-GC references in this doc.

This section now says Souffle and the interpreter share one collector, but Line 551 still says “Souffle GC” and Lines 942-966 still describe “Two GC systems” / “dual GC tracking.” Please bring those passages in line with the new model so contributors do not get contradictory guidance.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/souffle-vm.md` around lines 695 - 705, Update the outdated split-GC
wording to reflect the unified collector TGarbageCollector.Instance: replace any
“Souffle GC” and any “Two GC systems” / “dual GC tracking” descriptions with the
single-shared GC model, remove references to separate interpreter/VM collectors,
and clarify that MarkVMRoots and MarkExternalRoots are the external-root hooks
used to cover VM registers, call stack and bridge call paths; also update the
CollectIfNeeded/threshold note to state it’s currently disabled until
MarkExternalRoots covers all bridge paths and ensure references to
TSouffleHeapObject, PinObject/UnpinObject, AddTempRoot/RemoveTempRoot and O(1)
membership remain consistent with the unified singleton design.

🧹 Nitpick comments (3)

docs/adding-built-in-types.md (1)

75-82: Keep the starter template free of direct GC dependencies.

This snippet never calls TGarbageCollector, so adding GarbageCollector.Generic here teaches contributors to copy an unnecessary dependency into every new value unit. If a type later needs temp roots or explicit GC calls, that unit can be added then.

Based on learnings, TGocciaSharedPrototype.Create automatically pins both prototype and method host.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/adding-built-in-types.md` around lines 75 - 82, The uses clause
incorrectly includes GarbageCollector.Generic even though this unit does not
call TGarbageCollector; remove GarbageCollector.Generic from the uses list
(alongside the existing Goccia.* and Goccia.Values.* units) so the starter
template has no direct GC dependency, and do not add any TGarbageCollector
calls—rely on TGocciaSharedPrototype.Create which already pins the prototype and
method host.

units/GarbageCollector.Managed.pas (1)

8-13: Rename this base type before it spreads further.

TGCManagedObject becomes part of the new core GC surface, so the GC abbreviation is worth fixing now rather than after more units inherit from it.

As per coding guidelines, use full words in class, function, method, and type names instead of abbreviations (e.g., TGocciaGarbageCollector not TGocciaGC).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/GarbageCollector.Managed.pas` around lines 8 - 13, Rename the
abbreviated base type and its members to use full words: change class
TGCManagedObject to TGarbageCollectedObject, rename the private field FGCMarked
to FGarbageCollectedMarked, and rename the property GCMarked to
GarbageCollectedMarked (keep MarkReferences as-is and preserve virtual). Update
all references/usages, typecasts and unit/interface declarations that mention
TGCManagedObject, FGCMarked or GCMarked throughout the codebase to the new
identifiers so compilation and behavior remain identical.

docs/souffle-vm.md (1)

704-704: The membership type is now too narrow for the unified collector.

TDictionary<TSouffleHeapObject, Boolean> still reads like a Souffle-only implementation detail. With the generic collector managing a shared managed-object base, this should describe the generic managed type instead of TSouffleHeapObject.

✏️ Suggested doc tweak

-- **O(1) membership** — Pinned objects and temp roots use `TDictionary<TSouffleHeapObject, Boolean>` for hash-set semantics
+- **O(1) membership** — Pinned objects and temp roots use a dictionary-backed set keyed by the generic managed-object base for O(1) membership checks

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/souffle-vm.md` at line 704, Update the documentation phrase that
currently refers to the Souffle-specific type TDictionary<TSouffleHeapObject,
Boolean> so it describes the generic managed-object membership type used by the
unified collector; replace the Souffle-specific symbol with a generic descriptor
(e.g., TDictionary<TManagedObject, Boolean> or "dictionary of managed objects")
and mention that pinned objects and temp roots use this generic managed-object
membership structure to provide O(1) membership semantics for the unified
collector.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@AGENTS.md`:
- Line 159: Update the AGENTS.md table row for the GocciaScript Backend
(TGocciaSouffleBackend) to remove the outdated lifecycle claims: delete the
phrases "collects on destruction" and "shuts down in finalization" and instead
state the current behavior (automatic GC collection is disabled and
TGocciaSouffleBackend does not perform explicit collection or shutdown). Make
the wording concise and accurate so the row now reads that the backend
initializes TGarbageCollector with automatic collection disabled and leaves
collection/shutdown to external management.

In `@BenchmarkRunner.dpr`:
- Around line 184-186: Move the per-file garbage collection into a finally block
so it runs even if the file benchmark raises an exception: in both
CollectBenchmarkFileInterpreted and CollectBenchmarkFileBytecode wrap the
existing body that calls PopulateFileResult(FileResult, ScriptResult, AReporter,
Result) (and any subsequent success-only logic) in a try..finally and place the
check and call to TGarbageCollector.Instance.Collect inside the finally (i.e. if
Assigned(TGarbageCollector.Instance) then TGarbageCollector.Instance.Collect),
applying the same change for the other occurrence noted.

In `@docs/code-style.md`:
- Around line 417-418: The example shows manual calls to
TGarbageCollector.Instance.PinObject for FSharedPrototype and
FPrototypeMethodHost which are redundant and encourage double-pinning; remove
those two PinObject lines from the example and rely on
TGocciaSharedPrototype.Create (which already pins the prototype and method host)
so the example demonstrates correct single-pinning behavior for
TGocciaSharedPrototype.Create, avoiding references to
TGarbageCollector.Instance.PinObject for FSharedPrototype and
FPrototypeMethodHost.

In `@docs/souffle-vm.md`:
- Line 1010: The docs contain a contradiction: the section intro states "all
Souffle VM source files live in `souffle/` and use `Souffle.` prefix naming" but
the table includes `GarbageCollector.Generic.pas` which doesn't follow that
convention; fix by either moving the `GarbageCollector.Generic.pas` row into a
new or existing shared-infrastructure subsection (e.g., "Shared/Interpreter
Infrastructure") or adjust the section intro to say that most files follow the
`souffle/` and `Souffle.` convention while some shared components like
`GarbageCollector.Generic.pas` live outside that namespace; update the table row
or the intro text accordingly so both match.

In `@docs/testing.md`:
- Around line 527-529: The documentation claims GC.Collect runs "after each
file", but TestRunner.dpr skips calling GC.Collect on the single-file execution
path; either update docs to state GC.Collect runs only when multiple files are
executed or modify TestRunner.dpr to invoke GC.Collect on the single-file branch
as well. Locate the single-file execution branch in TestRunner.dpr (the code
path that handles running a lone test file) and add the same explicit GC.Collect
call used in the multi-file loop, or alternatively update docs/testing.md to
clarify that GC.Collect only runs between files when running multiple files.

In `@docs/value-system.md`:
- Around line 118-120: The documented sample should be updated to reflect the
unified GC by showing that FGCMarked and AfterConstruction now live on
TGCManagedObject (not on TGocciaValue) and that classes like TGocciaValue
inherit from TGCManagedObject rather than TInterfacedObject; change the sample
to declare TGCManagedObject with FGCMarked and an overridden AfterConstruction
that registers with TGarbageCollector.Instance, then show TGocciaValue
inheriting from TGCManagedObject and keep the descriptions of MarkReferences and
RuntimeCopy and examples (TGocciaObjectValue, TGocciaFunctionValue,
TGocciaArrayValue) unchanged so the prose and code sample no longer contradict
each other.

In `@souffle/Souffle.VM.pas`:
- Around line 96-104: Do not cache the GC singleton in FGC because it can be
freed before TSouffleVM.Destroy; instead, call TGarbageCollector.Instance at use
sites and guard that result. Replace uses of the cached FGC in the
constructor/initializer and in TSouffleVM.Destroy with a local GC :=
TGarbageCollector.Instance (or similar direct Instance call) and only call
AddExternalRootMarker/RemoveExternalRootMarker(MarkVMRoots) when that local GC
is Assigned, ensuring you never dereference a potentially dangling FGC field.

In `@units/GarbageCollector.Generic.pas`:
- Around line 140-144: TGarbageCollector.UnregisterObject currently only removes
AObject from FManagedObjects; update TGarbageCollector.UnregisterObject to also
remove any references to AObject from FPinnedObjects, FTempRoots, FRootObjects
and any entries in FActiveRootStack so no dangling pointers remain. Locate the
UnregisterObject method and for each container (FPinnedObjects, FTempRoots,
FRootObjects) call the appropriate Remove/Contains+Delete routine to remove
AObject, and for FActiveRootStack iterate its entries and remove or null-out
entries equal to AObject (or pop/update the stack) so active-root
scanning/MarkRoots will not dereference a freed object. Ensure you use the
existing collection APIs and equality semantics used elsewhere in the class
(e.g., same Remove/IndexOf pattern as FManagedObjects) to keep behavior
consistent.
- Around line 115-123: The destructor TGarbageCollector.Destroy currently frees
tracking containers (FManagedObjects, FPinnedObjects, FTempRoots, FRootObjects,
FActiveRootStack) but because FManagedObjects was created with OwnsObjects =
False it leaves the managed instances allocated; before freeing FManagedObjects
iterate its entries and free/release each managed object (the actual
deallocation method used elsewhere in this unit — e.g., Obj.Free or
Obj.Release/Dispose — consistent with how managed objects are normally
destroyed), then clear the list and continue to free
FPinnedObjects/FTempRoots/FRootObjects/FActiveRootStack; ensure the loop is
nil-safe and wrapped in try..finally so container cleanup always runs even if a
free raises.

In `@units/Goccia.Runtime.Operations.pas`:
- Around line 8689-8691: The current loop marks keys/values from
FArrayBridgeReverse, effectively making the reverse cache a permanent root;
instead, either prune/clear FArrayBridgeReverse whenever FArrayBridgeCache is
cleared or stop rooting entries from the reverse cache here. Update the code in
TGocciaRuntimeOperations to (a) remove the MarkReferences calls over
FArrayBridgeReverse (do not treat BridgeKey or its value as GC roots), or (b)
add logic to prune matching entries from FArrayBridgeReverse whenever
FArrayBridgeCache is cleared/evicted so the reverse map does not hold immortal
references; apply the same change at the other occurrence (around lines
8745–8747) and reference FArrayBridgeCache, FArrayBridgeReverse, BridgeKey and
TSouffleHeapObject.MarkReferences when making the change.
- Around line 1934-1937: The GC-managed helper objects TGocciaBridgedFunction
and TGocciaSuperCallHelper must override MarkReferences to mark their wrapped
TGocciaValue fields (FGocciaFn and FSuperClass) so the unified GC doesn't
reclaim those Goccia values; add/implement MarkReferences in
TGocciaBridgedFunction.MarkReferences and TGocciaSuperCallHelper.MarkReferences
that calls inherited and then invokes the garbage‑collector's marking routine on
FGocciaFn / FSuperClass (e.g., TGarbageCollector.Instance.MarkValue or the
project’s equivalent mark method) so these TGocciaValue references are traced;
apply the same pattern to the other helper/value types mentioned (the other
occurrences you noted).

---

Outside diff comments:
In `@docs/souffle-vm.md`:
- Around line 695-705: Update the outdated split-GC wording to reflect the
unified collector TGarbageCollector.Instance: replace any “Souffle GC” and any
“Two GC systems” / “dual GC tracking” descriptions with the single-shared GC
model, remove references to separate interpreter/VM collectors, and clarify that
MarkVMRoots and MarkExternalRoots are the external-root hooks used to cover VM
registers, call stack and bridge call paths; also update the
CollectIfNeeded/threshold note to state it’s currently disabled until
MarkExternalRoots covers all bridge paths and ensure references to
TSouffleHeapObject, PinObject/UnpinObject, AddTempRoot/RemoveTempRoot and O(1)
membership remain consistent with the unified singleton design.

In `@ScriptLoader.dpr`:
- Around line 205-217: The per-file GC shutdown leaves class-var prototype
caches pointing at freed objects; update TGarbageCollector.Shutdown to nil-out
the prototype/cache class variables (e.g., FSharedArrayPrototype,
FSharedMapPrototype, FPrototypeMethodHost and any other FShared* prototypes) so
InitializePrototype’s guarded early-exit (if Assigned(...)) will rebuild
prototypes under the new GC, or alternatively move GC lifetime out of
EmitBytecode/EmitWasm to a single process-wide initialization; pick the
nil-reset approach and ensure Shutdown clears all cached class vars before
freeing GC state.

In `@TestRunner.dpr`:
- Around line 337-401: Wrap the lifetime management of AllTestResults and
AllFailedTests in a try..finally so GC.RemoveTempRoot is always called (move
GC.AddTempRoot(AllTestResults)/GC.AddTempRoot(AllFailedTests) before a try and
call GC.RemoveTempRoot(...) in the finally), and ensure the per-file GC.Collect
call in the loop cannot be skipped by the Continue (either call GC.Collect
before using Continue or restructure the per-file processing so GC.Collect is
executed in a small try..finally around RunScriptFromFile/FileResult handling);
update the block referencing AllTestResults, AllFailedTests,
GC.AddTempRoot/GC.RemoveTempRoot, the for I := 0 to AFiles.Count - 1 loop,
RunScriptFromFile and the existing Continue so temp roots are always removed and
per-file GC.Collect always runs.

In `@units/Goccia.Builtins.Benchmark.pas`:
- Around line 432-493: The code roots ResultsArray but not the transient
TGocciaObjectValue instances (SingleResult and ResultObj) created while
populating it, risking GC during AssignProperty calls; after each
TGocciaObjectValue.Create for SingleResult (the ones created in the try and
except blocks) call TGarbageCollector.Instance.AddTempRoot(SingleResult) before
doing AssignProperty/SetElement and then call
TGarbageCollector.Instance.RemoveTempRoot(SingleResult) immediately after
ResultsArray.SetElement; likewise, after creating ResultObj call
AddTempRoot(ResultObj) before AssignProperty calls and RemoveTempRoot(ResultObj)
just before returning Result (and ensure all Add/Remove use
TGarbageCollector.Instance and mirror each other in the finally block).
- Around line 280-295: The SetupResult returned by ABenchCase.SetupFunction.Call
can be collected during TGocciaMicrotaskQueue.Instance.DrainQueue because it
isn’t rooted yet; immediately after calling ABenchCase.SetupFunction.Call store
and protect the returned value by calling GC.AddTempRoot(SetupResult) (using GC
:= TGarbageCollector.Instance) before invoking
TGocciaMicrotaskQueue.Instance.DrainQueue, and keep that temp root until after
warmup/calibration when it is explicitly removed with RemoveTempRoot; reference
SetupResult, ABenchCase.SetupFunction.Call,
TGocciaMicrotaskQueue.Instance.DrainQueue, and TGarbageCollector.AddTempRoot to
locate and adjust the code.

In `@units/Goccia.Builtins.GlobalArray.pas`:
- Around line 245-267: The partially-built ResultObj is only temp-rooted in the
iterator branch; protect ResultObj in all Array.from code paths (string fast
path, array-like, and standard array branches) by calling
TGarbageCollector.Instance.AddTempRoot(ResultObj) immediately after creating it
and ensuring a matching RemoveTempRoot(ResultObj) in the enclosing finally block
so mapping callbacks (MapCallback/MapArgs), per-element allocations (e.g.,
TGocciaStringLiteralValue) and CreateDataProperty calls cannot be collected;
mirror the iterator branch's temp-root placement and removal around any code
that invokes InvokeCallable or allocates element values.

In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 1072-1104: The returned TGocciaExpectationValue is not keeping the
unwrapped Promise result alive; instead of relying on a temp root around
DrainQueue, implement/override MarkReferences in TGocciaExpectationValue to mark
the internal TGocciaValue fields (the stored unwrapped value passed to
TGocciaExpectationValue.Create, plus any other TGocciaValue fields like
FTestAssertions or prototype/closure fields) so the GC will not reclaim the
wrapped value before chained matchers run; update
TGocciaExpectationValue.MarkReferences to call Mark on that stored value (and
call inherited where appropriate) rather than trying to extend the temp-root
scope in the resolves code path.

In `@units/Goccia.Engine.pas`:
- Around line 252-273: Add pinning for the null singleton in
TGocciaEngine.PinSingletons: locate the PinIfAssigned call sequence in
TGocciaEngine.PinSingletons and add a call for TGocciaNullLiteralValue.NullValue
(e.g., PinIfAssigned(TGocciaNullLiteralValue.NullValue)) so the null singleton
is pinned by the garbage collector just like UndefinedValue, TrueValue,
FalseValue, etc.; use the existing PinIfAssigned helper (which delegates to
TGarbageCollector.Instance.PinObject) to keep the pattern consistent.

In `@units/Goccia.Evaluator.pas`:
- Around line 243-252: The code unconditionally dereferences
TGarbageCollector.Instance (calling AddTempRoot/RemoveTempRoot) which can be
nil; change to grab a local variable (e.g. gc := TGarbageCollector.Instance) and
call gc.AddTempRoot(Iterator) only if Assigned(gc), then in the finally block
call gc.RemoveTempRoot(Iterator) only if Assigned(gc) so you don't crash when GC
is uninitialized or during shutdown; apply the same Assigned-guard pattern to
all other occurrences mentioned (around lines handling Iterator/AdvanceNext and
the other ranges) so AddTempRoot/RemoveTempRoot are always paired only when the
GC was actually added.

In `@units/Goccia.MicrotaskQueue.pas`:
- Around line 149-161: DrainQueue currently removes only the extra execution
roots but leaves the original queue-owned temp root alive after FQueue.Clear,
preventing collected callbacks from being freed; update DrainQueue so that after
clearing FQueue it also calls TGarbageCollector.Instance.RemoveTempRoot for the
temp root that was added by queueMicrotask (the queue-owned root), ensuring any
Task.Handler/Task.Value/Promise roots held solely by the Pascal queue are
released; reference queueMicrotask (where the AddTempRoot is applied),
DrainQueue, FQueue.Clear and TGarbageCollector.Instance.RemoveTempRoot when
making the change.

In `@units/Goccia.Runtime.Operations.pas`:
- Around line 7925-7975: The function FinalizeEnum adds EnumObj to the GC temp
roots via TGarbageCollector.Instance.AddTempRoot but never removes it, leaking
temp roots; after successfully wrapping EnumObj with WrapGocciaValue (result of
WrapGocciaValue(EnumObj) assigned to Result) call
TGarbageCollector.Instance.RemoveTempRoot(EnumObj) (or remove it in a finally
block after wrapping) so the temporary root is released once ownership is
transferred to the wrapper; ensure you still RemoveTempRoot on all success paths
and do not remove it on exception paths where ownership wasn't transferred.
- Around line 3585-3721: GetIterator is adding temporary GC roots
(TGarbageCollector.Instance.AddTempRoot) for Iterator and IteratorObj but never
removing them after ownership transfers via WrapGocciaValue, leaking pinned
iterators; fix by pairing each AddTempRoot with a RemoveTempRoot immediately
after WrapGocciaValue (or use a try/finally that calls RemoveTempRoot in the
finally) so the temp root is cleared once Result is produced, and apply the same
pattern in both the async IteratorObj path and the synchronous Iterator paths
where AddTempRoot is called; reference TGocciaRuntimeOperations.GetIterator,
WrapGocciaValue, AddTempRoot and RemoveTempRoot, and the local variables
Iterator and IteratorObj to locate the spots to update.

In `@units/Goccia.Values.ClassValue.pas`:
- Around line 747-757: The newly created object Instance is temp-rooted only
when ConstructorToCall is present but not when the native-init path is taken;
wrap the call to
TGocciaInstanceValue(NativeInstance).InitializeNativeFromArguments(AArguments)
with TGarbageCollector.Instance.AddTempRoot(Instance) before the call and
TGarbageCollector.Instance.RemoveTempRoot(Instance) in a finally block so
Instance is protected during InitializeNativeFromArguments; use the same
AddTempRoot/RemoveTempRoot pattern as around ConstructorToCall.Call and refer to
ConstructorToCall, NativeInstance,
TGocciaInstanceValue.InitializeNativeFromArguments and
TGarbageCollector.Instance.AddTempRoot/RemoveTempRoot.

In `@units/Goccia.Values.IteratorValue.pas`:
- Around line 284-295: The loop roots the Iterator but not the TGocciaValue
instances passed into user code or the Callback reference, so ensure any Value
and Callback that are passed to InvokeIteratorCallback (and similar methods like
reduce/some/every/find) are protected: call
TGarbageCollector.Instance.AddTempRoot(Value) and AddTempRoot(Callback) before
InvokeIteratorCallback/Call and remove them in a matching finally via
RemoveTempRoot(Value) and RemoveTempRoot(Callback); mirror this pattern around
every place in this unit where a locally-held TGocciaValue is handed back to
user code (e.g., the sections noted around reduce, some, every, find and the
other ranges mentioned).

In `@units/Goccia.Values.TemporalPlainDate.pas`:
- Around line 452-461: The TGarbageCollector.Instance dereference is unguarded
(in the DateSubtract/DateAdd path using NegatedDur), so wrap the
AddTempRoot/RemoveTempRoot calls with a nil-check: test
Assigned(TGarbageCollector.Instance) before calling AddTempRoot and record a
boolean (e.g., GCAdded) to indicate you added the temp root, then in the finally
only call RemoveTempRoot if GCAdded is true; keep the NewArgs creation and
DateAdd call intact (refer to TGarbageCollector.Instance, AddTempRoot,
RemoveTempRoot, NewArgs and DateAdd) so you avoid an access violation when
Instance is nil during init/teardown.

---

Nitpick comments:
In `@docs/adding-built-in-types.md`:
- Around line 75-82: The uses clause incorrectly includes
GarbageCollector.Generic even though this unit does not call TGarbageCollector;
remove GarbageCollector.Generic from the uses list (alongside the existing
Goccia.* and Goccia.Values.* units) so the starter template has no direct GC
dependency, and do not add any TGarbageCollector calls—rely on
TGocciaSharedPrototype.Create which already pins the prototype and method host.

In `@docs/souffle-vm.md`:
- Line 704: Update the documentation phrase that currently refers to the
Souffle-specific type TDictionary<TSouffleHeapObject, Boolean> so it describes
the generic managed-object membership type used by the unified collector;
replace the Souffle-specific symbol with a generic descriptor (e.g.,
TDictionary<TManagedObject, Boolean> or "dictionary of managed objects") and
mention that pinned objects and temp roots use this generic managed-object
membership structure to provide O(1) membership semantics for the unified
collector.

In `@units/GarbageCollector.Managed.pas`:
- Around line 8-13: Rename the abbreviated base type and its members to use full
words: change class TGCManagedObject to TGarbageCollectedObject, rename the
private field FGCMarked to FGarbageCollectedMarked, and rename the property
GCMarked to GarbageCollectedMarked (keep MarkReferences as-is and preserve
virtual). Update all references/usages, typecasts and unit/interface
declarations that mention TGCManagedObject, FGCMarked or GCMarked throughout the
codebase to the new identifiers so compilation and behavior remain identical.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 873dd5d1-a6c9-4589-aad1-375ff7cb5f05

📥 Commits

Reviewing files that changed from the base of the PR and between f977a1c and bf42528.

📒 Files selected for processing (61)

• AGENTS.md
• BenchmarkRunner.dpr
• README.md
• ScriptLoader.dpr
• TestRunner.dpr
• docs/adding-built-in-types.md
• docs/architecture.md
• docs/benchmarks.md
• docs/code-style.md
• docs/design-decisions.md
• docs/embedding.md
• docs/souffle-vm.md
• docs/testing.md
• docs/value-system.md
• souffle/Souffle.GarbageCollector.pas
• souffle/Souffle.Heap.pas
• souffle/Souffle.VM.NativeFunction.pas
• souffle/Souffle.VM.pas
• souffle/Souffle.Value.pas
• units/GarbageCollector.Generic.pas
• units/GarbageCollector.Managed.pas
• units/Goccia.AST.Expressions.pas
• units/Goccia.Builtins.Base.pas
• units/Goccia.Builtins.Benchmark.pas
• units/Goccia.Builtins.GlobalArray.pas
• units/Goccia.Builtins.GlobalPromise.pas
• units/Goccia.Builtins.Globals.pas
• units/Goccia.Builtins.Math.pas
• units/Goccia.Builtins.Temporal.pas
• units/Goccia.Builtins.TestAssertions.pas
• units/Goccia.Compiler.Test.pas
• units/Goccia.Engine.Backend.pas
• units/Goccia.Engine.pas
• units/Goccia.Evaluator.pas
• units/Goccia.GarbageCollector.pas
• units/Goccia.Interpreter.pas
• units/Goccia.MicrotaskQueue.pas
• units/Goccia.Runtime.Operations.pas
• units/Goccia.Scope.pas
• units/Goccia.SharedPrototype.pas
• units/Goccia.Values.ArrayBufferValue.pas
• units/Goccia.Values.ArrayValue.pas
• units/Goccia.Values.AsyncFunctionValue.pas
• units/Goccia.Values.BooleanObjectValue.pas
• units/Goccia.Values.ClassValue.pas
• units/Goccia.Values.FunctionBase.pas
• units/Goccia.Values.FunctionValue.pas
• units/Goccia.Values.Iterator.Lazy.pas
• units/Goccia.Values.IteratorValue.pas
• units/Goccia.Values.MapValue.pas
• units/Goccia.Values.NumberObjectValue.pas
• units/Goccia.Values.ObjectValue.pas
• units/Goccia.Values.Primitives.pas
• units/Goccia.Values.PromiseValue.pas
• units/Goccia.Values.SetValue.pas
• units/Goccia.Values.SharedArrayBufferValue.pas
• units/Goccia.Values.StringObjectValue.pas
• units/Goccia.Values.SymbolValue.pas
• units/Goccia.Values.TemporalPlainDate.pas
• units/Goccia.Values.ToPrimitive.pas
• units/Goccia.Values.TypedArrayValue.pas

💤 Files with no reviewable changes (2)

• souffle/Souffle.GarbageCollector.pas
• units/Goccia.GarbageCollector.pas

[frostney]

Re: comment 4031313636

Thanks for the benchmark results. The interpreted mode improvements (+5.8% avg) are a nice side effect of the GC unification. The bytecode regressions in collections.js (Map get/has/delete, keys/values/entries) appear to be pre-existing measurement noise unrelated to the GC changes — these operations don't interact with bridge caches or GC collection.

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

docs/souffle-vm.md (1)

702-702: ⚠️ Potential issue | 🟡 Minor

Incorrect method name MarkVMRoots.

The documentation references a MarkVMRoots method that doesn't exist in the TGarbageCollector API. The actual mechanism uses AddExternalRootMarker to register a callback. The protected MarkRoots method (shown in GarbageCollector.Generic.pas:230-249) iterates over registered external root markers.

📝 Proposed correction

-- **External root marker** — The VM registers `MarkVMRoots` to mark all values in the register file and call stack during collection
+- **External root marker** — The VM registers a root marker callback via `AddExternalRootMarker` to mark all values in the register file and call stack during collection

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/souffle-vm.md` at line 702, The docs incorrectly reference a
non-existent MarkVMRoots method; update the text to describe the actual API:
explain that callers register a callback via AddExternalRootMarker and that
TGarbageCollector.MarkRoots (the protected routine that iterates registered
external root markers) will invoke those callbacks during collection; replace
the MarkVMRoots mention with AddExternalRootMarker and a brief note that the
registered callback is invoked by TGarbageCollector.MarkRoots.

units/Goccia.Runtime.Operations.pas (1)

5271-5285: ⚠️ Potential issue | 🔴 Critical

Temp-root NewArr while callbacks are running.

After AllocateObject(NewArr), the array is only held in a Pascal local until the method returns. VM.ExecuteFunction(...) can trigger GC, so NewArr can be swept mid-loop in map, filter, and flatMap.

Suggested fix

   NewArr := TSouffleArray.Create(Arr.Count);
   NewArr.Delegate := GNativeArrayJoinRuntime.VM.ArrayDelegate;
   if Assigned(TGarbageCollector.Instance) then
-    TGarbageCollector.Instance.AllocateObject(NewArr);
-  for I := 0 to Arr.Count - 1 do
-  begin
-    if SouffleIsHole(Arr.Get(I)) then
-    begin
-      NewArr.Push(SouffleNilWithFlags(GOCCIA_NIL_HOLE));
-      Continue;
-    end;
-    NewArr.Push(GNativeArrayJoinRuntime.VM.ExecuteFunction(Callback,
-      [SouffleNil, Arr.Get(I), SouffleInteger(I), AReceiver]));
-  end;
+  begin
+    TGarbageCollector.Instance.AllocateObject(NewArr);
+    TGarbageCollector.Instance.AddTempRoot(NewArr);
+  end;
+  try
+    for I := 0 to Arr.Count - 1 do
+    begin
+      if SouffleIsHole(Arr.Get(I)) then
+      begin
+        NewArr.Push(SouffleNilWithFlags(GOCCIA_NIL_HOLE));
+        Continue;
+      end;
+      NewArr.Push(GNativeArrayJoinRuntime.VM.ExecuteFunction(Callback,
+        [SouffleNil, Arr.Get(I), SouffleInteger(I), AReceiver]));
+    end;
+  finally
+    if Assigned(TGarbageCollector.Instance) then
+      TGarbageCollector.Instance.RemoveTempRoot(NewArr);
+  end;
   Result := SouffleReference(NewArr);

Use the same pattern for NativeArrayFilter and NativeArrayFlatMap.

As per coding guidelines: "Values held only by Pascal code (not in any GocciaScript scope) must be protected with AddTempRoot/RemoveTempRoot for the duration they are needed".

Also applies to: 5303-5317, 5696-5718

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Runtime.Operations.pas` around lines 5271 - 5285, NewArr is only
a Pascal-local after AllocateObject and can be collected if
GNativeArrayJoinRuntime.VM.ExecuteFunction triggers GC; wrap NewArr with the
temp-root API by calling AddTempRoot(NewArr) immediately after
TGarbageCollector.Instance.AllocateObject(NewArr) and calling
RemoveTempRoot(NewArr) after the loop (before returning Result) so the array
cannot be swept mid-iteration; apply the same pattern in NativeArrayFilter and
NativeArrayFlatMap (and the other indicated ranges) around their AllocateObject
/ loops that call VM.ExecuteFunction.

♻️ Duplicate comments (1)

units/Goccia.Runtime.Operations.pas (1)

8694-8696: ⚠️ Potential issue | 🟠 Major

Don't make FArrayBridgeReverse a permanent root set.

FArrayBridgeReverse is never pruned when FArrayBridgeCache is cleared. Marking both its values and keys here keeps every bridged array pair alive for the lifetime of TGocciaRuntimeOperations, which can bring back the cross-file memory growth this PR is trying to remove.

Also applies to: 8750-8752

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Runtime.Operations.pas` around lines 8694 - 8696, The loop is
marking entries in FArrayBridgeReverse (both values and keys) which effectively
makes FArrayBridgeReverse a permanent root; stop marking from
FArrayBridgeReverse. Instead, iterate only the current FArrayBridgeCache entries
and call MarkReferences on their TSouffleHeapObject values (or otherwise mark
only objects reachable from FArrayBridgeCache). Update the code paths in
TGocciaRuntimeOperations that call TSouffleHeapObject(…).MarkReferences so they
reference FArrayBridgeCache items rather than FArrayBridgeReverse (apply the
same change for the other occurrence around the second block that mirrored this
behavior).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/souffle-vm.md`:
- Line 698: The doc incorrectly states that BenchmarkRunner calls Collect before
each measurement round; update the sentence in docs/souffle-vm.md to reflect the
actual behavior: BenchmarkRunner calls Collect in the finally block after each
file completes (see BenchmarkRunner.dpr where Collect is invoked at the end of
file processing), not before measurement rounds; revise the "Lifecycle"
paragraph to remove or replace the phrase "The BenchmarkRunner additionally
calls `Collect` before each measurement round" with wording that it calls
`Collect` after each file completes to reclaim memory between script executions.

In `@docs/value-system.md`:
- Around line 110-113: The sample exposes the private backing field FGCMarked
instead of the public property GCMarked; update the declaration for
TGCManagedObject to show GCMarked (the public property callers use) rather than
FGCMarked, and keep MarkReferences as-is; ensure the prose and example reference
GCMarked to match units/GarbageCollector.Managed.pas and align with the
documentation on Line 122.

In `@units/Goccia.Runtime.Operations.pas`:
- Around line 3631-3642: The AddTempRoot calls around iterator creation (e.g.,
when creating TGocciaMapIteratorValue/TGocciaSetIteratorValue in GetIterator and
similar sites) must be balanced by calling
TGarbageCollector.Instance.RemoveTempRoot immediately after WrapGocciaValue
returns so the temp root only guards the value during wrapping; update each
AddTempRoot in this block (and the other mentioned blocks and EnumObj) to add
the temp root, call Result := WrapGocciaValue(Iterator), then remove the temp
root (guard for Assigned(TGarbageCollector.Instance) when removing) so
iterators/enums are not permanently rooted.

---

Outside diff comments:
In `@docs/souffle-vm.md`:
- Line 702: The docs incorrectly reference a non-existent MarkVMRoots method;
update the text to describe the actual API: explain that callers register a
callback via AddExternalRootMarker and that TGarbageCollector.MarkRoots (the
protected routine that iterates registered external root markers) will invoke
those callbacks during collection; replace the MarkVMRoots mention with
AddExternalRootMarker and a brief note that the registered callback is invoked
by TGarbageCollector.MarkRoots.

In `@units/Goccia.Runtime.Operations.pas`:
- Around line 5271-5285: NewArr is only a Pascal-local after AllocateObject and
can be collected if GNativeArrayJoinRuntime.VM.ExecuteFunction triggers GC; wrap
NewArr with the temp-root API by calling AddTempRoot(NewArr) immediately after
TGarbageCollector.Instance.AllocateObject(NewArr) and calling
RemoveTempRoot(NewArr) after the loop (before returning Result) so the array
cannot be swept mid-iteration; apply the same pattern in NativeArrayFilter and
NativeArrayFlatMap (and the other indicated ranges) around their AllocateObject
/ loops that call VM.ExecuteFunction.

---

Duplicate comments:
In `@units/Goccia.Runtime.Operations.pas`:
- Around line 8694-8696: The loop is marking entries in FArrayBridgeReverse
(both values and keys) which effectively makes FArrayBridgeReverse a permanent
root; stop marking from FArrayBridgeReverse. Instead, iterate only the current
FArrayBridgeCache entries and call MarkReferences on their TSouffleHeapObject
values (or otherwise mark only objects reachable from FArrayBridgeCache). Update
the code paths in TGocciaRuntimeOperations that call
TSouffleHeapObject(…).MarkReferences so they reference FArrayBridgeCache items
rather than FArrayBridgeReverse (apply the same change for the other occurrence
around the second block that mirrored this behavior).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e0da2ca0-6337-4daf-8aca-8a5a7a107524

📥 Commits

Reviewing files that changed from the base of the PR and between bf42528 and 3fa88f1.

📒 Files selected for processing (7)

• AGENTS.md
• BenchmarkRunner.dpr
• docs/code-style.md
• docs/souffle-vm.md
• docs/testing.md
• docs/value-system.md
• units/Goccia.Runtime.Operations.pas

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

units/Goccia.Builtins.Benchmark.pas (1)

289-295: ⚠️ Potential issue | 🟠 Major

Root SetupResult before draining microtasks.

SetupResult is only held in a Pascal local until Line 295. DrainQueue on Lines 290-291 can re-enter script code before that root is added, so the setup value has a collection window right after Call returns.

Suggested fix

       StartNanoseconds := GetNanoseconds;
       SetupResult := ABenchCase.SetupFunction.Call(EmptyArgs, TGocciaUndefinedLiteralValue.UndefinedValue);
+      if Assigned(SetupResult) and Assigned(GC) then
+        GC.AddTempRoot(SetupResult);
       if Assigned(TGocciaMicrotaskQueue.Instance) then
         TGocciaMicrotaskQueue.Instance.DrainQueue;
       Result.SetupMs := (GetNanoseconds - StartNanoseconds) / 1000000;
-
-      if Assigned(SetupResult) and Assigned(GC) then
-        GC.AddTempRoot(SetupResult);

As per coding guidelines, "Values held only by Pascal code (not in any GocciaScript scope) must be protected with AddTempRoot/RemoveTempRoot for the duration they are needed."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Builtins.Benchmark.pas` around lines 289 - 295, SetupResult
returned from ABenchCase.SetupFunction.Call is vulnerable to collection because
TGocciaMicrotaskQueue.Instance.DrainQueue can re-enter script code before the GC
root is added; protect SetupResult immediately after the Call by calling
GC.AddTempRoot(SetupResult) before invoking
TGocciaMicrotaskQueue.Instance.DrainQueue and ensure you call
GC.RemoveTempRoot(SetupResult) after you no longer need it (e.g., after
computing Result.SetupMs and any subsequent use), so the root is held for the
full duration SetupResult is only referenced by Pascal code.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@AGENTS.md`:
- Line 367: The example is confusing because it shows "TGarbageCollector not
TGenericGC" where "TGenericGC" is not a reasonable abbreviation; update the
example to use a realistic abbreviation by replacing "TGenericGC" with "TGC" so
it reads "TGarbageCollector not TGC" (leave the exceptions list like `AST`,
`JSON`, `REPL`, `ISO`, `Utils` unchanged).

In `@docs/architecture.md`:
- Around line 356-360: The table documents the wrong active-scope API names;
update references of PushActiveRoot/PopActiveRoot to the correct
PushActiveScope/PopActiveScope, and adjust the explanatory text to state that
scopes register with the GC in their constructor and are pushed/popped via
PushActiveScope/PopActiveScope (used from TGocciaFunctionValue.Call), while
keeping mention of AddRootObject/RemoveRootObject and temp/external roots
intact; ensure the class names TGCManagedObject and TGarbageCollector and the
methods Initialize/Instance/Shutdown remain unchanged.

In `@units/Goccia.Builtins.Benchmark.pas`:
- Around line 428-430: The ResultsArray local must be created before entering
the try/finally to avoid passing an uninitialized variable to the GC on
exception; move the TGocciaArrayValue.Create call so ResultsArray is assigned
before the try, then inside the try/finally only call
TGarbageCollector.Instance.AddTempRoot(ResultsArray) and in the finally call
TGarbageCollector.Instance.RemoveTempRoot(ResultsArray) (guarded by
Assigned(ResultsArray) if needed) so RemoveTempRoot never receives an undefined
value; reference the ResultsArray variable, TGocciaArrayValue.Create,
TGarbageCollector.Instance.AddTempRoot and RemoveTempRoot when making the
change.

---

Outside diff comments:
In `@units/Goccia.Builtins.Benchmark.pas`:
- Around line 289-295: SetupResult returned from ABenchCase.SetupFunction.Call
is vulnerable to collection because TGocciaMicrotaskQueue.Instance.DrainQueue
can re-enter script code before the GC root is added; protect SetupResult
immediately after the Call by calling GC.AddTempRoot(SetupResult) before
invoking TGocciaMicrotaskQueue.Instance.DrainQueue and ensure you call
GC.RemoveTempRoot(SetupResult) after you no longer need it (e.g., after
computing Result.SetupMs and any subsequent use), so the root is held for the
full duration SetupResult is only referenced by Pascal code.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f095952d-165e-4949-a1bc-31d5efaee734

📥 Commits

Reviewing files that changed from the base of the PR and between 3fa88f1 and 21c62cb.

📒 Files selected for processing (6)

• AGENTS.md
• docs/architecture.md
• docs/benchmarks.md
• docs/design-decisions.md
• docs/souffle-vm.md
• units/Goccia.Builtins.Benchmark.pas

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

units/Goccia.Builtins.Benchmark.pas (1)

286-295: ⚠️ Potential issue | 🔴 Critical

Root SetupResult before draining microtasks.

SetupResult sits in Pascal-only storage after Line 289, but DrainQueue can run before Line 295 roots it. If setup returns a GC-managed value and the drain triggers collection, RunArgs can end up holding a freed value.

Suggested fix

       StartNanoseconds := GetNanoseconds;
       SetupResult := ABenchCase.SetupFunction.Call(EmptyArgs, TGocciaUndefinedLiteralValue.UndefinedValue);
+      if Assigned(SetupResult) and Assigned(GC) then
+        GC.AddTempRoot(SetupResult);
       if Assigned(TGocciaMicrotaskQueue.Instance) then
         TGocciaMicrotaskQueue.Instance.DrainQueue;
       Result.SetupMs := (GetNanoseconds - StartNanoseconds) / 1000000;
-
-      if Assigned(SetupResult) and Assigned(GC) then
-        GC.AddTempRoot(SetupResult);
     end;

As per coding guidelines, "Values held only by Pascal code (not in any GocciaScript scope) must be protected with `AddTempRoot`/`RemoveTempRoot` for the duration they are needed."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Builtins.Benchmark.pas` around lines 286 - 295, SetupResult
(returned by ABenchCase.SetupFunction.Call) can be collected during
TGocciaMicrotaskQueue.Instance.DrainQueue because it lives only in Pascal stack;
to fix, immediately guard SetupResult with GC.AddTempRoot after the Call and
before calling TGocciaMicrotaskQueue.Instance.DrainQueue, and then call
GC.RemoveTempRoot(SetupResult) once the value is no longer needed (e.g., after
assigning into Result or at end of scope); update the block around
ABenchCase.SetupFunction, SetupResult,
TGocciaMicrotaskQueue.Instance.DrainQueue, GC.AddTempRoot and GC.RemoveTempRoot
accordingly so the returned value is rooted during any microtask drain.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/value-system.md`:
- Around line 118-125: The docs show TGocciaValue now inheriting from
TGCManagedObject but later examples (IsPrimitive and Virtual Property Access
snippets) still declare TGocciaValue = class(TInterfacedObject); update those
later code examples to use TGocciaValue = class(TGCManagedObject) and adjust any
constructor/cleanup semantics in the examples to match the unified GC model
(e.g., remove manual reference counting or Free calls and rely on
AfterConstruction/GC registration), ensuring all references to TGocciaValue,
IsPrimitive, and the Virtual Property Access snippets reflect the
TGCManagedObject inheritance.

---

Outside diff comments:
In `@units/Goccia.Builtins.Benchmark.pas`:
- Around line 286-295: SetupResult (returned by ABenchCase.SetupFunction.Call)
can be collected during TGocciaMicrotaskQueue.Instance.DrainQueue because it
lives only in Pascal stack; to fix, immediately guard SetupResult with
GC.AddTempRoot after the Call and before calling
TGocciaMicrotaskQueue.Instance.DrainQueue, and then call
GC.RemoveTempRoot(SetupResult) once the value is no longer needed (e.g., after
assigning into Result or at end of scope); update the block around
ABenchCase.SetupFunction, SetupResult,
TGocciaMicrotaskQueue.Instance.DrainQueue, GC.AddTempRoot and GC.RemoveTempRoot
accordingly so the returned value is rooted during any microtask drain.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 37024ea0-7e56-49c5-b838-575af6ce0423

📥 Commits

Reviewing files that changed from the base of the PR and between 21c62cb and 460062f.

📒 Files selected for processing (3)

• AGENTS.md
• docs/value-system.md
• units/Goccia.Builtins.Benchmark.pas

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Update the later TGocciaValue snippets to match the unified GC model.

This section now documents TGocciaValue = class(TGCManagedObject), but the later examples under IsPrimitive and Virtual Property Access still show class(TInterfacedObject). That leaves the page self-contradictory for anyone using these snippets as reference.

📘 Suggested follow-up

-TGocciaValue = class(TInterfacedObject)
+TGocciaValue = class(TGCManagedObject)

Apply that replacement in the later code examples as well.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/value-system.md` around lines 118 - 125, The docs show TGocciaValue now
inheriting from TGCManagedObject but later examples (IsPrimitive and Virtual
Property Access snippets) still declare TGocciaValue = class(TInterfacedObject);
update those later code examples to use TGocciaValue = class(TGCManagedObject)
and adjust any constructor/cleanup semantics in the examples to match the
unified GC model (e.g., remove manual reference counting or Free calls and rely
on AfterConstruction/GC registration), ensuring all references to TGocciaValue,
IsPrimitive, and the Virtual Property Access snippets reflect the
TGCManagedObject inheritance.