Make generated Jass comparable across runs, and cut the suite from 13 to 7 minutes by Frotty · Pull Request #1235 · wurstscript/WurstScript

Frotty · 2026-08-15T16:58:55Z

Stacked on #1234.

Determinism

The counters naming generated temporaries were per thread and never reset, so a name depended on how much had been compiled before it in the process: the same source emitted temp0 on its own and temp70 after other tests had run — measured, not assumed. That is what made generated Jass impossible to compare across runs, which is the cheapest way there is to check that a change did not alter the output.

They restart at the top of each compilation rather than at each flattening, because flattening happens again after every optimisation and restarting there would give one function two locals of the same name.

Suite time

The suite ran one test at a time on an eight core machine: 786s of test time, 13m11s of wall clock. Measured from the result XML, the top ten classes are 61% of it.

Two things were needed for forking to actually help:

Forks rather than threads, because the harness keeps state in statics (current test environment, global caches, extracted lua binaries) and a fork gets its own copy. Gradle hands out whole classes, so DeterministicChecks, which writes fixed file names, stays inside one fork.
A processor count per fork. Each one otherwise sizes its GC and compiler threads for the whole machine. Eight forks doing that made every test ~3× slower and gave back only a third of the parallelism (8m34s). Capping them takes it to 7m03s. Four forks was measured too and is worse (8m39s) — less contention, but not enough parallelism to pay for it.

ExportToWurstTest was the slowest class at 108s, and none of it was the object data work: 30 of its 32 tests take no measurable time. Two tests looped over five cases and compiled each separately against the whole standard library. The cases declare different objects and only the compile needs the library, so they are compiled together: 108s → 67s. That class is worth the attention because wall clock cannot fall below the slowest single class.

Also included

Two pinned diagnostics with their diagnoses, no behaviour change: the constructor case for Lua, and where subclassing a bounded generic stops. Both are recorded in BACKLOG.md with what a fix would have to do — including one approach I tried and reverted, because the callee has no type variables of its own so annotating the call does nothing; the call must be redirected to the specialised copy.

Full test suite green.

The counters naming generated temporaries were per thread and never reset, so the name a temporary got depended on how much had been compiled before it in the process: the same source emitted temp0 on its own and temp70 after other tests had run. That is what made generated Jass impossible to compare across runs, which is the cheapest way there is to check that a change did not alter the output. They restart at the top of each compilation rather than at each flattening, because flattening happens again after every optimisation and restarting there would give one function two locals of the same name.

The suite ran one test at a time on an eight core machine: 786s of test time, 13m11s of wall clock. It now forks, which needs two things to actually help. Forks rather than threads, because the harness keeps state in statics - the current test environment, the global caches, the extracted lua binaries - and a fork gets its own copy. Gradle hands out whole classes, so DeterministicChecks, which writes fixed file names, stays inside one fork. And a processor count per fork, because each one otherwise sizes its garbage collector and compiler threads for the whole machine. Eight forks doing that made every test about three times slower and gave back only a third of the parallelism: 8m34s. Capping them takes it to 7m03s. ExportToWurstTest was the slowest class at 108s, and none of it was the object data work - thirty of its thirty-two tests take no measurable time. Two tests looped over five cases and compiled each one separately against the whole standard library. The cases declare different objects and only the compile needs the library, so they are compiled together now: 108s to 67s. That class is worth the attention because wall clock cannot fall below the slowest single class.

The override's super.size(extra) becomes a direct call to the superclass implementation, and that call carries no type arguments, because the type variables belong to the class rather than to the method. Nothing specialises it, so once the class is specialised and Box_size is replaced by Box_size⟪integer⟫, the super call points at what was removed. That is the same gap as the constructor case: addMemberTypeArguments visits method calls and member accesses, and nothing else. Constructor calls and super calls both go without. Whoever closes one should close the other. Also tried and reverted giving Jass the dangling-reference check the Lua backend has, which would have named the removed function instead of what happened to be inside it. The backends disagree on which functions exist: Jass is handed getCalledFunctions() and emits whatever is called, and three passing tests rely on a closure's construct_Lazy being detached and still called. The invariant is Lua's, not a shared one, so it stays where it was.

Tried the obvious reading of the previous commit's diagnosis: extend addMemberTypeArguments so a super call gets the receiver's type arguments the way a method call does. It changes nothing, and the reason is worth writing down. The callee has no type variables of its own - they belong to the class - so type arguments on the call have nothing to select. A class function is specialised by copying the whole class, so the call has to be redirected to that copy rather than annotated. Which also separates this from the constructor case it was grouped with: there the instantiation is not on any argument either, only on the type of what the call is assigned to. Same symptom, two different fixes.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bd64d08423

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-15T17:02:10Z

+    // fixed file names stays inside one fork.
+    // Wall time cannot fall below the slowest single class, which is why this does not need to be
+    // every core to get most of the win.
+    maxParallelForks = Math.max(1, (int) (Runtime.runtime.availableProcessors() / 2))


Bound test forks by available memory

On machines where visible CPU count is not proportional to RAM—such as a high-core CI runner or container—this creates processors / 2 workers with a 2 GiB heap each, in addition to the Gradle JVM's 3 GiB heap. The checked .github/workflows/build.yml invokes plain ./gradlew test without a worker cap, so a 16-core, 8-GiB environment can attempt eight memory-heavy compiler workers and thrash or be OOM-killed. Cap the default fork count or make it an overridable, memory-aware property.

AGENTS.md reference: AGENTS.md:L88-L91

Useful? React with 👍 / 👎.

Cores decide how much parallelism is useful; memory decides how much is survivable, and the two are not proportional everywhere. Sixteen cores and eight gigabytes - an ordinary CI runner - would have started eight two-gigabyte workers next to the daemon's own three and been killed for it, and the workflow runs plain gradlew test with no cap of its own. The count is now the smaller of half the cores and what memory allows, after reserving the daemon's heap and room for the lua and pjass processes the tests spawn. -PtestForks=N overrides it, and a machine that will not report its physical memory falls back to cores capped at four.

Frotty · 2026-08-16T07:03:24Z

Fixed. The concern is real and the CI path is exactly as you describe — .github/workflows/build.yml runs plain ./gradlew test, and gradle.properties gives the daemon -Xmx3g on top of the workers.

The count is now the smaller of half the cores and what memory allows, reserving the daemon's heap plus room for the lua and pjass processes the tests spawn. -PtestForks=N overrides it, and a machine that will not report physical memory falls back to cores capped at four rather than guessing high.

Kept the cores-based side as the upper bound rather than lowering it, because the measurements went the other way on this eight-core machine: serial 13m11s, four forks 8m39s, eight forks 7m03s. More forks kept winning even though each test runs about 2.6x slower under the contention — so the fix is to stop memory-poor machines over-committing, not to reduce parallelism everywhere.

Verified both paths: default resolves and runs, and -PtestForks=2 takes effect.

Also merged #1234 through, keeping both tests that landed at the same place in TypeClassTests.

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 83d0e08068

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-16T07:09:20Z

+    // Bounded by memory as well as cores, because the two are not proportional everywhere: a CI
+    // runner with sixteen cores and eight gigabytes would otherwise start eight two-gigabyte
+    // workers next to the daemon's own three and be killed for it. Override with -PtestForks=N.
+    maxParallelForks = testForkCount()


Initialize the shared stdlib before starting test forks

On a clean checkout whenever this resolves above one, independent worker JVMs can concurrently enter StdLib.downloadStandardlib(): its synchronized lock and isInitialized flag are process-local, while every worker clones/checks out the same ./temp/WurstStdlib2 directory (StdLib.java:41-71). One worker can therefore observe or mutate another worker's partially cloned repository, causing fresh CI runs to fail or leave a corrupt checkout; download the pinned stdlib once before the forked test task or give each worker an isolated directory.

AGENTS.md reference: AGENTS.md:L60-L63

Useful? React with 👍 / 👎.

…nism-and-parallel-tests

…proof (#1232)

…nd-parallel-tests # Conflicts: # BACKLOG.md # de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/EliminateGenerics.java # de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaRunnerTests.java # de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/TypeClassTests.java # de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java

StdLib guards its fetch with a lock and a flag held in one process, which stops guarding anything once the tests run in several. On a clean checkout each worker would clone into the same directory at the same time, so a fresh CI run could fail or leave a half-written checkout behind - a hazard this branch introduced by forking at all. The build now fetches it once, from its own JVM, before the workers start. The repository and pinned commit stay defined in StdLib rather than being repeated in the build file. The task deliberately declares no output: treating the directory as one would skip the fetch whenever it merely exists, which is exactly when it may be at the wrong commit or half cloned - the fetch checks and repairs that, and costs nothing when there is nothing to do. Verified from no checkout at all: clones, notices the wrong commit, and lands on the pinned one. Also logs the exception rather than the identity of its stack trace array, which is what that path printed while this was being diagnosed.

Frotty · 2026-08-16T08:29:47Z

Updated from master and the P1 is fixed — a real hazard, and one this branch introduced by forking at all.

StdLib guards the fetch with a synchronized method and an isInitialized flag, both of which stop guarding anything once the workers are separate JVMs pointed at the same ./temp/WurstStdlib2. It did not bite here only because that directory already existed.

The build now fetches it once from its own JVM, before the workers start, via a task the test task depends on. The repository URL and pinned commit stay defined in StdLib rather than being repeated in the build file.

One detail worth flagging, because I got it wrong first: the task deliberately declares no output. Declaring the checkout as one lets Gradle skip the fetch whenever the directory merely exists — which is precisely when it may be at the wrong commit or half cloned. My first attempt did that and left the checkout sitting at the wrong revision. The fetch verifies and repairs the pinned commit itself, and costs nothing when there is nothing to do.

Verified from no checkout at all: clones, reports Wrong version ... executing git pull, and lands on 16729fa.

Also fixed the logging in that path, which printed e.getStackTrace().toString() — the identity of an array — and told me nothing while I was diagnosing this.

DeterministicChecks, TypeClassTests and ExportToWurstTest pass.

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 96f8e2ac68

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-16T08:34:55Z

+}
+
 test {
+    dependsOn 'ensureStdLib'


Repair detached stdlib checkouts before gating tests

When temp/WurstStdlib2 is an existing detached or shallow checkout without a local master ref, this dependency makes every test enter the repair path, but StdLib.downloadStandardlib() checks out master before fetching it (StdLib.java:76). I reproduced this with the repository's prepopulated detached checkout: even an unrelated focused test failed in ensureStdLib with Ref master cannot be resolved. Fetch/create the remote tracking ref or reclone an invalid checkout before attempting the checkout so a stale cache does not block the entire suite.

Useful? React with 👍 / 👎.

The fetch went by way of master, and the checkout it leaves behind is detached with no local master to go back to - so on any machine that already had one, the repair path asked for a ref that was not there and failed with "Ref master cannot be resolved". That mattered little while only the tests using the library were affected. It matters now that every test waits on this, which is a hazard this branch introduced: a stale cache would stop the whole suite. It fetches from the remote with an explicit refspec and checks out the pinned commit directly, with no branch in between. A directory that cannot be opened as a repository, or has no HEAD - what an interrupted clone leaves - is discarded and cloned again rather than treated as a cache. Failures print to the console as well. The build waits on this, so whoever reads a failed run needs to see the reason there rather than in a log file; diagnosing this one started with an exception that reported only that something went wrong. Checked against all four states: no checkout, a good one, one detached with no local master, and one with its .git removed.

Frotty · 2026-08-16T08:40:44Z

Fixed, and reproduced exactly as you described.

The repair path went by way of master, and the checkout this leaves behind is detached with no local master — so on any machine that already had a checkout, it asked for a ref that was not there. Nothing to fetch and Ref master cannot be resolved were both on that path. It now fetches from the remote with an explicit refspec and checks out the pinned commit directly, with no branch in between.

Your framing of the severity is the part I had missed: this was survivable while only the tests using the library were affected, and it is not now that every test waits on ensureStdLib. That amplification is this branch's doing, so a directory that cannot be opened as a repository, or has no HEAD — what an interrupted clone leaves — is discarded and cloned again rather than trusted as a cache.

Failures also print to the console now. The build gates on this, so the reason belongs in the run output; diagnosing this started from an exception that said only that something went wrong.

Checked against all four states: no checkout at all, a good one, one detached with no local master, and one with its .git removed. All four end at the pinned commit.

@codex review

chatgpt-codex-connector

💡 Codex Review

WurstScript/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/StdLib.java

Lines 116 to 117 in 704691e

    
           Git.open(stdLibFolder).clean().setForce(true).setCleanDirectories(true).setIgnore(false).call(); 
        
           Git.open(stdLibFolder).checkout().setName(version).call();

Stop each fork from re-checking out the shared stdlib

With maxParallelForks > 1, the ensureStdLib JavaExec exits before the test workers start, so its process-local isInitialized flag is not inherited; the first StdLib.getLib() in each worker therefore reaches these unconditional clean and checkout calls against the same repository. Two stdlib-using test classes can consequently contend for JGit's index lock—or one can rewrite the checkout while another compiler reads it—making the newly parallel suite flaky. The new prefetch is fresh evidence that the earlier race remains: it serializes the initial clone, but not these per-worker mutations; workers should trust a successfully prefetched checkout or use isolated copies.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

The clean and the checkout ran on every first call in a process, and every worker is a process, so all of them rewrote the one shared directory - each taking the index lock, while the others were reading the same files. It costs nothing to ask first: reading the status writes nothing, and a checkout that is already at the pinned commit and clean has nothing to undo. Only a worker that finds something wrong touches it. Asking first showed that what was there did not do what it said. Checking out the commit HEAD already points at does nothing, so "reset all possible changes" removed untracked files but left a modified one modified. It resets hard to the pinned commit now, which is what the comment always claimed.

Frotty · 2026-08-16T08:51:16Z

Valid, with a caveat on how often it bites.

The structure is exactly as you describe: the clean and checkout sat outside the version check, so they ran on the first call in every process — and every worker is a process. Eight of them rewriting one directory, each taking JGit's index lock, while others read the same files.

Where I would temper it: I have not seen it fail. Three full runs at eight forks passed, because when the checkout is already correct clean finds nothing and checkout is a no-op — they still take the lock, so the window is real but narrow. It is the shape that shows up as occasional CI flakiness rather than a reproducible failure, which is reason enough given the fix is small.

Workers now read the status — which writes nothing — and only touch the directory if something is actually wrong.

Asking first turned up something else: the reset did not do what its comment claimed. Checking out the commit HEAD already points at is a no-op, so // reset all possible changes removed untracked files but left a modified tracked file modified. I found that by dirtying the checkout deliberately to test the repair path, and watching it not repair. It resets hard to the pinned commit now. Verified: modified file restored, untracked file removed, HEAD unchanged.

LuaTranslationTests, InterpreterTests, ExportToWurstTest and DeterministicChecks pass together, which is the parallel path this affects.

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a87a8cc781

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-16T08:57:18Z

+            // taking the index lock, while other workers are reading the same files. Reading the
+            // status does not write anything, and the ordinary case has nothing to repair.
+            try (Git git = Git.open(stdLibFolder)) {
+                if (repaired || !git.status().call().isClean()) {


Clean ignored artifacts before accepting the checkout

When the checkout contains only ignored artifacts, Status.isClean() still returns true, so this guard skips the clean(...setIgnore(false)) call that previously removed them. WurstStdlib2 explicitly ignores _build, objectEditingOutput, wurst.dependencies, and maps; in particular, a stale _build can contain .wurst files, and WurstCompilerJassImpl.addLibDir recursively scans every subdirectory and can let those files override the pinned library packages. Check getIgnoredNotInIndex() as well, or otherwise clean ignored paths without reintroducing unconditional index writes.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-16T08:57:18Z

 test {
+    dependsOn 'ensureStdLib'


Keep unrelated focused tests independent of the stdlib fetch

When the pinned checkout is absent or stale and GitHub is unavailable, this unconditional dependency makes even ./gradlew test --tests ... for a test that never uses the stdlib fail in ensureStdLib before the selected test starts. I reproduced that path with a focused TypeClassTests method and an unreachable remote; previously such a filtered invocation did not enter StdLib at all. Restrict the prefetch to runs that include stdlib-dependent tests, or provide an offline/filtered path so the documented focused-test workflow remains usable.

AGENTS.md reference: AGENTS.md:L97-L103

Useful? React with 👍 / 👎.

…etch Asking whether the checkout is clean does not notice ignored files, so guarding the clean on that alone stopped build leftovers being removed. That is not cosmetic: the compiler scans the library folder recursively and puts every .wurst file it finds under its package name, so a stale _build can quietly take a pinned package's place. Removing them per worker is what the guard was avoiding, though. The prefetch does it instead - it runs alone, before the workers, which is the one moment rewriting that directory disturbs nobody. Workers still repair a checkout at the wrong commit or with modified files, which is what a run outside the build sees. The fetch is also skippable with -PskipStdLibFetch, so a focused run of tests that never touch the library is not stopped by being unable to reach GitHub.

Frotty · 2026-08-16T09:55:14Z

Both valid, both fixed.

Ignored artifacts. Correct, and I confirmed the consequence rather than assuming it: addLibDir recurses into every subdirectory and does getLibs().put(libName, f) for each .wurst it finds, so a stale _build really can take a pinned package's place — quietly. And WurstStdlib2 does ignore /_build, objectEditingOutput/ and wurst.dependencies.

Checking getIgnoredNotInIndex() in the worker would have reintroduced exactly what the guard was avoiding, since those artefacts appear during a run and removing them mid-run is the contention in question. So the deep clean moved to the prefetch, which runs alone before the workers — the one moment rewriting that directory disturbs nobody. Workers still repair a wrong commit or modified tracked files, which is what a run outside the build sees. Verified: a planted _build/Stale.wurst is removed by the prefetch and left alone by workers.

Focused runs offline. Also correct, and it is a documented workflow, so: -PskipStdLibFetch skips the task. I had to read the property at configuration time rather than in onlyIf — referencing project there fails under the configuration cache, which this build enables.

LuaTranslationTests, InterpreterTests, ExportToWurstTest and DeterministicChecks pass together, and the checkout is clean afterwards.

@codex review

chatgpt-codex-connector · 2026-08-16T10:00:20Z

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: 645b943299

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Frotty added 4 commits August 15, 2026 10:26

chatgpt-codex-connector Bot reviewed Aug 15, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Aug 16, 2026

View reviewed changes

Frotty added 8 commits August 16, 2026 09:11

Merge branch 'interpreter/type-parameter-defaults' into build/determi…

9f6e101

…nism-and-parallel-tests

Sanitise Lua method names where they are assigned (#1230)

b9fda9d

Bound the wait on the Lua interpreter in tests (#1231)

ce3a1ea

Let declared names decide dispatch slots, and finish the FastHashMap …

7fa5c15

…proof (#1232)

Take a closure's instantiation from its construction on Lua (#1233)

886f556

Resolve a type parameter's default where the binding is known (#1234)

5ccc791

chatgpt-codex-connector Bot reviewed Aug 16, 2026

View reviewed changes

Frotty merged commit ae1583d into interpreter/type-parameter-defaults Aug 16, 2026
6 checks passed

Frotty deleted the build/determinism-and-parallel-tests branch August 16, 2026 10:13

Frotty mentioned this pull request Aug 16, 2026

Make generated Jass comparable across runs, and cut the suite from 13 to 7 minutes #1236

Merged

	Git.open(stdLibFolder).clean().setForce(true).setCleanDirectories(true).setIgnore(false).call();
	Git.open(stdLibFolder).checkout().setName(version).call();

Uh oh!

Conversation

Frotty commented Aug 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Determinism

Suite time

Also included

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 15, 2026

Choose a reason for hiding this comment

Uh oh!

Frotty commented Aug 16, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

Frotty commented Aug 16, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

Frotty commented Aug 16, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Frotty commented Aug 16, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

Frotty commented Aug 16, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Frotty commented Aug 15, 2026 •

edited

Loading