Repository navigation
A project's unit builds asked for together build in one run, and a PC build configures again only when its files change - #180
Open
silvervest wants to merge 21 commits into
Open
silvervest wants to merge 21 commits into
silvervest wants to merge 21 commits into
Conversation
added 6 commits
November 2, 2026 23:12
…nly its object failed_units matched the build's failed targets against each unit's object. A unit built in steps (parts compiled, then linked into its object) reports only the step that failed: with -k 0, ninja never tries the object. On an x86 game, edit_file kept a new file of stand-ins that didn't compile as breaking nothing; the agent's next compile then found the unit broken, was told it had been broken before the change, and lost its claim. That happened 302 times over 156 functions in a week. failed_units now also counts every target made from a failed one (ninja -t query, followed up to the unit's object), so the edit is undone with the build's error instead.
…nction came Where a function's code changes how its neighbours compile (inlined into them, or whole-program optimisation choosing registers across functions), a draft short of exact breaks a matched neighbour, and the compile is undone. The function's own score went with it: on an x86 game, agents of 63 functions wrote about this, and parked drafts behind #if flags they rebuilt by hand just to measure them. The rejection now gives the function's score and diff, says that such neighbours match again once it matches exactly, and records the attempt with its score. It never becomes the best attempt (best=False in _record): briefs offer the best to start from and a release puts it back in the file, where this code can't stay. Nothing counts as a match that didn't before.
…ng the function's list of names The message's list of broken functions was put in `names`, the variable that holds the ways the function may be spelled. Nothing read it after (the branch returns), but the next change to that branch could have.
…ion's unique name A function whose row name was made unique with a trailing "'" (a re-sync after a split) was reported as "its object doesn't define _f@4'". Agents took the quote for the defect, said no C makes such a name, and stopped looking for why the function really wasn't in the object.
…never compiled into On an x86 game, agents parked drafts of 13 functions (about 18 KB) in `#if defined(SOME_FLAG)` blocks no build defines. The brief showed each as the function's current source, the score as its own, and every compile replaced the definition inside the block, where the judge never saw it. csource.hidden_by follows a file's conditionals and says which branch leaves a position out, only when it can be sure: a test of a macro that may be defined (the compiler's own, one the build passes) hides nothing. ForkProject.never_defined finds the macros nothing could define (no #define in the project's sources, no mention in its build files, -D/ included); that search runs only when the code would be hidden with every such macro undefined, so files with no parked code pay nothing for it. The brief now marks a hidden definition, and a compile that would replace one is refused before anything is built, with how to bring it out.
… where the build sees it Refusing to compile into a parked definition left an agent stuck: on an x86 game the drafts were parked because, where the build sees them, they break a matched neighbour (whole-program optimisation ties their code together), so edit_file refused to move them out too, and the agent could do nothing with the function at all. A compile now puts the agent's version where the build sees it, by its place in the original, without the parked copy, and carries the copy's reccmp annotation (and its comment, unless the agent sent one) along with it. Kept, the function is visible from then on; broken neighbours undo it, and say how close the function came. The brief says so. csource.marker_above finds the annotation above a definition's comment.
This was referenced Nov 2, 2026
n-delic
requested changes
Nov 3, 2026
n-delic
left a comment
n-delic
left a comment
Owner
There was a problem hiding this comment.
Thanks. Building requested units together is a real speed-up, and each caller getting its own copy of a run's exception is right. A batched build must give each unit the same verdict as building it alone, though, and a few cases break that. (Reviewed this PR's own five commits, 2a97d07…395c847; the full diff also carries #178 and #179.)
1. One unit's output can mark another unit failed (forks/project.py:117-132, _judge_batch :710-734). _failures treats any line starting FAILED: as ninja's step header, including a line a build command prints, and blames the unit it names. Repro: unit a's step succeeds; unit b's step prints FAILED: build/a.o and exits 1. Then build_unit(a) gives (True, …), but build_units([a, b]) gives [(False, …), (False, …)]. Don't derive verdicts from parsed lines: after a failed batch, use _no_work per unit and build the units with work left alone. Add a test where a step prints another unit's FAILED: line.
2. The timeout kills ninja and leaves the compiler running (:480-482, :488-490, proc.kill()). Repro: a rule whose fake compiler sleeps 30 s; _execute(timeout=2, hard=True) returns after 2 s, and the compiler's PID is still alive. On Windows, kill() is TerminateProcess on ninja only. That's the hung-compiler case this PR is about. Use hostenv.stop_process_tree (or the process group) and test that the child is gone.
3. Conflict with open #114, and whole builds still hang the old way. #114 changes the same ForkProject.run timeout path (stop_process_tree, BUILD_OUTPUT_GRACE, an actionable message). Cherry-picking it onto this gives a conflict in forks/project.py. #114's test also fails here: run(sleep 5 s, timeout=2) with the default hard=False returns (True, …) after 5 s, because _execute still reads to EOF before wait. That's the path build_all, verify, reconfigure and setup use. Fold #114's approach into _execute for both modes.
4. Batched failure logs drop buildhelp.explain's hints (build_units discards them at :697), so a unit's batched log isn't "what building it alone would say".
5. The PC "configure only when files come or go" claim isn't true (configure.py:127-149, 204-213). build.ninja depends on configure.facts, whose inputs are the old file list:
- a file added alone is never seen (
ninja: no work to do, no object); - a removed file makes ninja fail before configure runs ("'src/auto/sample.cpp', needed by 'build/TEST/configure.facts', missing").
The test at tests/test_pc.py:268-273 passes only because it also edits an existing source. Make the facts step always run and rewrite facts only when the list changes, and test adding and removing a file alone.
Smaller: the timeout log is just "timed out" (#114's message says what to do), and the batch-timeout fallback has no test.
added 15 commits
November 3, 2026 20:21
… is one run of tries Every variant the permuter tried on a project's function was built twice: once as the variant, then again as the file it put back. On an x86 game built with link-time code generation a build is a ~5 s whole-unit link, so one variant took ~11 s and half of it went to the rebuild (a minute's turn tried five). ForkEngine.trial_run holds the function's unit for the turn: a try that isn't kept puts its file back without building it, the next try's baseline comes from the scores already known for that text, and the unit is built once at the end, as its file is. A unit linked into the game still takes the tree alone for each try, as before.
On an x86 game built with MSVC, decomp-permuter read none of the near misses it was given: one line of the shared header (`typedef unsigned __int64 ...;`) stopped pycparser on every function, and past it were `__declspec`, classes with methods, references and the types of system headers. Every turn fell back to the simple rewrites. The generator now gets the part of the header the function uses (seeds.HeaderIndex, read after MSVC's words go, as `struct __declspec(align(16)) T` was a struct named `__declspec` to it), and reads it as C: object-like macros put in, `__int64` as a stand-in type, calling conventions and `__declspec` dropped, classes as structs of their data, every tag a type by its own name, references as pointers, the undeclared types of system headers declared. In the function's own C, MSVC's words are noted and put back into each variant, so it compiles as the source did. On that game's near misses with only register, stack or order differences, the headers of all of them now parse (none did) and 43 of 185 parse whole; the rest are C++ in the function itself, which it still doesn't read.
A PC or Xbox game's function is named by its mangled symbol (`?Update@@Yaxh@Z`), and the generator was asked for that: no C spells it, so once decomp-permuter could read an x86 game's code, every turn ended with "Function ... not found in base.c" and fell back to the simple rewrites. ForkEngine.permuter_name gives the spelling the definition has, from the same names the engine finds definitions by.
…nd constructors Most of an x86 game's near misses are C++: `Class::method` definitions, template calls, references, locals built by their constructors. pycparser reads none of it, so those functions only ever got the simple rewrites. The generator's Restorer turns each into C pycparser reads and back, the way it already does MSVC's words: `A::B` and `A::~A` become plain names, a template-id followed by a call becomes a name noted in a table, a reference in a declaration a pointer named for it, `T t(a, b);` and `T(a, b)` calls of stand-ins, and a method's trailing `const` is noted and put back. The header's types (declared_types) tell a constructor from a call. The randomizer moves the stand-ins about like anything else; what comes back wrong costs a compile, and only the judge's score counts. On an x86 game's near misses with only register, stack or order differences, 169 of 185 now parse (43 did); the rest are macros that take a type, placement new and initializer lists.
…cast to a system type Two misses found by restoring decomp-permuter's own unchanged print of an x86 game's near misses and judging it: - `T *__stdcall f(` came back as `T __stdcall *f(`, which doesn't build: a calling convention goes before a pointer's `*` only where it was (a pointer to a function), else right before the name. - `(PDWORD) &x`, a cast to a type no header the permuter reads declares, was read as a call and came back as one, which doesn't build: a name only ever written in parentheses before an operand is declared a type.
decomp-permuter's inlining writes a helper function beside the function, and a fork's compile_and_diff refuses a helper sent with it. That NetworkError reached _turn, which takes a refusal for the best attempt being refused every time and leaves the function to agents for good: on an Xbox game, 2 of 3 near misses lost every turn they had within seconds of their first. A refused variant is now skipped like one that doesn't compile; only when every variant of a turn is refused is the refusal handed on, as before.
… type the header lacks, and a method on a renamed class On an Xbox game 43 of 352 near misses were C decomp-permuter couldn't read, so their turns had the simple rewrites only. 13 stopped at a macro: one called with a type (`F(int, 0x20)`, `MP_RAW(bool, 0x62C)`), defined in the header or the source itself, or an object-like one in the function's C (`MATCH_VOLATILE`); those are put in where they're used now, and a call of values stays a call. About 15 stopped at a local of a type the header doesn't have (the SDK's WAVEFORMATEX and BOOL, a type declared elsewhere): two names in a row or `T *p =` at a statement's start make it a type. And 6 were methods defined on a class the source names differently from the symbol (`C_vt_003AB500::Func_E46D0`): permuter_name now gives the definition's own spelling, which the generator looks the function up by.
…s own words `#define CPlayerCar CPlayerCarRaceView` put in renamed the function's own class, so decomp-permuter didn't find it: a macro naming another name reads as the name it is and stays. And a local's type needs two names in a row, not one name cut in two (`limit = 1.2f;` was read as a `limi` declaring `t`).
…ace, and a C++ function's classes Review of n-delic#179: - msvc_as_c leaves preprocessor lines and strings as they are, and takes only spaces and tabs after a word it removes: `#define WINAPI __stdcall` lost its name, `#define STDCALL __stdcall` its value (so variants lost the convention), and the newline after a word joined two lines. A macro standing for MSVC's words is put in where the function uses it, so its convention is kept like any written one. - A const method's `const` is marked on the definition itself (a last parameter of the stand-in type DDconst): by name it went back after the first `(...)`, so `operator()(int a) const` became `operator() const(int a)` and two const overloads got `const const` and none. - A C++ function's header keeps its `#ifdef __cplusplus` branch (seeds.c_only(cxx=True)), where its classes are; a C function's is still C's. - permuter_gen's docstring says what C++ isn't read yet (enum class, std:: containers, operators other than (), noexcept, throw(), new/delete), and a test pins each to the simple rewrites.
…edit build.ninja depended on every source and header, so each edit wrote the whole build again before compiling: on an LTCG game of 1,400 units, 3.1 s of every 4.2 s compile (the unit's own compile, link and judging take under one), for agents and for each of the permuter's variants. configure reads no file's contents, only which files there are, so build.ninja now depends on build/<version>/configure.facts, the lists of sources and headers, which the build brings up to date first (restat): configure runs when a file is added or removed, as before, and a plain edit compiles at once.
A project's builds take turns, as they share ninja's log: its permuter workers and agents queued for one unit at a time, while a run's own cost (reading the build file, configure's facts) is half a unit's build. On an x86 game's LTCG units, 8 built one by one took 5.6 s and 1.3 s in one run. The builds asked for while a run goes now go together in the next (_UnitBuilds, up to 16): one `ninja -k 0` of their objects (ForkProject.build_units), each unit told what building it alone would say. A unit whose step failed sees that step's failure and no other unit's; a run whose failure no unit owns (ninja stopping before it builds: configure failing, a target with no rule) builds each unit alone; one unit, and builds that aren't ninja, build alone as before. A build asked for during a run waits for the next run, as the running one may be building an older copy of its file.
… build lock unname() took the build lock to write back the names a try changed, and nearly every try changed none. With the builds asked for together running under that lock, each of the permuter's tries waited for the next run here, writing nothing: two of four workers sat in it at any moment, and batching gained 30% where one run of 8 units is four times faster than 8.
From a review of the batched builds. A unit without a failure of its own was taken as built: a run the memory cap ends, or a dependency walk cut short, left its old object to be judged as its new file's. Ninja is now asked (a dry run) whether the rest are built, and when it isn't sure they're built alone. A failed step making two files is one failure in its unit's log, not two; a run's time grows with its units, and one that runs out of time says so for each, rather than building them all again one by one under the build lock; a problem putting failures to units builds each alone; each thread raises its own copy of a run's exception; a project's batches are made under the engine's lock, as two first builds at once made two of them; and a built unit gets no log, as no caller reads it and what a run of several printed is theirs together.
… and a run of builds gives no verdict it can't stand by From a second review of the batched builds. A unit's build read its output to the end before its time limit was looked at, so a compiler hung under wibo held the build, and the project's build lock, forever (every build in the app had this; unit builds now read on a thread and stop at the limit). A run of several now has one unit's limit, and builds each alone when it runs out, as one unit's hang shouldn't fail the others; a run the memory cap ended (it covers the whole run, and kills whichever step is biggest) builds each alone too. Built means ninja says "no work to do" (a user's NINJA_STATUS or a project's --quiet took the progress lines the dry run was read by), asked of each unit when the batch's answer is no, so only the unit with work left builds again; and a unit whose own build raises gets the exception for its caller alone, not every caller of the run.
…at its limit takes its compilers with it, and PC configure sees a file added or removed alone Review of n-delic#180: - After a run of several units fails, each unit is built (ninja's dry run says so) or is built alone, so its verdict and log are what building it alone gives, buildhelp's hints too. What the run printed isn't read for whose failure it was any more: a step's own output saying `FAILED: <another unit's object>` failed that unit. - Every build step's time limit holds while the step still prints or hangs with its output open, not only a unit's (a whole build, verify, reconfigure and setup read their output to the end first), and a step stopped at it is stopped with everything it started: on Windows its process tree, elsewhere ninja is asked to stop its commands and its process group is then killed. Killing ninja alone left the compiler running. This is n-delic#114's fix (its reader thread, stop_process_tree and its message), folded in for both modes with its test. - PC configure's facts depend on the files they list and the folders holding them, each also a phony with no inputs: a file added on its own was never seen, and one removed stopped ninja before configure could run.
silvervest
force-pushed
the
perf/batched-unit-builds
branch
from
November 3, 2026 09:29
395c847 to
c493cab
Compare
Author
|
Thanks, all addressed in the new last commit ("A batched unit gets the verdict its own build gives, ..."):
|
n-delic
reviewed
Nov 4, 2026
n-delic
left a comment
n-delic
left a comment
Owner
There was a problem hiding this comment.
Thanks. All five points from the last round are fixed:
- no more attribution from
FAILEDlines (the per-unit_no_work); _stop_treestops the whole process group (taskkill on Windows);- #114's behaviour is folded in;
- the hints are kept;
- the PC facts edge case is handled.
It's good to go once it's rebased. It now conflicts with main (DIRTY), and the conflict in forks/project.py matters: main added run(..., platform=...), which consoles/pc/scratch.py calls, and this branch's run signature would drop it. Keep the platform argument through the rebase. CI can't run until the conflicts are resolved.
This branch has not been deployed
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
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.
Merge order: after #178 and #179. This PR builds on them: until they merge it shows their commits too, so review only its own 5 (the last ones). #185 builds on it.
Improvement.
On an x86 game of 1,400 units built with link-time code generation, a compile took 4.2 s, of which 3.1 s was configure writing the whole build again (build.ninja depended on every source and header), and the project's builds took turns, one unit at a time, while a run's own cost was half a unit's build.
build/<version>/configure.facts, the lists of sources and headers, which the build brings up to date first (restat)._UnitBuilds, up to 16): oneninja -k 0of their objects, each unit told what building it alone would say. 8 units: 5.6 s one by one, 1.3 s in one run. A try with nothing renamed puts its file back without waiting for the build lock.Tests: 7 in tests/test_forks.py (builds asked for during a run go together, each unit hears only its own failure, built only when ninja says so, the memory cap, a hang with its output open, a step making two files, a run that stops before building) and
test_a_pc_build_configures_again_only_when_its_files_change(tests/test_pc.py).ruffis clean.🤖 Generated with Claude Code