Add parallel execution for multi-file runners#306
Merged
Conversation
- Run scripts, tests, and benchmarks across worker threads - Move shared builtin state to thread-local storage - Add `--jobs` selection and preserve ordered reporting
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
👮 Files not reviewed due to content moderation or server errors (2)
📝 WalkthroughWalkthroughAdds a thread-pool for parallel file execution, per-thread runtime initialization and prototype caching, a --jobs/-j CLI option, and converts many global/class-shared singletons/prototype caches to thread-local storage; runners dispatch work to worker threads then join results on the main thread preserving input order. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (Main)
participant Init as EnsureSharedPrototypesInitialized
participant Pool as TGocciaThreadPool
participant Worker as TGocciaFileWorker
participant Main as Main Thread / Reporter
CLI->>Init: EnsureSharedPrototypesInitialized(GlobalBuiltins)
CLI->>Pool: Create(JobCount) / RunAll(Files, WorkerProc, WorkerData)
Pool->>Worker: start (per-worker InitThreadRuntime)
Worker->>Worker: for each dequeued file: call WorkerProc(file,index)
Worker-->>Pool: append TGocciaWorkerResult(Index, Success, ErrorMessage, ConsoleOutput, Data)
Worker->>Worker: ShutdownThreadRuntime (detach coverage)
Pool-->>CLI: all workers complete, Results[] ordered by Index
CLI->>Main: iterate Results[] in original order, Reporter.Add / Merge coverage
Main-->>CLI: final summary/output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
performance
Contributor
Benchmark Results364 benchmarks Interpreted: 🟢 5 improved · 🔴 357 regressed · 2 unchanged · avg -43.2% arraybuffer.js — Interp: 🔴 14 · avg -51.2% · Bytecode: 🔴 14 · avg -51.5%
arrays.js — Interp: 🔴 19 · avg -53.5% · Bytecode: 🔴 19 · avg -54.6%
async-await.js — Interp: 🔴 6 · avg -52.5% · Bytecode: 🔴 6 · avg -53.9%
base64.js — Interp: 🔴 10 · avg -51.5% · Bytecode: 🔴 10 · avg -51.0%
classes.js — Interp: 🔴 31 · avg -47.0% · Bytecode: 🔴 31 · avg -48.2%
closures.js — Interp: 🔴 11 · avg -50.1% · Bytecode: 🔴 11 · avg -52.0%
collections.js — Interp: 🔴 12 · avg -44.5% · Bytecode: 🔴 12 · avg -49.6%
destructuring.js — Interp: 🔴 22 · avg -45.8% · Bytecode: 🔴 22 · avg -48.0%
fibonacci.js — Interp: 🔴 8 · avg -48.5% · Bytecode: 🔴 8 · avg -53.6%
float16array.js — Interp: 🟢 2, 🔴 30 · avg -38.4% · Bytecode: 🔴 32 · avg -44.3%
for-of.js — Interp: 🔴 7 · avg -46.7% · Bytecode: 🔴 7 · avg -54.2%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🔴 42 · avg -44.0% · Bytecode: 🔴 42 · avg -47.7%
json.js — Interp: 🔴 20 · avg -44.5% · Bytecode: 🔴 20 · avg -47.2%
jsx.jsx — Interp: 🔴 21 · avg -46.4% · Bytecode: 🔴 21 · avg -50.8%
modules.js — Interp: 🔴 9 · avg -55.1% · Bytecode: 🔴 9 · avg -57.8%
numbers.js — Interp: 🔴 11 · avg -53.1% · Bytecode: 🔴 11 · avg -49.6%
objects.js — Interp: 🔴 7 · avg -49.3% · Bytecode: 🔴 7 · avg -50.1%
promises.js — Interp: 🔴 12 · avg -48.5% · Bytecode: 🔴 12 · avg -51.2%
regexp.js — Interp: 🔴 11 · avg -47.6% · Bytecode: 🔴 11 · avg -47.3%
strings.js — Interp: 🔴 19 · avg -43.2% · Bytecode: 🔴 19 · avg -34.0%
typed-arrays.js — Interp: 🟢 3, 🔴 17, 2 unch. · avg -5.9% · Bytecode: 🟢 2, 🔴 19, 1 unch. · avg -13.4%
uint8array-encoding.js — Interp: 🔴 18 · avg -26.1% · Bytecode: 🔴 17, 1 unch. · avg -28.3%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
units/Goccia.SharedPrototype.pas (1)
51-62:⚠️ Potential issue | 🟠 MajorRefresh
prototype.constructoron every expose.
TGocciaSharedPrototypeoutlives any single engine on a thread. With this guard, the throwaway pre-init engine — or just the first engine on that thread — permanently owns*.prototype.constructor, so later engines observe a stale constructor and keep the old object graph alive.Suggested fix
- if not FPrototype.HasProperty(PROP_CONSTRUCTOR) then - FPrototype.AssignProperty(PROP_CONSTRUCTOR, AConstructor); + FPrototype.AssignProperty(PROP_CONSTRUCTOR, AConstructor);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.SharedPrototype.pas` around lines 51 - 62, TGocciaSharedPrototype.ExposeOnConstructor currently skips writing PROP_CONSTRUCTOR if it's already present, which lets the first engine on a thread permanently own prototype.constructor; change ExposeOnConstructor so it always refreshes the constructor on FPrototype (i.e., remove the conditional HasProperty check and unconditionally call FPrototype.AssignProperty(PROP_CONSTRUCTOR, AConstructor) after handling TGocciaClassValue/TGocciaObjectValue) so every expose updates the shared prototype.constructor to the current AConstructor.units/Goccia.Values.ArrayValue.pas (1)
327-336:⚠️ Potential issue | 🟠 MajorKeep
Array.prototype.constructorsynced with the current engine.
FSharedArrayPrototypelives for the whole thread. AfterEnsureSharedPrototypesInitialized— or any earlier engine on the same thread — setsconstructor, later engines skip this write, soArray.prototype.constructorandArraySpeciesCreate()keep using the stale constructor from the old engine.Suggested fix
- if not FSharedArrayPrototype.HasProperty(PROP_CONSTRUCTOR) then - FSharedArrayPrototype.AssignProperty(PROP_CONSTRUCTOR, AConstructor); + FSharedArrayPrototype.AssignProperty(PROP_CONSTRUCTOR, AConstructor);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.ArrayValue.pas` around lines 327 - 336, The code currently skips setting Array.prototype.constructor if FSharedArrayPrototype already has PROP_CONSTRUCTOR, which leaves a stale constructor from a previous engine; in TGocciaArrayValue.ExposePrototype always (re)assign PROP_CONSTRUCTOR on FSharedArrayPrototype to the supplied AConstructor so the prototype is kept in sync with the current engine — replace the conditional HasProperty check and ensure you use the appropriate assign/replace method (e.g., AssignProperty or a ReplaceProperty overload) to overwrite PROP_CONSTRUCTOR with AConstructor.
🧹 Nitpick comments (6)
units/Goccia.Builtins.Math.pas (1)
81-82: Make the member-definition array local.
FStaticMembersis assigned on Line 146 and consumed on Line 150 inside the same constructor. It never acts as a cache, so the newthreadvarjust adds extra mutable state to reason about. A localDefinitionsvariable would keep the thread-safety benefit without storing a managed array in thread-local storage.Also applies to: 86-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.Math.pas` around lines 81 - 82, Remove the threadvar FStaticMembers and instead declare a local TArray<TGocciaMemberDefinition> variable (e.g., Definitions) inside the constructor where FStaticMembers is assigned and consumed (the code around the current assignments at the TGocciaBuiltinsMath constructor). Replace uses of FStaticMembers in that constructor with the local Definitions variable, ensure you populate Definitions the same way as before, and verify there are no other references to FStaticMembers elsewhere (if there are, convert them to use a local or refactor accordingly); this keeps thread-safety without storing a managed array in thread-local storage.units/Goccia.Builtins.JSONL.pas (1)
52-54: Use a local definitions array here.
FStaticMembersis written on Line 71 and read once on Line 76, with no later reads in the unit. Thethreadvardoesn't buy reuse here and can be a constructor-local variable instead.Also applies to: 55-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.JSONL.pas` around lines 52 - 54, The threadvar FStaticMembers is unnecessary because it's only written once and read once within the same initialization scope; remove the threadvar declaration and instead declare a local variable (e.g., LocalMembers: TArray<TGocciaMemberDefinition>) in the routine that currently assigns/uses FStaticMembers (the constructor/initializer where FStaticMembers is set at the assignment site and read later). Replace references to FStaticMembers with the local variable (use the local while building the member definitions) and delete the threadvar/FStaticMembers declaration so the array lifetime is limited to the routine.units/Goccia.Values.TemporalPlainMonthDay.pas (1)
56-58:FPrototypeMembersis dead cache state here.Once Line 161 starts returning early, this array is built once and never reread. Keeping
FSharedthread-local but making the member-definition array local would simplify the prototype-init path.Also applies to: 157-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalPlainMonthDay.pas` around lines 56 - 58, FPrototypeMembers is a stale threadvar cache causing the prototype init to only build once; change FPrototypeMembers from a threadvar field into a local variable used inside the prototype-initialization routine (where TGocciaSharedPrototype/FShared is referenced) so the member-definition array is constructed on each init-path instead of being kept thread-local. Locate usages of FPrototypeMembers and the prototype setup function that currently reads/sets it (references: FPrototypeMembers, FShared, TGocciaSharedPrototype, TGocciaMemberDefinition) and move the array declaration into that routine, update assignments/usages to the local variable, and remove the threadvar declaration to simplify and ensure fresh member-definition construction (apply same change pattern to the other similar block noted around the prototype init).units/Goccia.Values.NumberObjectValue.pas (1)
51-55: LocalizeFPrototypeMembers; keep the reusable thread-local pieces.
FSharedNumberPrototypeandFPrototypeMethodHostare the actual per-thread state here.FPrototypeMembersisn't: after Line 94 starts exiting early, the array built on Lines 98-107 is never reread.Also applies to: 90-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.NumberObjectValue.pas` around lines 51 - 55, FPrototypeMembers should not be a threadvar because it’s rebuilt each time and left stale when early exits occur; keep FSharedNumberPrototype and FPrototypeMethodHost as thread-local but remove FPrototypeMembers from the threadvar block and instead declare it as a local variable inside the routine that constructs the number prototype (the code that currently populates FPrototypeMembers in the TGocciaNumberObjectValue/TGocciaObjectValue prototype init). Update the constructor/initializer to use the new local TArray<TGocciaMemberDefinition> variable when building members (and free/leave scope on exit), and remove references to the global FPrototypeMembers; apply the same change to the adjacent prototype-building code paths noted (the 90-118 region) so no shared persistent array remains.units/Goccia.Builtins.GlobalObject.pas (1)
68-70:FStaticMembersis effectively a temporary.The constructor writes it on Line 164 and immediately consumes it on Line 168. A local variable would remove unnecessary thread-local state and keep this change focused on data that's actually shared across calls.
Also applies to: 133-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.GlobalObject.pas` around lines 68 - 70, FStaticMembers is declared as a threadvar but is only written in the constructor and immediately consumed, so remove the thread-local field and use a local variable instead: delete the threadvar FStaticMembers declaration and in the constructor (where FStaticMembers is currently assigned) introduce a local variable (e.g., LocalMembers: TArray<TGocciaMemberDefinition>) to build the TGocciaMemberDefinition array, then pass/consume that local array in the same places the constructor currently uses FStaticMembers; also update any code in the 133–168 region that references FStaticMembers to use the local variable or the consumer call so no thread-local state remains.units/Goccia.Benchmark.Reporter.pas (1)
108-111: Fail fast on out-of-rangeFiles[]access.
GetFileResultis now part of the public surface, but it does raw dynamic-array indexing. With range checks off, an invalid index becomes undefined memory access instead of a clear failure. Add an explicit bounds check here so misuse fails deterministically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Benchmark.Reporter.pas` around lines 108 - 111, TBenchmarkReporter.GetFileResult currently returns FFiles[AIndex] without validation; add an explicit bounds check against Length(FFiles) (or Low/High) and raise a clear exception (e.g., ERangeError or a custom exception) when AIndex is out of range so callers fail deterministically instead of producing undefined behavior; update TBenchmarkReporter.GetFileResult to validate AIndex before accessing FFiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ScriptLoader.dpr`:
- Around line 692-707: The worker should not call RunScriptFromFile directly
because that lets the script runtime write to stdout from background threads;
instead modify TScriptLoaderApp.ScriptWorkerProc to invoke RunSource (not
RunScriptFromFile), capture its returned console text and any error text into
AConsoleOutput and AErrorMessage, and propagate failures by setting ExitCode = 1
when RunSource indicates an error or raises; locate ScriptWorkerProc and replace
the direct RunScriptFromFile call with a call to RunSource (or the function that
returns printed output/errors), assign its outputs to
AConsoleOutput/AErrorMessage, and ensure exceptions are caught and marshaled
into AErrorMessage so RunScriptsParallel on the main thread can report failures
in order.
In `@TestRunner.dpr`:
- Around line 227-228: The parallel branch is chosen whenever
GetJobCount(Files.Count) > 1 which bypasses the CLI --exit-on-first-failure
behavior; update the condition so you only spawn RunScriptsFromFilesParallel
when GetJobCount(Files.Count) > 1 AND the exit-on-first-failure flag is not set
(e.g. check the boolean backing --exit-on-first-failure), otherwise call the
sequential runner that respects FailedCount and exits on first failure; change
the existing else-if around RunScriptsFromFilesParallel to include this
additional flag check (referencing GetJobCount, RunScriptsFromFilesParallel,
PrintTestResults and the exit-on-first-failure flag and FailedCount).
In `@units/Goccia.Builtins.GlobalPromise.pas`:
- Around line 56-57: The threadvar FStaticMembers (and the same pattern in the
other builtins) holds a managed dynamic array and leaks because Free Pascal
doesn't finalize managed threadvar data on thread exit; fix by removing managed
types from thread-local storage and adding explicit cleanup: either (preferred)
replace the threadvar FStaticMembers with a thread-safe singleton/cache or
instance-scoped field (e.g., a TGocciaBuiltinCache that exposes GetStaticMembers
per-thread) so managed arrays are owned and finalized by a regular object, or
(quick patch) register a thread-exit cleanup hook that sets FStaticMembers :=
nil for each affected unit (GlobalPromise/FStaticMembers and the other listed
builtin units) before the thread exits; apply the same consistent change to all
17 affected units.
In `@units/Goccia.Builtins.GlobalRegExp.pas`:
- Around line 68-70: GRegExpPrototype is a shared global causing races between
parallel workers; change its declaration from a plain var to a thread-local
variable so each thread has its own prototype. Update the declaration of
GRegExpPrototype in units/Goccia.RegExp.Runtime.pas to use threadvar instead of
var, leaving SetRegExpPrototype and CreateRegExpObject behavior intact so each
thread sets/reads its own GRegExpPrototype and avoids cross-thread prototype
overwrites. Ensure any references to GRegExpPrototype elsewhere compile
unchanged.
In `@units/Goccia.Coverage.pas`:
- Around line 406-422: Worker threads never initialize or merge coverage because
TGocciaCoverageTracker is a threadvar and workers don't call Initialize; update
TGocciaFileWorker.Execute (after InitThreadRuntime) to call
TGocciaCoverageTracker.Initialize and enable the tracker for that thread, and
ensure Shutdown is called on worker exit; add a per-worker export method (e.g.,
TGocciaCoverageTracker.ExportCoverage or similar) that serializes the worker's
file/line hit data and registered source-file metadata before shutdown, send or
store that export for the main thread, and implement a corresponding
merge/import routine on the main thread to merge worker exports into the main
tracker (avoiding GetOrCreateFile(..., 0) fallback by preserving original file
metadata such as executable-line counts and IDs during export/import); ensure
RegisterSourceFile and all Record* paths use the per-thread tracker when present
and that merged files reconcile counts and hits into the main
TGocciaCoverageTracker.Instance before final reporting.
In `@units/Goccia.ObjectModel.Engine.pas`:
- Around line 113-115: The current guard prevents rebinding
prototype.constructor when prototypes are shared across worker runs: remove the
conditional check and always set PROP_CONSTRUCTOR after ReplacePrototype so each
engine run resets the constructor reference; specifically, change the logic
around AConstructor.ReplacePrototype(ATypeDefinition.PrototypeProvider()) /
AConstructor.Prototype.HasProperty(PROP_CONSTRUCTOR) so that
AConstructor.Prototype.AssignProperty(PROP_CONSTRUCTOR, AConstructor) is invoked
unconditionally (or use an explicit replace method) to ensure
prototype.constructor points to the current AConstructor every run.
In `@units/Goccia.Threading.pas`:
- Around line 252-259: When a worker thread has a fatal exception we must not
copy its uninitialized default Result slots into the shared FResults (they have
Index = 0), so change the loop around Workers[I].Results: either skip the whole
copy when Assigned(Workers[I].FatalException) is true, or if you still want to
preserve any partial results copy only entries that are initialized (e.g. check
Results[ResultIdx].Index <> 0 or another sentinel) before assigning into
FResults; update the code paths around Workers, FatalException, Results and
FResults so only valid per-file results are propagated.
In `@units/Goccia.Timeout.pas`:
- Around line 26-29: The thread-local timeout variables (GTimeoutMilliseconds,
GStartNanoseconds, GCheckCounter) are never initialized on worker threads
because StartExecutionTimeout is only called on the main thread; update each
worker to initialize per-thread timeout state by invoking StartExecutionTimeout
at the start of TestWorkerProc, ScriptWorkerProc, and BenchmarkWorkerProc (or
alternatively replace the threadvar design with a shared, synchronized timeout
structure and adjust CheckExecutionTimeout accordingly) so that
CheckExecutionTimeout in worker threads reads a properly initialized timeout
state.
In `@units/Goccia.Values.FunctionBase.pas`:
- Around line 257-259: The threadvar FSharedPrototype is assigned to Self too
early and must be cleared if construction/registration fails; modify
TGocciaFunctionBase.Create (and any code paths that set FSharedPrototype) to set
FSharedPrototype := Self only temporarily and ensure a try..except/try..finally
clears FSharedPrototype when an exception occurs (or when construction did not
complete successfully), so that a half-constructed instance isn't left for later
calls to TGocciaFunctionBase.Create or SetSharedPrototypeParent to reuse; in
short, wrap the post-assignment registration steps and on error reset
FSharedPrototype to nil.
In `@units/Goccia.Values.SymbolValue.pas`:
- Around line 85-88: The prototype cache was made thread-local
(FSharedPrototype/FMethodHost) but the symbol system remains process-global, so
concurrent TGocciaSymbolValue.Create calls can race on
GNextSymbolId/GSymbolRegistry and well-known symbol singletons; fix by making
the symbol registry and well-known symbol singletons thread-local (threadvar) or
serialize access with a global critical section around all accesses/updates to
GNextSymbolId and GSymbolRegistry and when initializing well-known
TGocciaSymbolValue instances, update TGocciaSymbolValue.Create and any registry
lookup/registration paths to use the chosen protection (thread-local registry or
lock) and ensure proper per-thread cleanup/initialization for the well-known
symbols to avoid dangling pointers after a worker GC shutdown.
---
Outside diff comments:
In `@units/Goccia.SharedPrototype.pas`:
- Around line 51-62: TGocciaSharedPrototype.ExposeOnConstructor currently skips
writing PROP_CONSTRUCTOR if it's already present, which lets the first engine on
a thread permanently own prototype.constructor; change ExposeOnConstructor so it
always refreshes the constructor on FPrototype (i.e., remove the conditional
HasProperty check and unconditionally call
FPrototype.AssignProperty(PROP_CONSTRUCTOR, AConstructor) after handling
TGocciaClassValue/TGocciaObjectValue) so every expose updates the shared
prototype.constructor to the current AConstructor.
In `@units/Goccia.Values.ArrayValue.pas`:
- Around line 327-336: The code currently skips setting
Array.prototype.constructor if FSharedArrayPrototype already has
PROP_CONSTRUCTOR, which leaves a stale constructor from a previous engine; in
TGocciaArrayValue.ExposePrototype always (re)assign PROP_CONSTRUCTOR on
FSharedArrayPrototype to the supplied AConstructor so the prototype is kept in
sync with the current engine — replace the conditional HasProperty check and
ensure you use the appropriate assign/replace method (e.g., AssignProperty or a
ReplaceProperty overload) to overwrite PROP_CONSTRUCTOR with AConstructor.
---
Nitpick comments:
In `@units/Goccia.Benchmark.Reporter.pas`:
- Around line 108-111: TBenchmarkReporter.GetFileResult currently returns
FFiles[AIndex] without validation; add an explicit bounds check against
Length(FFiles) (or Low/High) and raise a clear exception (e.g., ERangeError or a
custom exception) when AIndex is out of range so callers fail deterministically
instead of producing undefined behavior; update TBenchmarkReporter.GetFileResult
to validate AIndex before accessing FFiles.
In `@units/Goccia.Builtins.GlobalObject.pas`:
- Around line 68-70: FStaticMembers is declared as a threadvar but is only
written in the constructor and immediately consumed, so remove the thread-local
field and use a local variable instead: delete the threadvar FStaticMembers
declaration and in the constructor (where FStaticMembers is currently assigned)
introduce a local variable (e.g., LocalMembers: TArray<TGocciaMemberDefinition>)
to build the TGocciaMemberDefinition array, then pass/consume that local array
in the same places the constructor currently uses FStaticMembers; also update
any code in the 133–168 region that references FStaticMembers to use the local
variable or the consumer call so no thread-local state remains.
In `@units/Goccia.Builtins.JSONL.pas`:
- Around line 52-54: The threadvar FStaticMembers is unnecessary because it's
only written once and read once within the same initialization scope; remove the
threadvar declaration and instead declare a local variable (e.g., LocalMembers:
TArray<TGocciaMemberDefinition>) in the routine that currently assigns/uses
FStaticMembers (the constructor/initializer where FStaticMembers is set at the
assignment site and read later). Replace references to FStaticMembers with the
local variable (use the local while building the member definitions) and delete
the threadvar/FStaticMembers declaration so the array lifetime is limited to the
routine.
In `@units/Goccia.Builtins.Math.pas`:
- Around line 81-82: Remove the threadvar FStaticMembers and instead declare a
local TArray<TGocciaMemberDefinition> variable (e.g., Definitions) inside the
constructor where FStaticMembers is assigned and consumed (the code around the
current assignments at the TGocciaBuiltinsMath constructor). Replace uses of
FStaticMembers in that constructor with the local Definitions variable, ensure
you populate Definitions the same way as before, and verify there are no other
references to FStaticMembers elsewhere (if there are, convert them to use a
local or refactor accordingly); this keeps thread-safety without storing a
managed array in thread-local storage.
In `@units/Goccia.Values.NumberObjectValue.pas`:
- Around line 51-55: FPrototypeMembers should not be a threadvar because it’s
rebuilt each time and left stale when early exits occur; keep
FSharedNumberPrototype and FPrototypeMethodHost as thread-local but remove
FPrototypeMembers from the threadvar block and instead declare it as a local
variable inside the routine that constructs the number prototype (the code that
currently populates FPrototypeMembers in the
TGocciaNumberObjectValue/TGocciaObjectValue prototype init). Update the
constructor/initializer to use the new local TArray<TGocciaMemberDefinition>
variable when building members (and free/leave scope on exit), and remove
references to the global FPrototypeMembers; apply the same change to the
adjacent prototype-building code paths noted (the 90-118 region) so no shared
persistent array remains.
In `@units/Goccia.Values.TemporalPlainMonthDay.pas`:
- Around line 56-58: FPrototypeMembers is a stale threadvar cache causing the
prototype init to only build once; change FPrototypeMembers from a threadvar
field into a local variable used inside the prototype-initialization routine
(where TGocciaSharedPrototype/FShared is referenced) so the member-definition
array is constructed on each init-path instead of being kept thread-local.
Locate usages of FPrototypeMembers and the prototype setup function that
currently reads/sets it (references: FPrototypeMembers, FShared,
TGocciaSharedPrototype, TGocciaMemberDefinition) and move the array declaration
into that routine, update assignments/usages to the local variable, and remove
the threadvar declaration to simplify and ensure fresh member-definition
construction (apply same change pattern to the other similar block noted around
the prototype init).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a76e37ae-0269-4d8b-84da-dd3eeb1c2bef
📒 Files selected for processing (65)
BenchmarkRunner.dprREPL.dprScriptLoader.dprTestRunner.dprunits/Goccia.Benchmark.Reporter.pasunits/Goccia.Builtins.Console.pasunits/Goccia.Builtins.GlobalArray.pasunits/Goccia.Builtins.GlobalArrayBuffer.pasunits/Goccia.Builtins.GlobalFFI.pasunits/Goccia.Builtins.GlobalNumber.pasunits/Goccia.Builtins.GlobalObject.pasunits/Goccia.Builtins.GlobalPromise.pasunits/Goccia.Builtins.GlobalReflect.pasunits/Goccia.Builtins.GlobalRegExp.pasunits/Goccia.Builtins.GlobalString.pasunits/Goccia.Builtins.GlobalSymbol.pasunits/Goccia.Builtins.GlobalURL.pasunits/Goccia.Builtins.Globals.pasunits/Goccia.Builtins.JSON.pasunits/Goccia.Builtins.JSON5.pasunits/Goccia.Builtins.JSONL.pasunits/Goccia.Builtins.Math.pasunits/Goccia.Builtins.Performance.pasunits/Goccia.Builtins.TOML.pasunits/Goccia.Builtins.YAML.pasunits/Goccia.CLI.Application.pasunits/Goccia.CallStack.pasunits/Goccia.Coverage.pasunits/Goccia.GarbageCollector.pasunits/Goccia.MicrotaskQueue.pasunits/Goccia.ObjectModel.Engine.pasunits/Goccia.ObjectModel.pasunits/Goccia.Profiler.pasunits/Goccia.SharedPrototype.pasunits/Goccia.Threading.Init.pasunits/Goccia.Threading.pasunits/Goccia.Timeout.pasunits/Goccia.Values.ArrayBufferValue.pasunits/Goccia.Values.ArrayValue.pasunits/Goccia.Values.BooleanObjectValue.pasunits/Goccia.Values.FFILibrary.pasunits/Goccia.Values.FFIPointer.pasunits/Goccia.Values.FunctionBase.pasunits/Goccia.Values.IteratorValue.pasunits/Goccia.Values.MapValue.pasunits/Goccia.Values.NumberObjectValue.pasunits/Goccia.Values.ObjectValue.pasunits/Goccia.Values.PromiseValue.pasunits/Goccia.Values.SetValue.pasunits/Goccia.Values.SharedArrayBufferValue.pasunits/Goccia.Values.StringObjectValue.pasunits/Goccia.Values.SymbolValue.pasunits/Goccia.Values.TemporalDuration.pasunits/Goccia.Values.TemporalInstant.pasunits/Goccia.Values.TemporalPlainDate.pasunits/Goccia.Values.TemporalPlainDateTime.pasunits/Goccia.Values.TemporalPlainMonthDay.pasunits/Goccia.Values.TemporalPlainTime.pasunits/Goccia.Values.TemporalPlainYearMonth.pasunits/Goccia.Values.TemporalZonedDateTime.pasunits/Goccia.Values.TextDecoderValue.pasunits/Goccia.Values.TextEncoderValue.pasunits/Goccia.Values.TypedArrayValue.pasunits/Goccia.Values.URLSearchParamsValue.pasunits/Goccia.Values.URLValue.pas
Comment on lines
+692
to
+707
| procedure TScriptLoaderApp.ScriptWorkerProc(const AFileName: string; | ||
| const AIndex: Integer; out AConsoleOutput: string; | ||
| out AErrorMessage: string; AData: Pointer); | ||
| begin | ||
| AConsoleOutput := ''; | ||
| AErrorMessage := ''; | ||
| try | ||
| RunScriptFromFile(AFileName); | ||
| except | ||
| on E: Exception do | ||
| begin | ||
| AErrorMessage := E.Message; | ||
| ExitCode := 1; | ||
| end; | ||
| end; | ||
| end; |
Contributor
There was a problem hiding this comment.
Don't run RunScriptFromFile directly inside the worker.
RunSource prints results/warnings/errors itself and catches most script exceptions, so this worker still writes from background threads while leaving AConsoleOutput / AErrorMessage mostly empty. In parallel runs that makes stdout ordering nondeterministic and prevents RunScriptsParallel from reporting failures in file order on the main thread.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ScriptLoader.dpr` around lines 692 - 707, The worker should not call
RunScriptFromFile directly because that lets the script runtime write to stdout
from background threads; instead modify TScriptLoaderApp.ScriptWorkerProc to
invoke RunSource (not RunScriptFromFile), capture its returned console text and
any error text into AConsoleOutput and AErrorMessage, and propagate failures by
setting ExitCode = 1 when RunSource indicates an error or raises; locate
ScriptWorkerProc and replace the direct RunScriptFromFile call with a call to
RunSource (or the function that returns printed output/errors), assign its
outputs to AConsoleOutput/AErrorMessage, and ensure exceptions are caught and
marshaled into AErrorMessage so RunScriptsParallel on the main thread can report
failures in order.
Owner
Author
There was a problem hiding this comment.
Valid concern — RunScriptFromFile does call WriteLn internally. For this initial implementation, interleaved stdout on parallel script execution is an accepted trade-off since ScriptLoader parallel mode is only used for directory inputs where output ordering is less critical. A proper fix would require refactoring the entire RunSource/ExecuteInterpreted chain to support output capture, which is a larger change. Filed as a follow-up.
Contributor
There was a problem hiding this comment.
@frostney, understood — thanks for the context. The trade-off makes sense given that directory-mode parallel execution is the only trigger and strict output ordering is less critical there. I'll keep this in mind for future reviews.
✏️ Learnings added
Learnt from: frostney
Repo: frostney/GocciaScript PR: 306
File: ScriptLoader.dpr:692-707
Timestamp: 2026-04-15T06:26:10.118Z
Learning: In frostney/GocciaScript (ScriptLoader.dpr), `TScriptLoaderApp.ScriptWorkerProc` intentionally calls `RunScriptFromFile` directly in worker threads, which causes interleaved/nondeterministic stdout in parallel directory runs. This is an accepted trade-off for the initial parallel implementation — ScriptLoader parallel mode is only used for directory inputs where output ordering is less critical. A proper fix (refactoring `RunSource`/`ExecuteInterpreted` to support output capture and returning text via `AConsoleOutput`/`AErrorMessage`) has been deferred to a follow-up. Do not re-flag the direct `RunScriptFromFile` call in `ScriptWorkerProc` as a regression or issue until the output-capture refactor lands.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: ScriptLoader.dpr:724-737
Timestamp: 2026-04-14T06:46:14.795Z
Learning: In frostney/GocciaScript (ScriptLoader.dpr), `--emit` with `--output=<file>` and multiple input paths or a directory input silently overwrites the same output file for each compiled module. This is pre-existing behavior preserved across the CLI refactor in PR `#299` and should NOT be re-flagged as a regression. A follow-up guard (raising TGocciaParseError when FEmit.Present and FOutputPath.Present and APaths.Count > 1 or the single path is a directory) was proposed but deferred.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 201
File: TestRunner.dpr:237-240
Timestamp: 2026-04-07T20:24:01.235Z
Learning: Repo: frostney/GocciaScript — TestRunner.dpr / Goccia.Coverage
The synthetic `runTests({...})` call is appended to `Source` before lexing in both `RunGocciaScriptBytecode` and `RunGocciaScriptInterpreted`. `Lexer.SourceLines` therefore includes this line, and `CountExecutableLines` counts it as +1 executable. Because the engine always executes it, coverage also records +1 hit. The net effect on the percentage is self-consistent (+1/+1) and does NOT skew coverage ratios. `PrintCoverageDetail` reads the original file from disk, so the synthetic line never appears in the detail output. Do not flag this as a coverage accuracy bug.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 289
File: ScriptLoader.dpr:140-156
Timestamp: 2026-04-12T21:19:18.811Z
Learning: For GocciaScript CLI tools, suppress human-readable status/progress WriteLn output when running in JSON output mode. In ScriptLoader (ScriptLoader.dpr), gate any status/progress WriteLn lines on `not GJsonOutput`, because `--output=json` streams the result document directly to stdout (mixing status text would corrupt JSON). In TestRunner and BenchmarkRunner (TestRunner.dpr, BenchmarkRunner.dpr), JSON is written to a file, so current unconditional stdout WriteLn calls are safe, but these tools should be refactored later to follow the same stdout/JSON suppression pattern used by ScriptLoader.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: TestRunner.dpr:132-139
Timestamp: 2026-04-14T06:46:10.919Z
Learning: In frostney/GocciaScript CLI tool sources, the duplicated `EscapeJSONString` helper intentionally escapes only `\`, `"`, `\n`, `\r`, and `\t` (leaving other C0 control characters U+0000–U+001F unescaped). During code review, do not flag missing additional control-character escaping in these per-tool `EscapeJSONString` copies while the shared-helper consolidation/fix PR is not yet landed; only re-check once the implementation has been consolidated into the shared helper and the tracking fix is in place.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: TestRunner.dpr:132-139
Timestamp: 2026-04-14T13:53:18.321Z
Learning: For GocciaScript CLI tool sources in this repo, when you see duplicated `EscapeJSONString` helpers that only escape `\`, `"`, `\n`, `\r`, and `\t` (leaving other C0 control characters U+0000–U+001F unescaped) in individual per-tool `.dpr` files, do not re-flag this as a review issue while the consolidation/fix tracked in issue `#303` has not landed. After the PR resolving `#303` is merged, start reviewing these helpers for full control-character JSON escaping again.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 289
File: ScriptLoader.dpr:649-650
Timestamp: 2026-04-12T21:08:07.595Z
Learning: In frostney/GocciaScript (ScriptLoader.dpr, units/Goccia.Engine.pas): `--source-map` is valid in interpreted mode when JSX preprocessing is active. Source maps are produced by the JSX pre-pass inside `ParseSource`/`TGocciaEngine`, not solely by the bytecode pipeline. `ExecuteInterpreted` needs to surface the engine's source map (via a public `SourceMap` property on `TGocciaEngine`) and pass it to `WriteSourceMapIfEnabled` — do not reject `--source-map` in interpreted mode.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 200
File: REPL.dpr:0-0
Timestamp: 2026-04-07T17:12:07.324Z
Learning: When reviewing timing/metrics code in the GocciaScript repo (e.g., REPL.dpr, ScriptLoader, TestRunner, BenchmarkRunner, and TGocciaEngine.Execute), follow the established timing convention: capture `StartTime` before the JSX pre-pass (`TGocciaJSXTransformer.Transform`) and `StringListToLFText`, then capture `LexEnd` after `ScanTokens`. The `Lex` duration (`LexEnd - StartTime`) is intended to include the JSX pre-pass because it is treated as part of the source→tokens pipeline. Do not flag this ordering as a bug and do not recommend moving `StartTime` to after the JSX transform, as it would break the bucket semantics.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: ScriptLoader.dpr:727-740
Timestamp: 2026-04-14T13:51:07.273Z
Learning: In frostney/GocciaScript (ScriptLoader.dpr), the `--emit` flag (bytecode compilation and output to a `.gbc` file) is an exploratory/temporary feature added to validate the bytecode pipeline. It is NOT the long-term solution for bytecode output. A proper bundling mechanism is being designed separately and tracked in issue `#156`. Do not flag `--emit` multi-file overwrite behavior or the presence of bytecode emission logic in ScriptLoader as architectural problems — the entire emit path is expected to be replaced by the bundler. ScriptLoader doing bytecode output is acknowledged as "too much responsibility" by the maintainer.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 107
File: units/Goccia.Runtime.Operations.pas:211-212
Timestamp: 2026-03-23T12:37:30.936Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas (PR `#107`)
Learning: In TGocciaRuntimeOperations.MarkExternalRoots, entries in FModuleCache (cached module namespace/JSON records) must be traversed and their TSouffleValue references marked as external roots to survive GC. This behavior was added in commit 36f69cc; do not re‑flag missing FModuleCache rooting unless the logic regresses.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 209
File: units/Goccia.FFI.DynamicLibrary.pas:56-63
Timestamp: 2026-04-08T13:55:35.057Z
Learning: In frostney/GocciaScript (units/Goccia.Values.FFILibrary.pas, units/Goccia.FFI.DynamicLibrary.pas), `TGocciaFFIBoundFunction.Call` intentionally does not check whether the owning `TGocciaFFILibraryHandle` is closed before invoking `FFIDispatchCall(FSymbol, ...)`. Bound functions capture a raw `CodePointer` at bind-time; after `Close()` unloads the module, those pointers become dangling. This is an acknowledged v1 limitation — the library handle lifetime vs. bound-function lifetime is not enforced in v1. Do not re-flag this use-after-close hazard until a v2 tracking issue is opened and addressed.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:38:51.694Z
Learning: Applies to **/*.pas : Use `Goccia.FileExtensions` constants (`EXT_JS`, `EXT_JSX`, `EXT_TS`, `EXT_TSX`, `EXT_MJS`, `EXT_JSON`, `EXT_SBC`) instead of hardcoded string literals. Use `ScriptExtensions` array, `IsScriptExtension`, and `IsJSXNativeExtension` helpers instead of duplicating extension lists.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 147
File: units/Goccia.VM.pas:3042-3084
Timestamp: 2026-03-31T18:38:23.367Z
Learning: Repo: frostney/GocciaScript — units/Goccia.VM.pas (PR `#147`)
As of commit 457e712: (1) TGocciaVM.GetPropertyValue performs own-only private static accessor lookup on '#…' for TGocciaClassValue and throws TypeError on class-brand misses; (2) TGocciaVM.SetPropertyValue avoids inherited private accessor resolution, but still allows raw private static writes via SetRawPrivateValue to support current class initialization that emits OP_SET_PROP_CONST for private static fields. Planned follow-up is to add dedicated opcodes to define private static fields at class-declaration time and then make SetPropertyValue strict (throw on brand miss with no fallback). Do not re-flag the current fallback until those opcodes exist.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to **/*.{pas,dpr} : Use file extension constants from `Goccia.FileExtensions` (`EXT_JS`, `EXT_JSX`, `EXT_TS`, `EXT_TSX`, `EXT_MJS`, `EXT_JSON`) instead of hardcoded string literals. Use the `ScriptExtensions` array, `IsScriptExtension`, and `IsJSXNativeExtension` helpers instead of duplicating extension lists or ad-hoc checks.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/ScopeMap.pas:0-0
Timestamp: 2026-03-17T22:18:02.668Z
Learning: Repo: frostney/GocciaScript — ScopeMap.pas (units/ScopeMap.pas) was deleted (commit d99cf96) after profiling showed TScopeMap's linear-scan IndexOf was ~2.7× slower than hash-based lookup (CreateBridgedContext samples rose from 24% to 51%). Scope bindings now use TOrderedStringMap<TLexicalBinding> (DJB2 hash, O(1) per level); scope chain walking is done via recursive TGocciaScope-level methods (Contains/GetLexicalBinding) rather than a parent-pointer chain inside the map. Do not flag TScopeMap or its Parent property in future reviews — the type no longer exists.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 286
File: docs/testing.md:774-776
Timestamp: 2026-04-12T15:44:39.321Z
Learning: In frostney/GocciaScript TestRunner.dpr, both interpreted (`RunGocciaScriptInterpreted` via `TGocciaEngine`) and bytecode (`RunGocciaScriptBytecode` via `TGocciaBytecodeBackend`) paths use the same globals set `[ggTestAssertions, ggFFI]`. `TGocciaBytecodeBackend.RegisterBuiltIns` accepts the same `TGocciaGlobalBuiltins` type. Documentation stating "creates a fresh `TGocciaEngine` with `[ggTestAssertions, ggFFI]`" is intentionally concise and accurate for both modes — do not flag it as incomplete for omitting the bytecode backend distinction.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 139
File: units/Goccia.ScriptLoader.JSON.Test.pas:1-113
Timestamp: 2026-03-30T13:02:33.857Z
Learning: In frostney/GocciaScript, CI workflows (`.github/workflows/ci.yml` and `.github/workflows/pr.yml`) use glob patterns (`build/Goccia.*.Test*` for artifact upload, `Goccia.*.Test` / `Goccia.*.Test.exe` for execution) to discover and run native Pascal test binaries automatically. Do not flag missing entries in a hard-coded test list — new `units/*.Test.pas` test programs are picked up automatically via these globs as of commit b63911d (PR `#139`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 60
File: units/Goccia.Runtime.Operations.pas:8694-8696
Timestamp: 2026-03-10T18:05:46.926Z
Learning: Repo: frostney/GocciaScript PR: 60 — units/Goccia.Runtime.Operations.pas: The GC rooting of FArrayBridgeReverse (marking values in MarkExternalRoots and keys in MarkWrappedGocciaValues) is intentional to preserve array identity across async/microtask boundaries (e.g., Promise.resolve(arr).then(v => v === arr)). Do not prune this map during normal clearing; per-file GC.Collect in runners bounds cross-file memory growth.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 222
File: .github/workflows/ci.yml:562-572
Timestamp: 2026-04-08T14:58:47.163Z
Learning: In frostney/GocciaScript, the `test262-conformance` CI job (`.github/workflows/ci.yml`) is intentionally non-blocking: it uses `|| true` on the runner step and `raise SystemExit(0)` when no report is produced, so it never fails the job. It is deliberately excluded from the `needs` list of both `artifacts` and `release` jobs. Do not flag the `|| true` or the `SystemExit(0)` as issues — the job is designed as a purely informational conformance-tracking signal.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 87
File: units/Goccia.Runtime.Operations.pas:3383-3387
Timestamp: 2026-03-17T16:28:05.526Z
Learning: Repo: frostney/GocciaScript — Follow-up tracking
Learning: Splitting the blueprint bridge caches (value bridge vs construct bridge) is tracked in issue `#99`. Until that lands, sharing FBlueprintBridgeCache between UnwrapToGocciaValue (BlueprintToClassValue) and Construct (ConvertBlueprintToClassValue) should not block PRs; reference `#99` instead of re-flagging.
Comment on lines
+56
to
+57
| threadvar | ||
| FStaticMembers: TArray<TGocciaMemberDefinition>; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Free Pascal, are managed threadvar variables such as dynamic arrays (TArray) finalized automatically when a thread exits?
💡 Result:
No, managed threadvar variables such as dynamic arrays (TArray) are not finalized automatically when a thread exits in Free Pascal. Dynamic arrays are managed types, automatically finalized when going out of scope in normal procedure contexts. However, threadvar variables are global and per-thread, not scoped to procedures. Thread termination via EndThread calls DoneThread, which finalizes RTL components including heap and explicitly calls the thread manager's ReleaseThreadVars to release threadvar memory blocks. This releases the storage for the threadvar variables but does not perform managed type finalization (e.g., reference count decrement for dynamic arrays). Evidence from FPC developer discussions confirms DoneThread finalizes RTL parts and releases threadvars, but no mention of calling Finalize on managed threadvar contents. Official docs on managed types describe finalization at scope exit, not threadvar cleanup. Thus, dynamic array contents leak unless manually finalized before thread exit (e.g., set to nil or Finalize(var)).
Citations:
- 1: https://lists.freepascal.org/pipermail/fpc-devel/2010-November/023273.html
- 2: https://www.freepascal.org/docs-html/ref/refse20.html
- 3: https://www.freepascal.org/docs-html/rtl/system/finalize.html
- 4: https://www.freepascal.org/docs-html/rtl/system/tthreadmanager.html
- 5: https://gitlab.com/freepascal.org/fpc/source/-/issues/40838
🏁 Script executed:
# Find the file and show context around lines 56-57
fd -t f "GlobalPromise.pas" --exec wc -l {} \;Repository: frostney/GocciaScript
Length of output: 110
🏁 Script executed:
# Show the file structure and threadvar usage
rg "threadvar|FStaticMembers" units/Goccia.Builtins.GlobalPromise.pas -B 3 -A 10Repository: frostney/GocciaScript
Length of output: 1219
🏁 Script executed:
# Check for any cleanup/finalization patterns for FStaticMembers in the file
rg "FStaticMembers" units/Goccia.Builtins.GlobalPromise.pas -B 2 -A 5Repository: frostney/GocciaScript
Length of output: 599
🏁 Script executed:
# Search other builtins for similar threadvar patterns
rg "threadvar.*FStaticMembers" units/Goccia.Builtins.*.pasRepository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
# Search more broadly for threadvar declarations with dynamic arrays
rg "threadvar" units/Goccia.Builtins.*.pas -A 2Repository: frostney/GocciaScript
Length of output: 3642
🏁 Script executed:
# Check how many times Create is called and understand the lifecycle
rg "TGocciaGlobalPromise.Create|procedure TGocciaGlobalPromise.Create" units/Goccia.Builtins.GlobalPromise.pas -B 5 -A 30 | head -60Repository: frostney/GocciaScript
Length of output: 1367
🏁 Script executed:
# Check if there's any cleanup handler registered for thread termination
rg "DoneThread|ReleaseThreadVars|thread.*cleanup|Finalize.*FStaticMembers" units/Goccia.Builtins.GlobalPromise.pasRepository: frostney/GocciaScript
Length of output: 47
Managed threadvar dynamic arrays leak memory on thread exit without explicit finalization.
FStaticMembers: TArray<TGocciaMemberDefinition> (and the same pattern in 16+ other builtins: YAML, TOML, Math, Performance, JSONL, JSON5, JSON, GlobalURL, GlobalString, GlobalSymbol, GlobalReflect, GlobalRegExp, GlobalObject, GlobalNumber, GlobalFFI, GlobalArray, GlobalArrayBuffer, Console) is a managed type stored in thread-local storage. Free Pascal's thread manager does not automatically finalize managed threadvar contents when a thread exits; ReleaseThreadVars releases the memory block but skips reference-count decrements and deep finalization. This causes memory leaks across thread lifetimes.
Add explicit cleanup:
- Either set
FStaticMembers := nil;before thread exit (requires a finalization callback or thread-pool cleanup hook) - Or use a thread-safe singleton cache or instance-scoped field instead of threadvar
All 17 affected builtins must be remedied consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Builtins.GlobalPromise.pas` around lines 56 - 57, The threadvar
FStaticMembers (and the same pattern in the other builtins) holds a managed
dynamic array and leaks because Free Pascal doesn't finalize managed threadvar
data on thread exit; fix by removing managed types from thread-local storage and
adding explicit cleanup: either (preferred) replace the threadvar FStaticMembers
with a thread-safe singleton/cache or instance-scoped field (e.g., a
TGocciaBuiltinCache that exposes GetStaticMembers per-thread) so managed arrays
are owned and finalized by a regular object, or (quick patch) register a
thread-exit cleanup hook that sets FStaticMembers := nil for each affected unit
(GlobalPromise/FStaticMembers and the other listed builtin units) before the
thread exits; apply the same consistent change to all 17 affected units.
Owner
Author
There was a problem hiding this comment.
Valid finding — FPC does not finalize managed threadvar contents on thread exit. In our usage, each worker thread is short-lived (processes a handful of files then exits), so the leaked TArray<TGocciaMemberDefinition> data per thread is small (a few hundred bytes per unit × ~30 units ≈ a few KB per thread). The GC-managed objects are already cleaned up by ShutdownThreadRuntime.
A proper fix would require either exposing finalization procedures from each of the ~30 affected units (the threadvars are in implementation sections, so not accessible externally) or a registration-based cleanup system. Deferred as a follow-up for when long-lived thread pools are introduced. For the current spawn-and-join model, the OS reclaims all process memory on exit.
Contributor
There was a problem hiding this comment.
@frostney, understood — the trade-off is well-reasoned: a few KB of leaked TArray shell memory per short-lived worker thread is negligible for the current spawn-and-join model, and the GC-managed heap objects are already properly cleaned up by ShutdownThreadRuntime. The real fix (registration-based cleanup or per-unit finalization hooks) makes more sense to build when long-lived thread pools become a concern.
I'll record this so it doesn't get re-flagged in future reviews. Would you like me to open a follow-up issue to track the registration-based cleanup approach for when long-lived thread pools are introduced?
✏️ Learnings added
Learnt from: frostney
Repo: frostney/GocciaScript PR: 306
File: units/Goccia.Builtins.GlobalPromise.pas:56-57
Timestamp: 2026-04-15T06:26:30.835Z
Learning: In frostney/GocciaScript (PR `#306`), ~30 builtin units (e.g., Goccia.Builtins.GlobalPromise.pas, Goccia.Builtins.GlobalArray.pas, Goccia.Builtins.Math.pas, and ~27 others) use `threadvar FStaticMembers: TArray<TGocciaMemberDefinition>` in their `implementation` sections. FPC does NOT finalize managed threadvar contents on thread exit, so the TArray shell (~few hundred bytes per unit) leaks per worker thread. This is an accepted/deferred trade-off: (1) threads are short-lived spawn-and-join workers, (2) the GC-managed TGocciaMemberDefinition objects are cleaned up by ShutdownThreadRuntime, (3) OS reclaims all memory on process exit, (4) a proper fix requires a registration-based cleanup system or per-unit finalization hooks since the threadvars are in implementation sections. Do NOT re-flag this managed-threadvar leak until long-lived thread pools are introduced and a cleanup registry is in place.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: frostney
Repo: frostney/GocciaScript PR: 147
File: units/Goccia.VM.pas:3042-3078
Timestamp: 2026-03-31T17:08:45.603Z
Learning: Repo: frostney/GocciaScript — units/Goccia.VM.pas
Learning: Private accessor handling (including static) is enforced in TGocciaVM:
- In GetPropertyValue, when AKey starts with '#':
- If AObject is TGocciaClassValue, use PrivatePropertyGetter/PrivatePropertySetter. Invoke the getter when present; if only a setter exists, throw TypeError "Private accessor #... was defined without a getter".
- If AObject is TGocciaObjectValue, walk the prototype chain for an own accessor descriptor keyed by the private name; if an accessor exists without a getter, throw the same TypeError. Otherwise, fall back to TryGetRawPrivateValue.
- In SetPropertyValue, symmetric behavior applies:
- For TGocciaClassValue, call the private setter when present; if only a getter exists, throw TypeError "Private accessor #... was defined without a setter".
- For instances, walk the prototype chain; if an accessor descriptor exists without a setter, throw; otherwise SetRawPrivateValue.
This ensures static and instance private accessors round-trip correctly and error on getter-only/setter-only cases.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 147
File: units/Goccia.VM.pas:3042-3084
Timestamp: 2026-03-31T18:38:23.367Z
Learning: Repo: frostney/GocciaScript — units/Goccia.VM.pas (PR `#147`)
As of commit 457e712: (1) TGocciaVM.GetPropertyValue performs own-only private static accessor lookup on '#…' for TGocciaClassValue and throws TypeError on class-brand misses; (2) TGocciaVM.SetPropertyValue avoids inherited private accessor resolution, but still allows raw private static writes via SetRawPrivateValue to support current class initialization that emits OP_SET_PROP_CONST for private static fields. Planned follow-up is to add dedicated opcodes to define private static fields at class-declaration time and then make SetPropertyValue strict (throw on brand miss with no fallback). Do not re-flag the current fallback until those opcodes exist.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 58
File: units/Goccia.Runtime.Operations.pas:5959-6132
Timestamp: 2026-03-10T11:17:46.617Z
Learning: GocciaScript’s Promise statics are intentionally not species/subclass-aware. Interpreter-level implementations (PromiseAll, PromiseRace, PromiseAllSettled, PromiseAny, withResolvers, try, resolve, reject) construct plain TGocciaPromiseValue via WrapAsPromise and do not use the call receiver (AThisValue). The Souffle VM bridge mirrors this by not forwarding AReceiver and by constructing TGocciaPromiseValue directly where applicable. Do not flag missing receiver-based construction for Promise statics. This applies to units/Goccia.Runtime.Operations.pas and related Promise implementations; ensure consistency across related Pas files in the frostney/GocciaScript repository.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 63
File: units/Goccia.Runtime.Operations.pas:7458-7476
Timestamp: 2026-03-11T09:31:51.130Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas
Learning: NativeRecordHasOwnProperty for TSouffleRecord receivers intentionally treats accessor getters as own properties (checks Rec.Has(Key) or (Rec.HasGetters and Rec.Getters.Has(Key))) to mirror interpreter behavior; this ensures performance.hasOwnProperty('timeOrigin') returns true in bytecode mode.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 87
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-17T16:07:07.425Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas
Learning: In TGocciaRuntimeOperations.Construct, the native blueprint instantiation fast-path must be gated by the root blueprint: walk Bp up to WalkBp (the chain root) and only take the native path if not FBlueprintSuperValues.ContainsKey(WalkBp). This prevents derived chains that ultimately extend a wrapped/non-blueprint super from running field initializers and delegate wiring early; those should fall through to the bridge path instead. The delegate wiring loop should iterate WalkBp and only assign Methods.Delegate := FVM.RecordDelegate at the true root.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 87
File: units/Goccia.Runtime.Operations.pas:3383-3387
Timestamp: 2026-03-17T16:08:41.070Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas
Maintainer preference: FBlueprintBridgeCache is intentionally shared between UnwrapToGocciaValue (which uses BlueprintToClassValue) and Construct (which uses ConvertBlueprintToClassValue); this pre‑existing pattern on main is acceptable and should not be re‑flagged in future reviews unless a functional bug is demonstrated.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-05T07:58:33.798Z
Learning: Applies to **/{Builtins,Engine}*.pas : Use Goccia.ObjectModel member-definition tables and RegisterMemberDefinitions instead of ad hoc RegisterNativeMethod blocks when adding prototype surfaces
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : Singleton values (e.g., `UndefinedValue`, `TrueValue`, `NaNValue`, `SmallInt` cache) must be pinned via `TGocciaGarbageCollector.Instance.PinValue` during engine initialization (consolidated in `PinSingletons`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 58
File: units/Goccia.Runtime.Operations.pas:2791-2794
Timestamp: 2026-03-10T09:21:24.467Z
Learning: In frostney/GocciaScript, units/Goccia.Runtime.Operations.pas GetProperty uses delegate-first lookup for built-ins (String, Number, Map, Set, Promise): it queries the respective delegate (e.g., FPromiseDelegate for TGocciaPromiseValue instances; FPromiseStaticDelegate for the Promise constructor) and only falls back to the underlying Goccia object/property lookup if the delegate doesn’t have the key. Built-in prototype methods on shared prototypes are immutable in GocciaScript and are not meant to be shadowed or reassigned; the delegate-first pattern is by design.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : Values held only by Pascal code (not in any GocciaScript scope) must be protected with `AddTempRoot`/`RemoveTempRoot` for the duration they are needed.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 60
File: units/Goccia.Runtime.Operations.pas:8694-8696
Timestamp: 2026-03-10T18:05:46.926Z
Learning: Repo: frostney/GocciaScript PR: 60 — units/Goccia.Runtime.Operations.pas: The GC rooting of FArrayBridgeReverse (marking values in MarkExternalRoots and keys in MarkWrappedGocciaValues) is intentional to preserve array identity across async/microtask boundaries (e.g., Promise.resolve(arr).then(v => v === arr)). Do not prune this map during normal clearing; per-file GC.Collect in runners bounds cross-file memory growth.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 271
File: units/Goccia.Builtins.GlobalObject.pas:104-129
Timestamp: 2026-04-11T19:43:06.627Z
Learning: In frostney/GocciaScript, `TGarbageCollector.AddTempRoot`/`RemoveTempRoot` are set-membership (not reference-counted). In built-in method implementations, ONLY call `AddTempRoot`/`RemoveTempRoot` if the value was freshly boxed by `ToObject` (i.e., the argument was not already a `TGocciaObjectValue`). Do not unconditionally add/remove temp roots for values passed in as method arguments, since a single `RemoveTempRoot(Obj)` will remove the root for all holders and can cause use-after-free on the next GC cycle. Pattern: `if Assigned(TGarbageCollector.Instance) and not (AArgs.GetElement(0) is TGocciaObjectValue) then AddTempRoot(Obj); try ... finally if Assigned(TGarbageCollector.Instance) and not (AArgs.GetElement(0) is TGocciaObjectValue) then RemoveTempRoot(Obj); end;`
Learnt from: frostney
Repo: frostney/GocciaScript PR: 209
File: units/Goccia.Values.FFILibrary.pas:73-82
Timestamp: 2026-04-08T13:55:31.119Z
Learning: In frostney/GocciaScript (units/Goccia.Values.FFILibrary.pas, PR `#209`), `TGocciaFFIBoundFunction` intentionally holds only a raw `CodePointer` with no back-reference to the owning `TGocciaFFILibraryHandle`. Calling `close()` while bound functions or raw symbol pointers still exist is documented UB — the same contract as Deno/Bun FFI. This is an explicit v1 design decision, gated behind the `ggFFI` opt-in flag so only embedders who accept the contract enable it. A ref-counted handle guard is planned as a v2 enhancement. Do NOT re-flag the missing library back-reference or "bind() leaks" in future reviews of this unit.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 60
File: units/Goccia.Runtime.Operations.pas:3631-3642
Timestamp: 2026-03-10T21:38:23.719Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas
In TGocciaRuntimeOperations.GetIterator and FinalizeEnum, AddTempRoot is intentionally not balanced with RemoveTempRoot. Newly created TGocciaIteratorValue/TGocciaEnumValue instances are immediately wrapped via WrapGocciaValue and stored in FWrappedValues; TGocciaRuntimeOperations registers MarkWrappedGocciaValues as an external root marker so the underlying TGocciaValue is traced during GC. The temp root only protects the pre-wrap window; cleaning it up is a separate lifetime refactor and out of scope for PR `#60`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 58
File: units/Goccia.Values.PromiseValue.pas:457-472
Timestamp: 2026-03-10T09:20:08.105Z
Learning: In `units/Goccia.Values.PromiseValue.pas`: `TGocciaPromiseFinallyWrapper` instances (`WrapFulfilled`, `WrapRejected`) created in `TGocciaPromiseValue.InvokeFinally` (and the original `PromiseFinally`) do NOT require explicit `PinValue` calls. They survive GC because: (1) all `TGocciaValue` instances auto-register with the GC via `AfterConstruction`; (2) the `TGocciaNativeFunctionValue` handlers wrapping their `Invoke` methods are stored in promise reactions (`FReactions`) or microtask queue entries; (3) `TGocciaPromiseValue.MarkReferences` marks all reactions and their `OnFulfilled`/`OnRejected` handlers. Do NOT flag missing pinning on these wrapper objects as a GC issue.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 107
File: units/Goccia.Runtime.Operations.pas:211-212
Timestamp: 2026-03-23T12:37:30.936Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas (PR `#107`)
Learning: In TGocciaRuntimeOperations.MarkExternalRoots, entries in FModuleCache (cached module namespace/JSON records) must be traversed and their TSouffleValue references marked as external roots to survive GC. This behavior was added in commit 36f69cc; do not re‑flag missing FModuleCache rooting unless the logic regresses.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 60
File: units/GarbageCollector.Generic.pas:115-123
Timestamp: 2026-03-10T17:59:00.467Z
Learning: In units/GarbageCollector.Generic.pas (TGarbageCollector.Destroy): FManagedObjects is intentionally created with OwnsObjects = False and the destructor does NOT iterate and free remaining managed objects. This is pre-existing behavior (carried over from TGocciaGarbageCollector). Shutdown is only called at process exit (OS reclaims memory) or between engine instances (engine destructor chain frees values via scope teardown). Objects still present at shutdown are typically pinned singletons with process lifetime. Do NOT flag missing per-object freeing in this destructor as a memory leak issue.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 209
File: units/Goccia.FFI.DynamicLibrary.pas:56-63
Timestamp: 2026-04-08T13:55:35.057Z
Learning: In frostney/GocciaScript (units/Goccia.Values.FFILibrary.pas, units/Goccia.FFI.DynamicLibrary.pas), `TGocciaFFIBoundFunction.Call` intentionally does not check whether the owning `TGocciaFFILibraryHandle` is closed before invoking `FFIDispatchCall(FSymbol, ...)`. Bound functions capture a raw `CodePointer` at bind-time; after `Close()` unloads the module, those pointers become dangling. This is an acknowledged v1 limitation — the library handle lifetime vs. bound-function lifetime is not enforced in v1. Do not re-flag this use-after-close hazard until a v2 tracking issue is opened and addressed.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 267
File: units/Goccia.Parser.pas:949-961
Timestamp: 2026-04-11T09:44:39.519Z
Learning: In frostney/GocciaScript (units/Goccia.AST.Node.pas, units/Goccia.AST.Statements.pas): `TGocciaProgram` has no destructor and does NOT free its `FBody: TObjectList<TGocciaStatement>` when `Free` is called. `TGocciaExpressionStatement` similarly has no destructor and does NOT free its `FExpression: TGocciaExpression`. Therefore, extracting a child expression from a temporary `TGocciaProgram` (e.g., via `TGocciaExpressionStatement(ProgramNode.Body[0]).Expression`) and then calling `ProgramNode.Free` is safe — the extracted expression is NOT freed and remains valid. This pattern is intentional and is used in `ParseTaggedTemplate` and similar parsing helpers. Do NOT flag this as a dangling-pointer or use-after-free issue.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 209
File: units/Goccia.Builtins.GlobalFFI.pas:0-0
Timestamp: 2026-04-08T15:02:36.765Z
Learning: In frostney/GocciaScript (units/Goccia.Builtins.GlobalFFI.pas, PR `#209`), `FFI.alloc` and `FFI.free` were removed in a later commit. User-owned FFI memory is now managed through `new ArrayBuffer(size)` and TypedArray views rather than a dedicated FFI allocation API. Do not flag `GetMem`/`EOutOfMemory` or `Double→Integer` overflow issues in `FFIAlloc`; the method no longer exists in the FFI surface.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 60
File: souffle/Souffle.VM.pas:96-104
Timestamp: 2026-03-10T17:58:57.247Z
Learning: In souffle/Souffle.VM.pas, TSouffleVM caches TGarbageCollector.Instance in FGC during construction. This is safe because TGocciaSouffleBackend.Destroy always frees the VM (TSouffleVM.Destroy) before calling GC.Shutdown. The enforced lifecycle order is: GC.Initialize → VM.Create → ... → VM.Destroy → Backend.Destroy → GC.Shutdown. Do NOT flag the cached FGC field as a dangling-pointer risk — re-querying TGarbageCollector.Instance in the destructor would add overhead without addressing an actual bug.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 45
File: units/Goccia.ControlFlow.pas:13-18
Timestamp: 2026-03-07T23:37:29.010Z
Learning: In `units/Goccia.ControlFlow.pas`: `TGocciaControlFlow.Value` (a `TGocciaValue` held in a plain record) is intentionally left unrooted. CF propagation is synchronous — no GC-triggering allocations occur between production and consumption of the value — so there is no live-value collection window. Do NOT flag missing `AddTempRoot`/`RemoveTempRoot` on `TGocciaControlFlow` fields; adding managed-record operators (Initialize/Finalize) would add per-creation/destruction overhead that contradicts the performance goal of this refactoring (replacing exception-based control flow with lightweight records).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/ScopeMap.pas:0-0
Timestamp: 2026-03-17T22:18:02.668Z
Learning: Repo: frostney/GocciaScript — ScopeMap.pas (units/ScopeMap.pas) was deleted (commit d99cf96) after profiling showed TScopeMap's linear-scan IndexOf was ~2.7× slower than hash-based lookup (CreateBridgedContext samples rose from 24% to 51%). Scope bindings now use TOrderedStringMap<TLexicalBinding> (DJB2 hash, O(1) per level); scope chain walking is done via recursive TGocciaScope-level methods (Contains/GetLexicalBinding) rather than a parent-pointer chain inside the map. Do not flag TScopeMap or its Parent property in future reviews — the type no longer exists.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : `type`/`interface` declarations and `import type`/`export type` must produce `TGocciaEmptyStatement` (no-op at runtime). Access modifiers (`public`, `protected`, `private`, `readonly`, `override`, `abstract`) in class bodies must be consumed and discarded.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 306
File: ScriptLoader.dpr:692-707
Timestamp: 2026-04-15T06:26:10.118Z
Learning: In frostney/GocciaScript (ScriptLoader.dpr), `TScriptLoaderApp.ScriptWorkerProc` intentionally calls `RunScriptFromFile` directly in worker threads, which causes interleaved/nondeterministic stdout in parallel directory runs. This is an accepted trade-off for the initial parallel implementation — ScriptLoader parallel mode is only used for directory inputs where output ordering is less critical. A proper fix (refactoring `RunSource`/`ExecuteInterpreted` to support output capture and returning text via `AConsoleOutput`/`AErrorMessage`) has been deferred to a follow-up. Do not re-flag the direct `RunScriptFromFile` call in `ScriptWorkerProc` as a regression or issue until the output-capture refactor lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 139
File: units/Goccia.ScriptLoader.JSON.Test.pas:1-113
Timestamp: 2026-03-30T13:02:33.857Z
Learning: In frostney/GocciaScript, CI workflows (`.github/workflows/ci.yml` and `.github/workflows/pr.yml`) use glob patterns (`build/Goccia.*.Test*` for artifact upload, `Goccia.*.Test` / `Goccia.*.Test.exe` for execution) to discover and run native Pascal test binaries automatically. Do not flag missing entries in a hard-coded test list — new `units/*.Test.pas` test programs are picked up automatically via these globs as of commit b63911d (PR `#139`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: ScriptLoader.dpr:724-737
Timestamp: 2026-04-14T06:46:14.795Z
Learning: In frostney/GocciaScript (ScriptLoader.dpr), `--emit` with `--output=<file>` and multiple input paths or a directory input silently overwrites the same output file for each compiled module. This is pre-existing behavior preserved across the CLI refactor in PR `#299` and should NOT be re-flagged as a regression. A follow-up guard (raising TGocciaParseError when FEmit.Present and FOutputPath.Present and APaths.Count > 1 or the single path is a directory) was proposed but deferred.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: TestRunner.dpr:132-139
Timestamp: 2026-04-14T13:53:18.321Z
Learning: In frostney/GocciaScript CLI tool sources, the duplicated `EscapeJSONString` helper (escaping only `\`, `"`, `\n`, `\r`, `\t`, leaving other C0 control characters U+0000–U+001F unescaped) is tracked for consolidation into a shared helper and full fix in issue `#303`. During code review, do not re-flag missing control-character escaping in individual per-tool `EscapeJSONString` copies (e.g., TestRunner.dpr, BenchmarkRunner.dpr) until the consolidation PR landing issue `#303` is resolved.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 289
File: ScriptLoader.dpr:140-156
Timestamp: 2026-04-12T21:19:18.811Z
Learning: In frostney/GocciaScript (ScriptLoader.dpr, TestRunner.dpr, BenchmarkRunner.dpr): suppressing status/progress WriteLn messages in JSON output mode is intentionally handled per-tool for now. The long-term goal is to unify the stdout/JSON-output suppression pattern across all CLI tools. In ScriptLoader, status lines must be gated on `not GJsonOutput` because `--output=json` routes the result document directly to stdout. TestRunner and BenchmarkRunner write JSON to a file, so their current unconditional WriteLn to stdout is safe but will need to align with the unified pattern in a future refactor.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 200
File: REPL.dpr:0-0
Timestamp: 2026-04-07T17:12:07.324Z
Learning: When reviewing timing/metrics code in the GocciaScript repo (e.g., REPL.dpr, ScriptLoader, TestRunner, BenchmarkRunner, and TGocciaEngine.Execute), follow the established timing convention: capture `StartTime` before the JSX pre-pass (`TGocciaJSXTransformer.Transform`) and `StringListToLFText`, then capture `LexEnd` after `ScanTokens`. The `Lex` duration (`LexEnd - StartTime`) is intended to include the JSX pre-pass because it is treated as part of the source→tokens pipeline. Do not flag this ordering as a bug and do not recommend moving `StartTime` to after the JSX transform, as it would break the bucket semantics.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 201
File: TestRunner.dpr:237-240
Timestamp: 2026-04-07T20:24:01.235Z
Learning: Repo: frostney/GocciaScript — TestRunner.dpr / Goccia.Coverage
The synthetic `runTests({...})` call is appended to `Source` before lexing in both `RunGocciaScriptBytecode` and `RunGocciaScriptInterpreted`. `Lexer.SourceLines` therefore includes this line, and `CountExecutableLines` counts it as +1 executable. Because the engine always executes it, coverage also records +1 hit. The net effect on the percentage is self-consistent (+1/+1) and does NOT skew coverage ratios. `PrintCoverageDetail` reads the original file from disk, so the synthetic line never appears in the detail output. Do not flag this as a coverage accuracy bug.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 222
File: .github/workflows/ci.yml:562-572
Timestamp: 2026-04-08T14:58:47.163Z
Learning: In frostney/GocciaScript, the `test262-conformance` CI job (`.github/workflows/ci.yml`) is intentionally non-blocking: it uses `|| true` on the runner step and `raise SystemExit(0)` when no report is produced, so it never fails the job. It is deliberately excluded from the `needs` list of both `artifacts` and `release` jobs. Do not flag the `|| true` or the `SystemExit(0)` as issues — the job is designed as a purely informational conformance-tracking signal.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 53
File: souffle/Souffle.Wasm.Emitter.pas:1467-1477
Timestamp: 2026-03-09T17:21:38.193Z
Learning: In this codebase (Frostney/GocciaScript), for Pascal/Delphi (FPC) files using AnsiString with {$mode delphi} {H+}, treat String as byte-indexed. Length(str) returns byte count and Byte(str[I]) is a no-op cast. Do not replace per-byte copy loops like Data[I-1] := Byte(AStr[I]) with TEncoding.GetBytes, since TEncoding is a Delphi RTL feature unavailable in FPC's SysUtils and would break the build. Apply this guidance to all .pas files under the project; ensure manual per-byte copying logic remains correct for UTF-8 content encoded in AnsiString, and avoid introducing RTL dependencies not supported by FPC.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 63
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-11T01:50:11.071Z
Learning: Replace hardcoded string literals 'now', 'toJSON', and 'timeOrigin' with the new constants from Goccia.Constants.PropertyNames (PROP_NOW, PROP_TO_JSON, PROP_TIME_ORIGIN). Apply this refactor across the repository wherever these literals are used, including but not limited to units/Goccia.Runtime.Operations.pas and units/Goccia.Builtins.Performance.pas. Use a project-wide search (e.g., regex or code navigation) to identify all occurrences, update to the respective constants, and adjust imports or uses accordingly. Ensure all references compile and behavior remains unchanged.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 63
File: units/Goccia.Runtime.Operations.pas:7514-7544
Timestamp: 2026-03-11T08:22:20.482Z
Learning: When bridging a native namespace object to a TSouffleRecord (e.g., performance) in units/Goccia.Runtime.Operations.pas, always set Result.Delegate := ARuntime.VM.RecordDelegate after TSouffleRecord.Create(...). This ensures Object.prototype methods (toString/hasOwnProperty/valueOf/toLocaleString) work in bytecode mode and NativeRecordToString honors Symbol.toStringTag. Apply this pattern to BuildPerformanceRecord and any future bridged records.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/HashMap.pas:266-309
Timestamp: 2026-03-17T22:17:37.410Z
Learning: Do not flag enumeration in THashMap<TKey, TValue> as concurrent-modification unsafe. The no-mutation contract is maintained during enumeration: GetEnumerator shares the internal FSlots array (no per-call allocation) and only mutation occurs via destructor or Clear, not during active iteration. This pattern should be recognized across similar Pascal units/pascal collection implementations; when reviewing code, ensure no concurrent-modification warnings are raised for shared internal structures during enumeration if the no-mutation contract is preserved.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/OrderedMap.pas:0-0
Timestamp: 2026-03-17T22:17:55.588Z
Learning: Enforce the constraint on TOrderedMap<TKey, TValue> in units/OrderedMap.pas: TKey must be an unmanaged, fixed-size value type (no strings, dynamic arrays, or records with managed/pointer fields). HashKey should use fast paths for SizeOf(TKey)=8 (QWord) and SizeOf(TKey)=4 (Cardinal); KeysEqual should compare via direct PQWord/PCardinal for those sizes. The raw-byte DJB2/CompareMem fallback is dead code for current uses. Document this constraint in a unit-header comment as the enforcement mechanism. Do NOT re-flag the byte-wise hash/equality in future reviews as long as all TOrderedMap specializations use pointer-sized unmanaged keys.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/ScopeMap.pas:0-0
Timestamp: 2026-03-17T22:18:00.984Z
Learning: Do not re-introduce TScopeMap. In frostney/GocciaScript, TScopeMap<TValue> was removed due to performance regressions from its linear-scan Add-as-upsert behavior. Reviews should flag any use of TScopeMap in Units/ScopeMap.pas or elsewhere and ensure bindings use TOrderedStringMap<TLexicalBinding> instead. If reviewing similar Pascal files, prefer avoiding upsert-like add semantics that overwrite existing entries, as they correlated with bytecode performance drops. Ensure changes adhere to the established replacement: TOrderedStringMap<TLexicalBinding>.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:24.165Z
Learning: Shared/utility units under the repository (e.g., units/BaseMap.pas, HashMap.pas, OrderedMap.pas, etc.) that are used across multiple parts of the project should not be required to use the Goccia.* prefix. A separate naming convention will be defined in a follow-up issue. Do not flag missing Goccia.* prefix for these shared infrastructure units in reviews.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:24.165Z
Learning: Do not flag missing Goccia.* prefixes for shared/utility units that are used across multiple parts of the project or can be extracted as standalone infrastructure (e.g., BaseMap.pas, HashMap.pas, OrderedMap.pas, OrderedStringMap.pas, GC, test assertion units). A separate naming convention (to be defined in issue `#104`) should apply to these targets. When reviewing, apply this guideline to all similar shared utility units under the units directory; for one-off implementations outside this scope, continue with the existing naming expectations.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 110
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-23T11:25:00.381Z
Learning: In this repo’s GocciaScript Pascal code, when implementing thenable detection/callability checks (e.g., around Await/unwrap logic), always read the thenable’s `then` property using `Goccia.Constants.PropertyNames.PROP_THEN` rather than a string literal. If the value being inspected is a `TSouffleRecord`, unwrap the retrieved `then` property via `UnwrapToGocciaValue` before testing whether it is callable (e.g., in `TGocciaRuntimeOperations.AwaitValue`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 107
File: units/Goccia.Runtime.Operations.pas:4349-4353
Timestamp: 2026-03-23T12:38:54.412Z
Learning: In Pascal source under units/, when decoding JSON string escape sequences that represent Unicode code points and then converting them to UTF-8, prefer using the existing helper (implemented via CodePointToUTF8) that correctly combines UTF-16 surrogate pairs (\uD800–\uDBFF + \uDC00–\uDFFF) into a single Unicode code point before UTF-8 encoding. Avoid ad-hoc conversions like Char(StrToInt(...)) for code-point→UTF-8 conversion logic; use the helper instead to ensure surrogate pairs are handled correctly.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 109
File: units/Goccia.Runtime.Operations.pas:6337-6344
Timestamp: 2026-03-23T15:57:55.802Z
Learning: In Pascal unit files under units/, any iterator result record objects produced by native iterators (e.g., in routines analogous to NativeMapIteratorNext / NativeSetIteratorNext) must: (1) set `Rec.Delegate := FVM.RecordDelegate`; and (2) set properties using `PROP_VALUE` / `PROP_DONE` constants from `Goccia.Constants.PropertyNames` rather than hard-coded string literals. Apply the same pattern to any future native iterator result objects so they interoperate correctly in bytecode mode (e.g., for `Object.prototype` method behavior) and keep property-name usage consistent via the shared constants.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 136
File: docs/adding-built-in-types.md:105-133
Timestamp: 2026-03-27T14:09:25.371Z
Learning: In frostney/GocciaScript Pascal code, do not flag or suggest refactoring the early-exit singleton initialization guard `if Assigned(FShared) then Exit` when it appears at the top of `InitializePrototype`. The project documentation treats this early-exit form as the canonical/shared-prototype singleton initialization template (i.e., avoid recommending an `if not Assigned(FShared) then ...` wrapping alternative).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 189
File: units/Goccia.JSON.pas:499-526
Timestamp: 2026-04-05T17:33:32.709Z
Learning: In frostney/GocciaScript, when using `TStringBuffer` from the `StringBuffer` unit, treat it as an advanced record (stack-allocated) with automatic cleanup by the compiler. For `TStringBuffer` locals, do not require or flag missing `SB.Free` calls in `try/finally` blocks—no destructor/free call is needed or appropriate. Only look for `Free` when the variable is actually a heap-allocated class instance.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 189
File: units/Goccia.JSON.pas:535-597
Timestamp: 2026-04-05T17:33:35.381Z
Learning: When reviewing Free Pascal code in this repo, treat `TStringBuffer` (from the `StringBuffer` unit) as an advanced record (value type), meaning instances are stack-allocated. Do not report or require `SB.Free` / `SB.FreeAndNil` (or any `Free` call) for variables typed as `TStringBuffer`, and do not treat an empty or no-op `finally` block after `TStringBuffer` usage as an issue. Only expect heap-management calls for actual `class`/reference types; calling `Free` on an advanced record is incorrect.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 201
File: units/Goccia.AST.Expressions.pas:1339-1346
Timestamp: 2026-04-07T21:00:57.707Z
Learning: When reviewing GocciaScript coverage/branch tracking, do not require call-sites to explicitly pre-register binary (two-arm) branches before recording hits. In units/Goccia.Coverage.pas, TGocciaFileCoverage.RecordBranchHit automatically ensures the opposite arm exists via EnsureBranchExists(ALine, AColumn, 1 - ABranchIndex), establishing a zero-hit baseline for the paired arm whenever either arm is first recorded. Therefore, call-sites such as TGocciaConditionalExpression.Evaluate (Goccia.AST.Expressions.pas), EvaluateIf/EvaluateBinary/EvaluateSwitch (Goccia.Evaluator.pas), and OP_JUMP_IF_* handlers (Goccia.VM.pas) should not be flagged for missing a separate RegisterBranch call for binary branches—pairing is handled inside RecordBranchHit.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 201
File: units/Goccia.VM.pas:3547-3561
Timestamp: 2026-04-07T21:01:16.129Z
Learning: When reviewing calls that record branch coverage via `TGocciaFileCoverage.RecordBranchHit(ALine, AColumn, ABranchIndex)` for binary branches (branch indices 0 and 1), do not require an explicit prior `RegisterBranch(...)` at the call site. `RecordBranchHit` internally calls `EnsureBranchExists(ALine, AColumn, 1 - ABranchIndex)` for binary branches, which guarantees both arms exist in the branch list (the untaken arm is pre-registered with zero hits). Therefore VM/evaluator/AST branch recording sites in `Goccia.VM.pas`, `Goccia.Evaluator.pas`, and `Goccia.AST.Expressions.pas` should not be flagged for missing explicit pre-registration before `RecordBranchHit(...)`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 209
File: units/Goccia.FFI.DynamicLibrary.pas:7-21
Timestamp: 2026-04-08T13:56:59.262Z
Learning: In Free Pascal (FPC), `TLibHandle` is defined in the `System` unit (`TLibHandle = PtrInt`), not in `Dynlibs`. Since `System` is implicitly available in all Pascal units, referencing `TLibHandle` in the interface section (e.g., as a private field type of a class declared in the interface/`interface` section) does not require `Dynlibs` to be listed in the interface `uses` clause. Only ensure `Dynlibs` is included in the implementation `uses` clause when you actually use `LoadLibrary`, `GetProcAddress`, or `FreeLibrary`. Therefore, do not flag a missing `Dynlibs` entry in the interface `uses` clause when the interface section references only `TLibHandle`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 242
File: AGENTS.md:396-400
Timestamp: 2026-04-09T11:50:01.452Z
Learning: In .pas code, avoid unsafe Int64→Double conversions/workarounds that trigger FPC 3.2.2 miscompilations. Specifically, do not rely on `Double(Int64Var)` when it performs bit reinterpretation (Delphi-mode front-end bug on affected versions/platforms). Also do not use arithmetic patterns like `Int64Var * 1.0` / `Int64Var * Double` that can produce incorrect results near ±2^31 on AArch64. Prefer value-safe conversions via implicit assignment (e.g., `var D: Double; D := SomeInt64`) or implicit promotion through function/constructor parameter passing (e.g., `TGocciaNumberLiteralValue.Create(SomeInt64)`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 253
File: units/Goccia.Values.Iterator.Concat.pas:84-88
Timestamp: 2026-04-10T12:44:59.067Z
Learning: In frostney/GocciaScript (GocciaScript), `TGocciaFunctionBase(callable).Call(args, thisValue)` is the project-standard way to invoke callable values. During code review, do not flag `TGocciaFunctionBase(...)` casts at call sites as unsafe and do not recommend replacing them with an alternative “generic callable path”—for this codebase, this cast-and-`Call` pattern is the intended generic callable invocation mechanism.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 273
File: units/Goccia.Values.URLValue.pas:465-470
Timestamp: 2026-04-11T17:42:27.330Z
Learning: In this codebase, when implementing or updating a `TGCManagedObject.MarkReferences` override for a held reference, treat `MarkReferences` as the act of marking *and* tracing children. The expected pattern is: after a nil check, call the held reference’s `MarkReferences` implementation directly (e.g., `TGocciaSomeValue(FRef).MarkReferences`) rather than invoking any non-existent separate `Mark(obj)`/`Mark()` helper. Do not suggest “call `Mark(obj)`”; this project’s marking flow starts by setting `FGCMark := FCurrentMark` and then tracing child references from within `MarkReferences` (consistent with `TGocciaMapValue`, `TGocciaSetValue`, `TGocciaArrayValue`, etc.).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 278
File: units/Goccia.Parser.pas:1071-1111
Timestamp: 2026-04-12T07:39:20.695Z
Learning: Do not free the result of `TGocciaLexer.ScanTokens` (e.g., in `ParseInterpolationExpression` and any other caller). `ScanTokens` returns a reference to the lexer's internal `FTokens: TObjectList<TGocciaToken>` created with `OwnsObjects=True` and freed by `TGocciaLexer`’s destructor. Adding `Tokens.Free` (or otherwise freeing the returned list/object) will cause double-free/access violations; token lifetime is managed entirely by the `TGocciaLexer` instance.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 285
File: units/Goccia.Lexer.pas:207-213
Timestamp: 2026-04-12T15:46:28.683Z
Learning: In this Frostney/GocciaScript project (Free Pascal / FPC, including files under units/), do not recommend adding explicit `{$B-}` directives (either inside units or in `Goccia.inc`) to “fix” short-circuit evaluation safety. `{$B-}` is already FPC’s default in all compiler modes (`fpc`, `delphi`, `objfpc`). Treat missing `{$B-}` as expected and do not flag it as a maintainability or safety issue. Only note `{$B-}` overrides if an explicit `{$B+}` is present in the file or via a highly visible project-level `fpc.cfg` setting.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 296
File: units/Goccia.REPL.Formatter.pas:15-29
Timestamp: 2026-04-13T16:53:02.191Z
Learning: For Pascal units in frostney/GocciaScript, enforce `uses`-clause grouping order as follows (with blank lines between groups): (1) System units (e.g., `SysUtils`), (2) Third-party units, then (3) Project units (e.g., `Goccia.*`). Specifically, `StringBuffer` (from `StringBuffer.pas`) must be treated as a Third-party unit and placed in the Third-party group. Do not report a `uses` ordering violation when `StringBuffer` appears between the System group and the `Goccia.*` group, since `./format.pas` and the Lefthook pre-commit hook enforce this grouping.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 302
File: docs/contributing/code-style.md:31-37
Timestamp: 2026-04-14T13:36:54.107Z
Learning: When reviewing Free Pascal (FPC) sources in this project, do not flag the compiler directive `{H+}` as a syntax error or as an inconsistency caused by a missing `$`. In FPC, `{H+}` (short-form) is valid and equivalent to `{$H+}` for enabling long/AnsiStrings. This project’s own `Goccia.inc` and documentation intentionally use `{H+}`, so the reviewer should only consider it an issue if it’s not valid FPC syntax for the surrounding context.
- Run scripts, tests, and benchmarks with configurable jobs - Update shared prototype setup for concurrent startup - Document new `--jobs` / `-j` options and progress output
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ScriptLoader.dpr (1)
715-730: Correct initialization and error aggregation pattern.The parallel implementation correctly:
- Initializes shared prototypes before spawning workers
- Aggregates errors on the main thread using
Pool.Results[I].ErrorMessage- Sets
ExitCode := 1when any script failsHowever, the worker's captured errors only contain
E.Message(line 703), losing the detailed formatting available forTGocciaErrorandTGocciaThrowValueexceptions.💡 Consider preserving detailed error messages
procedure TScriptLoaderApp.ScriptWorkerProc(const AFileName: string; const AIndex: Integer; out AConsoleOutput: string; out AErrorMessage: string; AData: Pointer); begin AConsoleOutput := ''; AErrorMessage := ''; try RunScriptFromFile(AFileName); except - on E: Exception do + on E: TGocciaError do + begin + AErrorMessage := E.GetDetailedMessage; + ExitCode := 1; + end; + on E: TGocciaThrowValue do begin AErrorMessage := E.Message; ExitCode := 1; + end; + on E: Exception do + begin + AErrorMessage := 'Fatal error: ' + E.Message; + ExitCode := 1; end; end; end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ScriptLoader.dpr` around lines 715 - 730, ScriptWorkerProc currently stores only E.Message into Pool.Results[].ErrorMessage, which discards the rich formatting available for TGocciaError and TGocciaThrowValue; update the exception handler inside ScriptWorkerProc to detect those types (TGocciaError, TGocciaThrowValue) and produce a detailed formatted string (using their available formatting/to-string helpers or by extracting fields) before assigning to Pool.Results[WorkerIndex].ErrorMessage so the main thread's error aggregation sees full details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@BenchmarkRunner.dpr`:
- Around line 604-650: BenchmarkWorkerProc spawns worker threads that call
CollectBenchmarkFile which currently writes directly to StdErr on exceptions,
causing interleaved stderr output; modify the worker to capture any stderr
output and forward it via AErrorMessage instead of letting background threads
write to StdErr: wrap the CollectBenchmarkFile call in a temporary stderr
capture (e.g., redirect StdErr to a memory stream or string buffer) inside
TBenchmarkRunnerApp.BenchmarkWorkerProc around the call to
CollectBenchmarkFile(AFileName, WorkerReporter, Mode, False), then on exit or
exception append the captured stderr contents to AErrorMessage (along with
E.Message if an exception occurs) so no thread writes to global StdErr, or
alternatively change the
CollectBenchmarkFileInterpreted/CollectBenchmarkFileBytecode implementations to
return error text instead of writing to StdErr and have BenchmarkWorkerProc
assign that text to AErrorMessage.
In `@TestRunner.dpr`:
- Around line 634-636: RunGocciaScript (and its helpers
RunGocciaScriptInterpreted and RunGocciaScriptBytecode) currently call WriteLn
on worker threads; change these functions to capture any error messages into the
worker's return structure (e.g. add/populate an ErrorText field on FileResult or
TestResult) instead of calling WriteLn at the error sites (the current WriteLn
calls around lines noted in RunGocciaScriptInterpreted and
RunGocciaScriptBytecode). Remove the WriteLn invocations from those functions,
ensure RunGocciaScript returns the error text alongside TestResult, and update
the main-thread aggregation code that handles FileResult/TestResult to print the
collected ErrorText to stdout so all console output happens on the main thread.
---
Nitpick comments:
In `@ScriptLoader.dpr`:
- Around line 715-730: ScriptWorkerProc currently stores only E.Message into
Pool.Results[].ErrorMessage, which discards the rich formatting available for
TGocciaError and TGocciaThrowValue; update the exception handler inside
ScriptWorkerProc to detect those types (TGocciaError, TGocciaThrowValue) and
produce a detailed formatted string (using their available formatting/to-string
helpers or by extracting fields) before assigning to
Pool.Results[WorkerIndex].ErrorMessage so the main thread's error aggregation
sees full details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61a5c9e5-4626-46cf-a01b-896475b0bca2
📒 Files selected for processing (9)
AGENTS.mdBenchmarkRunner.dprREADME.mdScriptLoader.dprTestRunner.dprdocs/benchmarks.mddocs/build-system.mddocs/testing.mdunits/Goccia.Values.ArrayValue.pas
✅ Files skipped from review due to trivial changes (5)
- docs/benchmarks.md
- README.md
- docs/build-system.md
- AGENTS.md
- docs/testing.md
🚧 Files skipped from review as they are similar to previous changes (1)
- units/Goccia.Values.ArrayValue.pas
Comment on lines
+634
to
+636
| try | ||
| FileResult := RunGocciaScript(AFileName); | ||
| TestResult := FileResult.TestResult; |
Contributor
There was a problem hiding this comment.
Worker still invokes methods that write to stdout from background threads.
RunGocciaScript (via RunGocciaScriptInterpreted/RunGocciaScriptBytecode) contains WriteLn calls for errors (lines 321, 326, 332 in interpreted; 465, 470, 476 in bytecode). These execute on worker threads, causing nondeterministic output ordering.
Consider capturing error output in the worker data record instead of writing directly, then printing on the main thread during result aggregation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TestRunner.dpr` around lines 634 - 636, RunGocciaScript (and its helpers
RunGocciaScriptInterpreted and RunGocciaScriptBytecode) currently call WriteLn
on worker threads; change these functions to capture any error messages into the
worker's return structure (e.g. add/populate an ErrorText field on FileResult or
TestResult) instead of calling WriteLn at the error sites (the current WriteLn
calls around lines noted in RunGocciaScriptInterpreted and
RunGocciaScriptBytecode). Remove the WriteLn invocations from those functions,
ensure RunGocciaScript returns the error text alongside TestResult, and update
the main-thread aggregation code that handles FileResult/TestResult to print the
collected ErrorText to stdout so all console output happens on the main thread.
Owner
Author
There was a problem hiding this comment.
Same situation as the ScriptLoader and BenchmarkRunner stdout comments — the WriteLn calls in RunGocciaScriptInterpreted/RunGocciaScriptBytecode are in error-handling paths that write error details. In practice, these are rare (only on script load failures or uncaught exceptions) and the messages include filenames so they're identifiable even if interleaved. A full output-capture refactor of the execution chain is deferred as a follow-up.
Contributor
There was a problem hiding this comment.
@frostney, understood — same accepted trade-off as in ScriptLoader and BenchmarkRunner. The WriteLn calls in the error-handling paths of RunGocciaScriptInterpreted / RunGocciaScriptBytecode are rare and filename-prefixed, so interleaving is tolerable for now. I'll note this to avoid re-flagging it until a full output-capture refactor lands.
✏️ Learnings added
Learnt from: frostney
Repo: frostney/GocciaScript PR: 306
File: TestRunner.dpr:634-636
Timestamp: 2026-04-15T06:27:53.198Z
Learning: In frostney/GocciaScript (PR `#306`, TestRunner.dpr), the `WriteLn` calls inside the error-handling paths of `RunGocciaScriptInterpreted` and `RunGocciaScriptBytecode` (e.g. on EStreamError, TGocciaError, TGocciaThrowValue, and generic Exception) execute on worker threads during parallel directory runs, causing potentially interleaved stdout. This is an accepted trade-off: these paths are rare (script-load failures / uncaught exceptions only), messages include filenames for identification, and a proper output-capture refactor (routing error text through AConsoleOutput/AErrorMessage and printing on the main thread) is deferred to a follow-up. Do NOT re-flag these WriteLn calls in TestRunner.dpr parallel worker context until the output-capture refactor lands.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: frostney
Repo: frostney/GocciaScript PR: 306
File: ScriptLoader.dpr:692-707
Timestamp: 2026-04-15T06:26:10.118Z
Learning: In frostney/GocciaScript (ScriptLoader.dpr), `TScriptLoaderApp.ScriptWorkerProc` intentionally calls `RunScriptFromFile` directly in worker threads, which causes interleaved/nondeterministic stdout in parallel directory runs. This is an accepted trade-off for the initial parallel implementation — ScriptLoader parallel mode is only used for directory inputs where output ordering is less critical. A proper fix (refactoring `RunSource`/`ExecuteInterpreted` to support output capture and returning text via `AConsoleOutput`/`AErrorMessage`) has been deferred to a follow-up. Do not re-flag the direct `RunScriptFromFile` call in `ScriptWorkerProc` as a regression or issue until the output-capture refactor lands.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 306
File: units/Goccia.Coverage.pas:406-422
Timestamp: 2026-04-15T06:26:41.145Z
Learning: In frostney/GocciaScript, `GetJobCount` (in `units/Goccia.CLI.Application.pas`) intentionally returns 1 when `--coverage` is enabled, forcing sequential execution for coverage runs. Per-thread `TGocciaCoverageTracker` initialization and cross-thread coverage merging are deferred to a follow-up. Do not re-flag missing worker-thread coverage initialization in `TGocciaFileWorker.Execute` (`units/Goccia.Threading.pas`) — the fix is that parallel mode is disabled for coverage, not that workers initialize a tracker.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 201
File: TestRunner.dpr:237-240
Timestamp: 2026-04-07T20:24:01.235Z
Learning: Repo: frostney/GocciaScript — TestRunner.dpr / Goccia.Coverage
The synthetic `runTests({...})` call is appended to `Source` before lexing in both `RunGocciaScriptBytecode` and `RunGocciaScriptInterpreted`. `Lexer.SourceLines` therefore includes this line, and `CountExecutableLines` counts it as +1 executable. Because the engine always executes it, coverage also records +1 hit. The net effect on the percentage is self-consistent (+1/+1) and does NOT skew coverage ratios. `PrintCoverageDetail` reads the original file from disk, so the synthetic line never appears in the detail output. Do not flag this as a coverage accuracy bug.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 200
File: REPL.dpr:0-0
Timestamp: 2026-04-07T17:12:07.324Z
Learning: When reviewing timing/metrics code in the GocciaScript repo (e.g., REPL.dpr, ScriptLoader, TestRunner, BenchmarkRunner, and TGocciaEngine.Execute), follow the established timing convention: capture `StartTime` before the JSX pre-pass (`TGocciaJSXTransformer.Transform`) and `StringListToLFText`, then capture `LexEnd` after `ScanTokens`. The `Lex` duration (`LexEnd - StartTime`) is intended to include the JSX pre-pass because it is treated as part of the source→tokens pipeline. Do not flag this ordering as a bug and do not recommend moving `StartTime` to after the JSX transform, as it would break the bucket semantics.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 289
File: ScriptLoader.dpr:140-156
Timestamp: 2026-04-12T21:19:18.811Z
Learning: For GocciaScript CLI tools, suppress human-readable status/progress WriteLn output when running in JSON output mode. In ScriptLoader (ScriptLoader.dpr), gate any status/progress WriteLn lines on `not GJsonOutput`, because `--output=json` streams the result document directly to stdout (mixing status text would corrupt JSON). In TestRunner and BenchmarkRunner (TestRunner.dpr, BenchmarkRunner.dpr), JSON is written to a file, so current unconditional stdout WriteLn calls are safe, but these tools should be refactored later to follow the same stdout/JSON suppression pattern used by ScriptLoader.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 306
File: units/Goccia.Timeout.pas:26-29
Timestamp: 2026-04-15T06:27:18.195Z
Learning: In frostney/GocciaScript (PR `#306`, units/Goccia.Timeout.pas), `StartExecutionTimeout` / `ClearExecutionTimeout` / `CheckExecutionTimeout` use `threadvar` storage. `StartExecutionTimeout` is called inside execution functions (`RunGocciaScriptInterpreted`, `ExecuteInterpreted`, `CollectBenchmarkFileInterpreted`, etc.) that run directly on worker threads — NOT on the main thread. Each worker thread correctly initializes its own timeout state before the engine loop calls `CheckExecutionTimeout`. The CLI timeout option value is a read-only/immutable CLI option object shared safely across threads. Do NOT re-flag the `threadvar` timeout design as leaving worker threads un-initialized.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: ScriptLoader.dpr:724-737
Timestamp: 2026-04-14T06:46:14.795Z
Learning: In frostney/GocciaScript (ScriptLoader.dpr), `--emit` with `--output=<file>` and multiple input paths or a directory input silently overwrites the same output file for each compiled module. This is pre-existing behavior preserved across the CLI refactor in PR `#299` and should NOT be re-flagged as a regression. A follow-up guard (raising TGocciaParseError when FEmit.Present and FOutputPath.Present and APaths.Count > 1 or the single path is a directory) was proposed but deferred.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: TestRunner.dpr:132-139
Timestamp: 2026-04-14T06:46:10.919Z
Learning: In frostney/GocciaScript CLI tool sources, the duplicated `EscapeJSONString` helper intentionally escapes only `\`, `"`, `\n`, `\r`, and `\t` (leaving other C0 control characters U+0000–U+001F unescaped). During code review, do not flag missing additional control-character escaping in these per-tool `EscapeJSONString` copies while the shared-helper consolidation/fix PR is not yet landed; only re-check once the implementation has been consolidated into the shared helper and the tracking fix is in place.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 306
File: units/Goccia.Builtins.GlobalPromise.pas:56-57
Timestamp: 2026-04-15T06:26:30.835Z
Learning: In frostney/GocciaScript (PR `#306`), ~30 builtin units (e.g., Goccia.Builtins.GlobalPromise.pas, Goccia.Builtins.GlobalArray.pas, Goccia.Builtins.Math.pas, and ~27 others) use `threadvar FStaticMembers: TArray<TGocciaMemberDefinition>` in their `implementation` sections. FPC does NOT finalize managed threadvar contents on thread exit, so the TArray shell (~few hundred bytes per unit) leaks per worker thread. This is an accepted/deferred trade-off: (1) threads are short-lived spawn-and-join workers, (2) the GC-managed TGocciaMemberDefinition objects are cleaned up by ShutdownThreadRuntime, (3) OS reclaims all memory on process exit, (4) a proper fix requires a registration-based cleanup system or per-unit finalization hooks since the threadvars are in implementation sections. Do NOT re-flag this managed-threadvar leak until long-lived thread pools are introduced and a cleanup registry is in place.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: TestRunner.dpr:132-139
Timestamp: 2026-04-14T13:53:18.321Z
Learning: For GocciaScript CLI tool sources in this repo, when you see duplicated `EscapeJSONString` helpers that only escape `\`, `"`, `\n`, `\r`, and `\t` (leaving other C0 control characters U+0000–U+001F unescaped) in individual per-tool `.dpr` files, do not re-flag this as a review issue while the consolidation/fix tracked in issue `#303` has not landed. After the PR resolving `#303` is merged, start reviewing these helpers for full control-character JSON escaping again.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 286
File: docs/benchmarks.md:121-123
Timestamp: 2026-04-12T15:44:31.303Z
Learning: In frostney/GocciaScript BenchmarkRunner.dpr, both interpreted and bytecode execution paths use the same `[ggBenchmark]` globals set. `TGocciaEngine` is constructed with `[ggBenchmark]` for interpreted mode, and `TGocciaBytecodeBackend.RegisterBuiltIns([ggBenchmark])` is called for bytecode mode. Documentation referring to `[ggBenchmark]` as the globals for the benchmark runtime is accurate for both modes. Do not flag the doc step as mode-inaccurate on the basis of different backend types.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 286
File: docs/testing.md:774-776
Timestamp: 2026-04-12T15:44:39.321Z
Learning: In frostney/GocciaScript TestRunner.dpr, both interpreted (`RunGocciaScriptInterpreted` via `TGocciaEngine`) and bytecode (`RunGocciaScriptBytecode` via `TGocciaBytecodeBackend`) paths use the same globals set `[ggTestAssertions, ggFFI]`. `TGocciaBytecodeBackend.RegisterBuiltIns` accepts the same `TGocciaGlobalBuiltins` type. Documentation stating "creates a fresh `TGocciaEngine` with `[ggTestAssertions, ggFFI]`" is intentionally concise and accurate for both modes — do not flag it as incomplete for omitting the bytecode backend distinction.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 299
File: ScriptLoader.dpr:727-740
Timestamp: 2026-04-14T13:51:07.273Z
Learning: In frostney/GocciaScript (ScriptLoader.dpr), the `--emit` flag (bytecode compilation and output to a `.gbc` file) is an exploratory/temporary feature added to validate the bytecode pipeline. It is NOT the long-term solution for bytecode output. A proper bundling mechanism is being designed separately and tracked in issue `#156`. Do not flag `--emit` multi-file overwrite behavior or the presence of bytecode emission logic in ScriptLoader as architectural problems — the entire emit path is expected to be replaced by the bundler. ScriptLoader doing bytecode output is acknowledged as "too much responsibility" by the maintainer.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 107
File: units/Goccia.Runtime.Operations.pas:211-212
Timestamp: 2026-03-23T12:37:30.936Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas (PR `#107`)
Learning: In TGocciaRuntimeOperations.MarkExternalRoots, entries in FModuleCache (cached module namespace/JSON records) must be traversed and their TSouffleValue references marked as external roots to survive GC. This behavior was added in commit 36f69cc; do not re‑flag missing FModuleCache rooting unless the logic regresses.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : Built-ins are registered by the engine via `TGocciaGlobalBuiltins` flags. The TestRunner adds `ggTestAssertions` for the test framework. The BenchmarkRunner adds `ggBenchmark` for the benchmark framework.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 139
File: units/Goccia.ScriptLoader.JSON.Test.pas:1-113
Timestamp: 2026-03-30T13:02:33.857Z
Learning: In frostney/GocciaScript, CI workflows (`.github/workflows/ci.yml` and `.github/workflows/pr.yml`) use glob patterns (`build/Goccia.*.Test*` for artifact upload, `Goccia.*.Test` / `Goccia.*.Test.exe` for execution) to discover and run native Pascal test binaries automatically. Do not flag missing entries in a hard-coded test list — new `units/*.Test.pas` test programs are picked up automatically via these globs as of commit b63911d (PR `#139`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 60
File: units/Goccia.Runtime.Operations.pas:8694-8696
Timestamp: 2026-03-10T18:05:46.926Z
Learning: Repo: frostney/GocciaScript PR: 60 — units/Goccia.Runtime.Operations.pas: The GC rooting of FArrayBridgeReverse (marking values in MarkExternalRoots and keys in MarkWrappedGocciaValues) is intentional to preserve array identity across async/microtask boundaries (e.g., Promise.resolve(arr).then(v => v === arr)). Do not prune this map during normal clearing; per-file GC.Collect in runners bounds cross-file memory growth.
- Force sequential execution when --exit-on-first-failure is set - Force sequential when --coverage is enabled (not yet thread-safe) - Convert GRegExpPrototype to threadvar to prevent cross-thread races - Convert GNextSymbolId/GSymbolRegistry to threadvar for thread isolation - Guard FSharedPrototype assignment in FunctionBase with try/except - Handle uninitialized result slots on fatal worker thread exceptions - Fix CPU detection: use sysconf directly (FPC 3.2.2 bug workaround) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.CLI.Application.pas`:
- Around line 258-260: GetJobCount currently only checks
FCoverageOptions.Enabled.Present before forcing sequential execution which
misses cases where InitializeCoverageIfEnabled turns on coverage via
Format.Present or OutputPath.Present; update the check in GetJobCount to also
consider FCoverageOptions.Format.Present and FCoverageOptions.OutputPath.Present
(or whatever properties InitializeCoverageIfEnabled reads) so that if any of
Enabled.Present, Format.Present, or OutputPath.Present is true you call Exit(1)
to force sequential mode; reference the GetJobCount routine and
FCoverageOptions.Enabled/Format/OutputPath fields to locate and modify the logic
to mirror InitializeCoverageIfEnabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 466bd623-3fe7-474d-a1dc-906af8e72f56
📒 Files selected for processing (6)
TestRunner.dprunits/Goccia.CLI.Application.pasunits/Goccia.RegExp.Runtime.pasunits/Goccia.Threading.pasunits/Goccia.Values.FunctionBase.pasunits/Goccia.Values.SymbolValue.pas
🚧 Files skipped from review as they are similar to previous changes (2)
- units/Goccia.Values.FunctionBase.pas
- units/Goccia.Values.SymbolValue.pas
Instead of forcing sequential execution when these flags are set, implement proper parallel support: - Add CancelOnError to TGocciaThreadPool: when a worker detects a failure, it sets the shared cancelled flag so all workers skip remaining files. Up to N in-flight files may complete (expected). - Add EnableCoverage to TGocciaThreadPool: each worker initialises its own thread-local coverage tracker. After all workers complete, MergeCoverageInto collects per-thread coverage data into the main tracker via the new TGocciaCoverageTracker.MergeFrom method. - Remove the GetJobCount hack that forced --jobs=1 for coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
units/Goccia.Coverage.pas (1)
515-518: Line hit merge loses actual hit counts — only records presence.The comment (lines 81-83) says "Line hits and branch hits are summed," but
RecordLineHit(I)increments by 1, not bySrcFile.GetLineHitCount(I). If a line was hit 5 times in a worker, the merged result shows 1 hit.This is fine for coverage percentage calculations (
LinesHitchecks> 0), but if lcov/JSON export requires accurate hit counts, they'll be underreported in parallel mode.♻️ Optional fix to preserve actual hit counts
{ Merge line hits — sum counts for each line. } for I := 1 to SrcFile.LineHitCount - 1 do - if SrcFile.GetLineHitCount(I) > 0 then - DstFile.RecordLineHit(I); + begin + HitCount := SrcFile.GetLineHitCount(I); + while HitCount > 0 do + begin + DstFile.RecordLineHit(I); + Dec(HitCount); + end; + end;Or add a
RecordLineHits(ALine, ACount: Integer)method for efficiency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Coverage.pas` around lines 515 - 518, The merge loop currently discards actual hit counts by calling DstFile.RecordLineHit(I) (which increments by 1) inside the for I := 1 to SrcFile.LineHitCount - 1 loop; change it to add the exact count returned by SrcFile.GetLineHitCount(I) to the destination. Implement or use a method like DstFile.RecordLineHits(ALine: Integer; ACount: Integer) and call DstFile.RecordLineHits(I, SrcFile.GetLineHitCount(I)) (or, if you prefer not to add a new API, add the returned count in a small loop or an existing accumulator) so merged results preserve the true hit counts rather than only presence.units/Goccia.CLI.Application.pas (1)
72-104: Document or prepare for non-glibc Linux and BSD support.The hardcoded value
SC_NPROCESSORS_ONLN = 84is correct for glibc-based Linux, which covers all current CI targets (x86_64-linux, aarch64-linux). However, musl libc (Alpine Linux), FreeBSD, NetBSD, and OpenBSD define different numeric values for this constant. While not an issue today, document the glibc assumption in a comment or consider adding a FreeBSD case if future platforms are targeted, since the value 84 will not work for those systems.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.CLI.Application.pas` around lines 72 - 104, GetProcessorCount currently hardcodes SC_NPROCESSORS_ONLN = 84 for non-DARWIN UNIX which assumes glibc; document this assumption and/or add platform-specific branches for musl and BSDs by extending the conditional around SC_NPROCESSORS_ONLN (or adding new constants) so libc_sysconf calls use the correct numeric constant on FreeBSD/NetBSD/OpenBSD and musl-based Linux; update the comment above libc_sysconf/GetProcessorCount to state the glibc assumption and, if you want runtime support, add {$IFDEF FREEBSD}/{$IFDEF NETBSD}/{$IFDEF OPENBSD} cases (or detect libc/machine at compile time) and set SC_NPROCESSORS_ONLN appropriately so GetProcessorCount returns correct values across those platforms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Threading.pas`:
- Around line 98-100: Remove the duplicated comment above the CancelOnError
property: keep a single descriptive comment for the property that references
FCancelOnError and CancelOnError, and delete the redundant duplicate line so
only one "{ When True, the first worker error automatically cancels remaining
files. }" remains immediately above "property CancelOnError: Boolean read
FCancelOnError write FCancelOnError;".
---
Nitpick comments:
In `@units/Goccia.CLI.Application.pas`:
- Around line 72-104: GetProcessorCount currently hardcodes SC_NPROCESSORS_ONLN
= 84 for non-DARWIN UNIX which assumes glibc; document this assumption and/or
add platform-specific branches for musl and BSDs by extending the conditional
around SC_NPROCESSORS_ONLN (or adding new constants) so libc_sysconf calls use
the correct numeric constant on FreeBSD/NetBSD/OpenBSD and musl-based Linux;
update the comment above libc_sysconf/GetProcessorCount to state the glibc
assumption and, if you want runtime support, add {$IFDEF FREEBSD}/{$IFDEF
NETBSD}/{$IFDEF OPENBSD} cases (or detect libc/machine at compile time) and set
SC_NPROCESSORS_ONLN appropriately so GetProcessorCount returns correct values
across those platforms.
In `@units/Goccia.Coverage.pas`:
- Around line 515-518: The merge loop currently discards actual hit counts by
calling DstFile.RecordLineHit(I) (which increments by 1) inside the for I := 1
to SrcFile.LineHitCount - 1 loop; change it to add the exact count returned by
SrcFile.GetLineHitCount(I) to the destination. Implement or use a method like
DstFile.RecordLineHits(ALine: Integer; ACount: Integer) and call
DstFile.RecordLineHits(I, SrcFile.GetLineHitCount(I)) (or, if you prefer not to
add a new API, add the returned count in a small loop or an existing
accumulator) so merged results preserve the true hit counts rather than only
presence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea2dc9f3-c80f-4c4d-bbae-5ea846d901ef
📒 Files selected for processing (4)
TestRunner.dprunits/Goccia.CLI.Application.pasunits/Goccia.Coverage.pasunits/Goccia.Threading.pas
✅ Files skipped from review due to trivial changes (1)
- TestRunner.dpr
Round-robin pre-partitioned files across workers at startup, causing heavy imbalance when file execution times vary (e.g. 183s vs 19s in benchmarks). Workers with short files finished early while one worker was stuck with all the heavy files. Replace with a shared TGocciaWorkQueue protected by a critical section. Each worker pulls the next file when ready, so fast workers naturally pick up more files and all cores stay busy. Results on full benchmark suite (21 files, 10 workers): Round-robin: 8m 03s → Work-stealing: 5m 01s (4.0× vs sequential) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Threading.pas`:
- Around line 327-343: The current startup loop can leak a freed Queue if a
TGocciaFileWorker.Create/Start raises; fix by tracking how many workers
successfully started (use FWorkers and a StartedCount), and on any exception set
FCancelled := True to signal workers to stop, then iterate the started workers
and call WaitFor (and Free) on each to ensure they exit before allowing
Queue.Free to run; wrap the worker-creation loop in try/except to perform this
cleanup and re-raise the exception, so Queue (TGocciaWorkQueue) is only freed
after all started TGocciaFileWorker instances have been waited on and freed.
- Around line 305-317: Reset the pool state at the top of
TGocciaThreadPool.RunAll by clearing the cancelled flag so previous
Cancel()/CancelOnError runs don't taint subsequent calls: inside RunAll (e.g.,
immediately after begin) set FCancelled := False (and any other per-run
flags/results if present) before proceeding with FileCount handling so the pool
will execute the new batch normally.
- Around line 72-73: FCancelled is accessed concurrently causing a data race;
synchronize reads/writes by guarding all accesses to FCancelled with the
existing queue critical section (the same
EnterCriticalSection/LeaveCriticalSection pattern used in TGocciaWorkQueue) or
replace FCancelled with an atomic/interlocked flag and use
InterlockedExchange/CompareExchange for writes and a single atomic read for
checks. Update the worker loop that reads FCancelled, the error-path that sets
FCancelled, and the Cancel() method to use the chosen synchronization (reference
symbols: FCancelled, Cancel, the queue lock methods/critical section used by
TGocciaWorkQueue) so no unsynchronized reads/writes remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eed70509-1fff-44c8-851c-cabfd53f678b
📒 Files selected for processing (1)
units/Goccia.Threading.pas
- Add Goccia.Threading.Test.pas with 10 unit tests covering: work queue drain/order/empty, pool file dispatch, result ordering, cancel, cancel-on-error, state reset between runs, empty list, and single-worker mode. - Reset FCancelled at start of RunAll so a pool can be reused after a previous Cancel or CancelOnError run. - Pre-nil FWorkers array and move WaitFor into the finally block so a Create/Start failure doesn't free the queue while workers are still running. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three module-level variables in the bytecode compiler were shared across all worker threads, causing intermittent crashes (Bus error, Access violation, Invalid pointer operation) and corrupted bytecode output (ReferenceError: true/false is not defined) during parallel execution: - GBreakJumps, GPendingFinally, GBreakFinallyBase in Goccia.Compiler.Statements.pas — break/finally jump tracking corrupted when two workers compiled simultaneously - GUsingResources in Goccia.Compiler.Statements.pas — using/dispose resource tracking shared across threads - GClassPrivateCounter in Goccia.Compiler.Scope.pas — private field prefix counter incremented without synchronisation All converted to threadvar. Verified with 10 consecutive parallel bytecode runs: 0 crashes, 0 intermittent failures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ScriptLoader,TestRunner, andBenchmarkRunnerwith a new--jobsoption and automatic worker-count selection.Testing
./build.pas testrunner && ./build/TestRunner tests./build.pas loader && ./build/ScriptLoader path/to/multi-file/scripts --jobs=4./build.pas benchmarkrunner && ./build/BenchmarkRunner benchmarks --jobs=4./format.pas --check