diff --git a/.github/workflows/build-memory.yml b/.github/workflows/build-memory.yml new file mode 100644 index 0000000000..0268a6949e --- /dev/null +++ b/.github/workflows/build-memory.yml @@ -0,0 +1,305 @@ +# Per-TU peak RSS, published as an artifact — issue #1305. +# +# WHY THIS WORKFLOW EXISTS +# ------------------------ +# Every self-hosted Linux build in this repo runs at `--parallel 2` on a 16-thread host, +# each runner unit carries a 14 GiB `memory.max` and the account slice carries 19 GiB. +# All of those came from a belief that one `clang++` TU needs ~9 GB. Five sources +# disagreed about that number by 3x (asan.yml's comments said ~3 GB; a hand measurement +# on the box said 9.0 GB with one TU at 13.9 GB; the slice drop-in justified itself as +# "~2 x 9 = 18 G"), and nothing measured it on a schedule. +# +# #759 and #822 wired the CMake Instrumentation API, which records per-command TIME and a +# SYSTEM-WIDE host-memory sample — it cannot attribute memory to a TU at all. So the +# "handful of heavy TUs" everything rests on was identified by compile time, a proxy. +# cmake/ProcStatReport.cmake closes that with clang's `-fproc-stat-report`, and this +# workflow is the half that stops the number decaying again: #759's one-off measurement +# rotted exactly the way an uncommitted number does, and a committed recipe is not a +# substitute for a published artifact somebody can diff against last month's. +# +# WHAT IT DOES NOT PROVE +# ---------------------- +# Nothing about Windows or MSVC. `-fproc-stat-report` is clang-only, and the `msvc` +# preset is not the host whose caps are in question — deliberately out of scope rather +# than unmentioned. Nothing about vcpkg ports or FetchContent dependencies either: they +# build outside this project's add_compile_options() reach, the same blind spot #759 had. +# +# THE COMPILER CACHE IS OFF HERE, AND THAT IS THE POINT +# ----------------------------------------------------- +# A cache HIT runs no compiler, so it appends no record. A measurement run on a warm +# cache therefore produces a ranking of whatever happened to miss — which looks exactly +# like a cheap build. `CCACHE_DISABLE=1` buys a complete census, and the analyser's +# `--fail-under-coverage` gate fails the job rather than publishing a partial one. +# This is a MEASUREMENT job, not a build gate; the ordinary jobs keep their cache. +name: Build memory (per-TU peak RSS) + +# Weekly and on demand, because what it defends against is a number DRIFTING over +# months, not a specific PR breaking something. The pull_request paths are deliberately +# narrow — the files the measurement itself is made of — rather than `cmake/**`: this is a +# full cold Debug build of the engine plus the test suite, and the box sits at rho 1.69 +# (#1219, #1284). Anything wider would spend that on every PR that touches CMake for +# unrelated reasons, and a pull request runs ONE cell rather than both (see the job's `if`). +# +# NOTE, because it is not obvious and it cost this PR three redundant runs: GitHub matches +# a pull_request `paths` filter against the WHOLE PR DIFF, not against the latest push. So +# once a PR touches any file above, EVERY subsequent push to it re-runs this workflow. +on: + schedule: + - cron: '0 7 * * 4' # weekly, Thursday 07:00 UTC — off video-ffmpeg's Monday and steam-stub's Wednesday slots + workflow_dispatch: + pull_request: + paths: + - '.github/workflows/build-memory.yml' + - 'cmake/ProcStatReport.cmake' + - 'scripts/analyze_proc_stat.py' + # The root CMakeLists declares OLO_BUILD_INSTRUMENTATION and include()s the module, + # so a wiring regression lives here just as much as in cmake/ProcStatReport.cmake — + # and it would make this workflow vacuous rather than red. It is a frequently-touched + # file, which is exactly why the `if:` on the job below drops the non-sanitizer cell + # on pull requests: widening the trigger without halving the cost would put two cold + # builds on the box for every unrelated CMake change. + - 'CMakeLists.txt' + +# Reads the checkout, builds, and uploads an artifact. Nothing comments, labels or +# publishes to the repo. Declared explicitly rather than inherited so a future step +# cannot quietly acquire write scopes — same as asan.yml and steam-stub.yml. +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + # Never cancel a non-PR run: the weekly measurement is the artifact history this whole + # workflow exists to build, and a cancelled run leaves a gap in it. + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +jobs: + rank: + name: Rank ${{ matrix.label }} + # SELF-HOSTED WHEN THE CODE IS OURS, hosted Ubuntu when it is not. The condition IS + # the security boundary — see vulkan-off.yml for the full reasoning. NOT ON THE BOX + # FOR A `push` (#1084); this workflow has no push trigger, but the disjunct is kept + # so the expression stays copy-comparable with its four siblings. + # + # The box is the RIGHT host for this, not merely an available one: the caps being + # justified are its own, and its toolchain is the pinned LLVM 23.1.0 tarball rather + # than apt clang-23. A hosted fallback still answers the ranking question — per-process + # peak RSS is a property of the toolchain and the TU, not of the host — but the + # absolute numbers a cap should be set from should come from the box. + runs-on: ${{ vars.OLO_LINUX_SELF_HOSTED == 'true' + && github.event_name != 'push' + && (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository) + && fromJSON('["self-hosted","linux","x64","olo-ci"]') || 'ubuntu-24.04' }} + # BOUNDED, because this job is cold BY DESIGN. Disabling the compiler cache means + # every run is a full from-scratch build, so it never gets the warm path that lets + # the sanitizer jobs sit under the 360-minute default. asan.yml's TSan note measures + # a cold instrumented Linux build at ~57 min of build time (~75 min for the arm); + # this job compiles the same tree and runs no tests, so 180 is generous. The point + # is the ceiling: a runaway must not hold one of the box's two slots for six hours + # while the queue behind it grows (#1219, #1284). + timeout-minutes: 180 + strategy: + # ONE AT A TIME. Both cells are full cold builds, and the box has exactly two + # olo-ci slots — running them together would hold the whole box and starve every + # other workflow behind it. Serial costs wall-clock this job does not have to + # optimise for: nothing waits on a weekly measurement. + max-parallel: 1 + # A failing cell must not hide the other's numbers: a plain-Debug ranking is + # useful on its own even if the sanitizer cell OOMs, and vice versa. + fail-fast: false + # ONE CELL ON A PULL REQUEST, both on the schedule and on demand, and the selection + # MUST happen here rather than in a job-level `if:`. + # + # A job-level `if` cannot see the `matrix` context — only github, needs, vars and + # inputs are available there. Writing `if: matrix.slug == ...` does not merely + # evaluate false, it makes the whole workflow FILE invalid: GitHub rejects the run + # with "This run likely failed because of a workflow file issue" and no job starts, + # so the workflow silently stops guarding anything. Building the matrix with + # fromJSON is the supported way, because `strategy` can read the github context. + # + # The third cell PRICES A LEVER rather than tracking drift: `-gsplit-dwarf` keeps + # debug info out of the objects so the linker never loads it, which is the most + # direct attack on the 11.17 GiB link #1313 is about. It is OFF in every real build + # (it costs inlined frames in sanitizer stacks while -ffile-prefix-map is in effect — + # cmake/SplitDwarf.cmake has the measurement), so this cell exists purely to answer + # "what would it buy?" with a number. Compare its link peak against `debug-asan` from + # the same run; both publish their full build configuration alongside the ranking. + # + # The cells: `debug-asan` is the configuration the sanitizer `--parallel 2` pins and + # the 14 GiB unit cap were derived from — Debug, ASan, full -g, no split DWARF — and + # is the stricter of the two, so a PR proves the instrumentation end to end with it + # alone. Plain `Debug` is NOT redundant on the schedule: steam-stub.yml's + # non-sanitizer build is pinned to `--parallel 2` too, and its own comment says the + # reason is the NEIGHBOUR ("the cap is on the account, not on the job"), so what a + # plain Debug TU costs is a question in its own right. Tracking that over time is + # the weekly run's job, not a PR's. + matrix: + include: ${{ github.event_name == 'pull_request' + && fromJSON('[{"label":"Debug + ASan","slug":"debug-asan","cmake-args":"-DOLO_ENABLE_ASAN=ON"}]') + || fromJSON('[{"label":"Debug + ASan","slug":"debug-asan","cmake-args":"-DOLO_ENABLE_ASAN=ON"},{"label":"Debug","slug":"debug","cmake-args":""},{"label":"Debug + ASan + split DWARF","slug":"debug-asan-split-dwarf","cmake-args":"-DOLO_ENABLE_ASAN=ON -DOLO_SPLIT_DWARF=ON"}]') }} + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + # Python, the shader toolchain and clang, resolved for whichever runner kind this + # landed on. `cache-name` still names a ccache directory; the build below disables + # ccache per-invocation rather than by unsetting it, so the action stays unmodified + # and the same directory keeps serving every other workflow. + - name: Setup Linux build environment + id: linux + uses: ./.github/actions/setup-linux-build + with: + cache-name: build-memory + + - name: Setup vcpkg + id: vcpkg + uses: ./.github/actions/setup-vcpkg + with: + triplet: x64-linux + + # The heavy optional subsystems are off for the same reason steam-stub.yml turns + # them off: the question is what the ENGINE's own TUs cost. USD and Alembic are + # vcpkg ports whose own compiles this instrumentation cannot see anyway. + # + # -DOLO_ENABLE_COMPILER_CACHE=OFF comes AFTER extra-cmake-args, which sets it ON — + # CMake takes the last -D for a given variable, so the order is load-bearing. The + # cache is disabled at BOTH levels: here, so no launcher is wired and PCH/unity + # keep the same forced-off settings the cached CI jobs build with, and via + # CCACHE_DISABLE at build time as a belt-and-braces guard. + - name: Configure (instrumented) + run: | + cmake -S . -B build \ + -DOLO_BUILD_INSTRUMENTATION=ON \ + -DOLO_WITH_USD=OFF -DOLO_WITH_ALEMBIC=OFF -DOLO_WITH_MATERIALX=OFF \ + -DOLO_VIDEO_FFMPEG=OFF \ + -DCMAKE_CXX_COMPILER=${{ steps.linux.outputs.cxx }} \ + -DCMAKE_C_COMPILER=${{ steps.linux.outputs.cc }} \ + -DCMAKE_BUILD_TYPE=Debug \ + -DCMAKE_EXE_LINKER_FLAGS=-fuse-ld=lld \ + -DBUILD_TESTS=ON \ + ${{ steps.vcpkg.outputs.toolchain-args }} \ + ${{ steps.linux.outputs.extra-cmake-args }} \ + ${{ matrix.cmake-args }} \ + -DOLO_ENABLE_COMPILER_CACHE=OFF \ + -DOLO_ENABLE_PCH=OFF \ + -DOLO_ENABLE_UNITY_BUILD=OFF \ + -DPython_EXECUTABLE="${{ steps.linux.outputs.python-exe }}" + + # Fail loudly if the instrumentation did not actually take. Without this the job + # publishes an empty ranking and reads as "nothing used much memory" — the precise + # shape of the silent decay this workflow exists to prevent. Checked on the + # GENERATED build system, not on the CMake cache: the cache says what was asked + # for, the makefiles say what will run. + - name: Verify the flag reached the compile and link lines + run: | + if ! grep -q '^OLO_BUILD_INSTRUMENTATION:BOOL=ON' build/CMakeCache.txt; then + echo "::error::OLO_BUILD_INSTRUMENTATION did not resolve to ON." + exit 1 + fi + # Unix Makefiles keeps a target's compile flags in its own flags.make. + COMPILES=$(grep -rl 'fproc-stat-report' build --include=flags.make | wc -l) + echo "flags.make files carrying the flag: $COMPILES" + if [ "$COMPILES" -eq 0 ]; then + echo "::error::No compile line carries -fproc-stat-report — cmake/ProcStatReport.cmake did not wire up." + exit 1 + fi + # NO textual assertion on the LINK line here, deliberately. Where the Makefiles + # generator puts an executable's link command is platform-dependent: it writes a + # separate link.txt in some configurations and inlines the command into build.make + # in others (observed inlined when generating a Unix Makefiles tree with clang++). + # A grep for one spelling would fail this job for a correct build. The link half is + # asserted on the OUTCOME instead — `--require-link-records` in the Rank step below + # fails unless a real ld.lld record came out, which is generator-independent and a + # stronger claim than any amount of matching on generated text. + # The relative path is what keeps the compiler cache shareable across runner + # slots (measured: an absolute path takes cross-tree hits to 0%). Assert it + # here so a well-meaning "make this an absolute path" edit fails in CI rather + # than quietly costing every other job its cache. + if grep -rh -o 'fproc-stat-report=[^ ]*' build --include=flags.make | grep -qE '=(/|[A-Za-z]:)'; then + echo "::error::-fproc-stat-report was given an ABSOLUTE path. That is hashed by ccache and" + echo "cannot be relativised by base_dir, which drops cross-slot cache sharing to zero." + exit 1 + fi + echo "OK: compiles and links are instrumented, with a relative records path." + + - name: Save vcpkg binary cache + if: always() + continue-on-error: true + uses: ./.github/actions/save-vcpkg-cache + with: + key: ${{ steps.vcpkg.outputs.cache-key }} + path: ${{ steps.vcpkg.outputs.cache-path }} + + # --parallel 2 matches the pins this measurement is meant to inform, so the run + # stays inside the caps that exist today. It does NOT affect the numbers: peak RSS + # is per-process, and the derivation table sums the N heaviest TUs rather than + # observing them concurrently. A wider -j would only risk the OOM this is measuring. + # + # The records files are removed first because clang APPENDS and the self-hosted + # workspace persists between runs — a stale file would silently mix two builds + # into one published ranking. There is one PER SUBDIRECTORY under the Makefiles + # generator this job gets (the compile rule `cd`s into each), hence the find. + - name: Build (instrumented) + env: + CCACHE_DISABLE: '1' + run: | + find build -name olo-proc-stat.csv -delete + cmake --build build --target OloEngine-Tests --parallel 2 + + # Non-fatal on its own: a build that reached the link step has already produced the + # compile records, and losing the ranking to a missing analyser dependency would be + # a worse outcome than a red job with the artifact attached. The coverage gate below + # is what actually decides whether the numbers are publishable. + # RUNS EVEN IF THE BUILD FAILED, which is the case where the records are most worth + # having: a build killed by the OOM killer leaves behind exactly the ranking that + # explains why. Gated on the setup step rather than on nothing, so a job that never + # got a toolchain does not go looking for records that cannot exist. The coverage + # gate below still fails on a partial ranking — correctly, because a partial ranking + # IS a lower bound — and `if: always()` on the upload keeps the artifact either way. + - name: Rank by peak RSS + id: rank + if: ${{ !cancelled() && steps.linux.conclusion == 'success' }} + run: | + mkdir -p build-memory-report + # The BUILD TREE is the argument, not a file: the Makefiles generator writes + # one records file per subdirectory and the analyser gathers them all. The raw + # records ride along in the artifact, concatenated with their source paths so a + # future reader can re-derive the ranking without trusting this script. + find build -name olo-proc-stat.csv -print -exec cat {} \; \ + > build-memory-report/olo-proc-stat.raw.txt + "${{ steps.linux.outputs.python-exe }}" scripts/analyze_proc_stat.py \ + build \ + --build-dir build \ + --top 20 \ + --cap-gib 14 \ + --max-lanes 16 \ + --markdown build-memory-report/report.md \ + --json build-memory-report/report.json \ + --require-link-records \ + --fail-under-coverage 95 + + # The artifact IS the deliverable — and so is the job summary, because an artifact + # nobody downloads decays the same way an uncommitted number does. + - name: Publish the ranking to the job summary + if: always() + run: | + if [ -f build-memory-report/report.md ]; then + { + echo "# ${{ matrix.label }} — ${{ runner.name }}" + echo + cat build-memory-report/report.md + } >> "$GITHUB_STEP_SUMMARY" + else + echo "No report was produced — see the Rank step." >> "$GITHUB_STEP_SUMMARY" + fi + + - name: Upload the ranking + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: build-memory-${{ matrix.slug }} + path: build-memory-report/ + if-no-files-found: error + # Long enough to diff against next month's run, which is the whole anti-drift + # mechanism, without holding a year of CSVs. + retention-days: 90 diff --git a/CMakeLists.txt b/CMakeLists.txt index 4dd22d75f0..af4feb2349 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -569,10 +569,24 @@ math(EXPR OLO_ARCHIVE_FAIL_BYTES "(4294967296 / 100) * ${OLO_ARCHIVE_FAIL_PERCEN # left off: it inflates snippet volume considerably for data this project # doesn't currently consume — timing and host memory before/after (the two # questions #759 asked) don't need it. +# +# THE option() IS DELIBERATELY OUTSIDE THE 4.3 GUARD (issue #1305). It is now the +# switch for two independent mechanisms with different requirements, and nesting it +# would lend one of them a gate it does not need: +# +# - cmake_instrumentation() below: needs CMake 4.3+ AND a Makefile/Ninja/FASTBuild +# generator. Still guarded, unchanged. +# - cmake/ProcStatReport.cmake (per-invocation peak RSS): a clang COMMAND-LINE flag. +# No CMake version floor, no generator restriction — it works under the Visual +# Studio generator too, which is the one place the instrumentation API cannot go. +# +# On CMake < 4.3 the option now simply exists and drives the second mechanism, instead +# of producing a "Manually-specified variables were not used by the project" warning +# that made -DOLO_BUILD_INSTRUMENTATION=ON look like a typo. +option(OLO_BUILD_INSTRUMENTATION + "Instrument this build tree: CMake Instrumentation API trace data (compile/link/custom timing + host memory before/after; Ninja/Makefile/FASTBuild only) AND per-invocation peak RSS for every compile and link (clang only — see cmake/ProcStatReport.cmake, issue #1305)." + OFF) if(CMAKE_VERSION VERSION_GREATER_EQUAL 4.3) - option(OLO_BUILD_INSTRUMENTATION - "Emit CMake Instrumentation API trace data (compile/link/custom timing + host memory before/after) for this build tree. Ninja/Makefile/FASTBuild only; no effect under other generators (Visual Studio, Xcode, ...)." - OFF) if(OLO_BUILD_INSTRUMENTATION) if(CMAKE_GENERATOR MATCHES "Makefiles|Ninja|FASTBuild") cmake_instrumentation( @@ -591,6 +605,19 @@ if(CMAKE_VERSION VERSION_GREATER_EQUAL 4.3) endif() endif() +# The field the API above cannot record: per-invocation PEAK RSS, for compiles and links +# alike (issue #1305). Rides the same OLO_BUILD_INSTRUMENTATION switch and returns +# immediately when it is OFF, so an ordinary build is untouched. MUST stay above the +# add_subdirectory() calls: it works through add_compile_options()/add_link_options(), +# which are read when a target is created. +include(cmake/ProcStatReport.cmake) + +# Split debug info out of the objects so the linker never loads it (issue #1313). OFF by +# default and measured rather than assumed — cmake/SplitDwarf.cmake has the symbolization +# trade-off in full. MUST stay above the add_subdirectory() calls for the same reason as +# the include above: it works through add_compile_options(). +include(cmake/SplitDwarf.cmake) + set(CMAKE_CXX_STANDARD 23) set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_EXTENSIONS ON) diff --git a/OloEditor/CMakeLists.txt b/OloEditor/CMakeLists.txt index e7160bd0e9..b7c681bb4a 100644 --- a/OloEditor/CMakeLists.txt +++ b/OloEditor/CMakeLists.txt @@ -5,6 +5,32 @@ # Main targets built by this project add_subdirectory(src) +# OloEditorCore (issue #1314) carries the editor's headless automation/MCP core, shared +# with OloEngine-Tests. Its usage requirements are PUBLIC on purpose: the tests link this +# library instead of recompiling its sources, so whatever the sources need to compile and +# link must travel with it rather than being restated in the test CMakeLists — restating +# is how the two drifted into compiling the same files with different settings. +olo_set_compiler_options(OloEditorCore) +olo_set_common_definitions(OloEditorCore) +olo_enable_lto(OloEditorCore) +olo_set_common_include_directories(OloEditorCore) +target_include_directories(OloEditorCore PUBLIC + ${CMAKE_CURRENT_SOURCE_DIR}/src + # The ContentBrowser helpers are included as bare "DirectoryTree.h" by their siblings. + ${CMAKE_CURRENT_SOURCE_DIR}/src/Panels/ContentBrowser +) +# PUBLIC: OloEngine is part of this library's interface, and httplib appears in the MCP +# server headers a consumer includes. +target_link_libraries(OloEditorCore PUBLIC OloEngine httplib::httplib) +# PRIVATE, both of them, and GLFW_INCLUDE_NONE deliberately so. It is what keeps a TU that +# pulls GLFW before glad compiling, and these sources had it in the editor build — but +# OloEngine-Tests compiled this same set for years WITHOUT it, so it is demonstrably not +# part of the interface. PUBLIC would put it on all ~900 test TUs and rebuild them for no +# reason. If a consumer ever does need it, that is a header leaking GLFW and worth fixing +# at the header instead. +target_compile_definitions(OloEditorCore PRIVATE GLFW_INCLUDE_NONE _CRT_SECURE_NO_WARNINGS) +set_target_properties(OloEditorCore PROPERTIES FOLDER "Applications") + # Configure the OloEditor target using our common properties olo_configure_app(OloEditor) @@ -25,6 +51,7 @@ target_include_directories(OloEditor PRIVATE # it here rather than on OloEngine to avoid putting on the whole engine's # include path. target_link_libraries(OloEditor + OloEditorCore OloEngine httplib::httplib ) diff --git a/OloEditor/assets/tests/visual/GroomBinding_GL_Deferred.png b/OloEditor/assets/tests/visual/GroomBinding_GL_Deferred.png index 937e4408a5..c722e709b6 100644 Binary files a/OloEditor/assets/tests/visual/GroomBinding_GL_Deferred.png and b/OloEditor/assets/tests/visual/GroomBinding_GL_Deferred.png differ diff --git a/OloEditor/assets/tests/visual/GroomBinding_GL_Forward.png b/OloEditor/assets/tests/visual/GroomBinding_GL_Forward.png index 937e4408a5..c722e709b6 100644 Binary files a/OloEditor/assets/tests/visual/GroomBinding_GL_Forward.png and b/OloEditor/assets/tests/visual/GroomBinding_GL_Forward.png differ diff --git a/OloEditor/assets/tests/visual/GroomBinding_GL_ForwardPlus.png b/OloEditor/assets/tests/visual/GroomBinding_GL_ForwardPlus.png index 937e4408a5..c722e709b6 100644 Binary files a/OloEditor/assets/tests/visual/GroomBinding_GL_ForwardPlus.png and b/OloEditor/assets/tests/visual/GroomBinding_GL_ForwardPlus.png differ diff --git a/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_Front.png b/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_Front.png index 937e4408a5..c722e709b6 100644 Binary files a/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_Front.png and b/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_Front.png differ diff --git a/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_HumanHair.png b/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_HumanHair.png index 7faaee3486..1a4796a8ac 100644 Binary files a/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_HumanHair.png and b/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_HumanHair.png differ diff --git a/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_LongCoat.png b/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_LongCoat.png index 9f50ab762d..568f34c8d2 100644 Binary files a/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_LongCoat.png and b/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_LongCoat.png differ diff --git a/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_Morph.png b/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_Morph.png index 7327e388a5..71ffe742a2 100644 Binary files a/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_Morph.png and b/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_Morph.png differ diff --git a/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_ShortCoat.png b/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_ShortCoat.png index 937e4408a5..c722e709b6 100644 Binary files a/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_ShortCoat.png and b/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_ShortCoat.png differ diff --git a/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_Side.png b/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_Side.png index e1e7ee87b3..078da6db22 100644 Binary files a/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_Side.png and b/OloEditor/assets/tests/visual/GroomBinding_GL_Forward_Side.png differ diff --git a/OloEditor/src/CMakeLists.txt b/OloEditor/src/CMakeLists.txt index 93d35edfa4..5696b256bc 100644 --- a/OloEditor/src/CMakeLists.txt +++ b/OloEditor/src/CMakeLists.txt @@ -196,31 +196,114 @@ endif() # Heavy-TU compile pool (issue #822): see the matching block in OloEngine/src/CMakeLists.txt for # why the removal must stay conditional on the pool actually being available. +# -------------------------------------------------------------------------- +# OloEditorCore — the editor's headless automation/MCP core (issue #1314). +# +# WHY THIS LIBRARY EXISTS: OloEngine-Tests does not link OloEditor (an executable +# cannot be linked), so it used to RECOMPILE these sources as its own. Measured on a +# build of both targets, that cost 47 duplicated translation units and 231 s — 3.8% of +# the build's entire compile time — and it duplicated the worst TU in the tree: +# McpFieldRegistry.cpp is #1 by peak compiler RSS (8.09 GiB under ASan) and was paid for +# twice. Extracting the shared set to a static library both consumers link removes the +# duplication outright. +# +# THE SEAM IS THE ONE THE TESTS ALREADY DREW, not a new one: every file here was already +# being compiled into the test binary, so the set is known to be self-contained against +# OloEngine. What stays in the executable is the ImGui editor — 34 Panels/*, EditorLayer, +# OloEditorApp, McpServerPanel and the thumbnail cache — none of which any test compiles. +# +# SAFE AS A STATIC LIBRARY because nothing here self-registers. Automation commands are +# published through explicit `Register*Commands(AutomationRegistry&)` entry points that a +# caller invokes, so no object depends on being pulled in for a static initializer's side +# effect — the failure mode that makes naive static-library extraction drop behaviour +# silently. Checked before the move; if a self-registering TU is ever added here it needs +# WHOLE_ARCHIVE or an explicit reference, not a quiet regression. +set(OLO_EDITOR_CORE_SOURCES + "Automation/AutomationAssetCommands.cpp" + "Automation/AutomationAssetIndex.cpp" + "Automation/AutomationBuildCommands.cpp" + "Automation/AutomationCatalogue.cpp" + "Automation/AutomationComponentCommands.cpp" + "Automation/AutomationComponentRegistry.cpp" + "Automation/AutomationEditorCommands.cpp" + "Automation/AutomationEntityCommands.cpp" + "Automation/AutomationEvents.cpp" + "Automation/AutomationFileWrite.cpp" + "Automation/AutomationPrefabCommands.cpp" + "Automation/AutomationRegistry.cpp" + "Automation/AutomationResult.cpp" + "Automation/AutomationSceneCommands.cpp" + "Automation/AutomationSceneDocument.cpp" + "Automation/AutomationSchemaValidation.cpp" + "Automation/AutomationTransaction.cpp" + "Automation/AutomationTransactionCommands.cpp" + "MCP/McpClient.cpp" + "MCP/McpFieldRegistry.cpp" + "MCP/McpScriptApi.cpp" + "MCP/McpScriptTools.cpp" + "MCP/McpServer.cpp" + "MCP/McpTools.cpp" + "MCP/McpToolsAssets.cpp" + "MCP/McpToolsBenchmark.cpp" + "MCP/McpToolsCamera.cpp" + "MCP/McpToolsDiagnostics.cpp" + "MCP/McpToolsEditor.cpp" + "MCP/McpToolsGateway.cpp" + "MCP/McpToolsInput.cpp" + "MCP/McpToolsPerf.cpp" + "MCP/McpToolsPhysics.cpp" + "MCP/McpToolsRender.cpp" + "MCP/McpToolsScene.cpp" + "MCP/McpToolsScripting.cpp" + "MCP/McpToolsShader.cpp" + "MCP/McpToolsTesting.cpp" + "MCP/McpToolsValidation.cpp" + "Panels/ContentBrowser/ContentBrowserItem.cpp" + "Panels/ContentBrowser/DirectoryTree.cpp" + # Arrived on master (#1249) while this library was in flight, listed as ordinary + # editor sources that OloEngine-Tests recompiled. They belong here for the same + # reason as everything above: the test target needs them, and the seam is "what + # both consumers use", not "what the editor happens to ship". + "Automation/AutomationGroomCommands.cpp" + "Groom/GroomBindingAuthoring.cpp" +) +list(REMOVE_ITEM EDITOR_SOURCES ${OLO_EDITOR_CORE_SOURCES}) + +# Same conditional-removal rule as the engine's pool block: pull the heavy TUs out of the +# plain source list ONLY when the pool exists to re-add them via a FILE_SET, or they are +# dropped from the build entirely. if(OLO_HEAVY_COMPILE_POOL_AVAILABLE) - list(REMOVE_ITEM EDITOR_SOURCES "MCP/McpFieldRegistry.cpp" "MCP/McpToolsRender.cpp" "Automation/AutomationComponentRegistry.cpp") + list(REMOVE_ITEM OLO_EDITOR_CORE_SOURCES "MCP/McpFieldRegistry.cpp" "MCP/McpToolsRender.cpp" "Automation/AutomationComponentRegistry.cpp") endif() -add_executable(OloEditor - ${EDITOR_SOURCES} - ${GLSL_SOURCE_FILES} - ${EDITOR_RESOURCES} -) +add_library(OloEditorCore STATIC ${OLO_EDITOR_CORE_SOURCES}) source_group(TREE ${CMAKE_CURRENT_SOURCE_DIR} - FILES ${EDITOR_SOURCES} + FILES ${OLO_EDITOR_CORE_SOURCES} ) if(OLO_HEAVY_COMPILE_POOL_AVAILABLE) - olo_bind_heavy_compile_pool(OloEditor "${CMAKE_CURRENT_SOURCE_DIR}" - # Instantiates typed snapshots for the complete generated component set. - "Automation/AutomationComponentRegistry.cpp" + olo_bind_heavy_compile_pool(OloEditorCore "${CMAKE_CURRENT_SOURCE_DIR}" + # Ranks #1 and #2 of 1761 TUs by peak compiler RSS (8.09 and 3.85 GiB, Linux + # Debug + ASan, 2026-09-17). Both instantiate typed snapshots over the complete + # generated component set. "MCP/McpFieldRegistry.cpp" - # 7,838 MB peak compiler RSS measured cold (issue #822 follow-up), and like - # McpFieldRegistry it is compiled into OloEngine-Tests as well, so the cost is paid twice. + "Automation/AutomationComponentRegistry.cpp" + # 1.50 GiB, #17 — at the >= 1.5 GiB membership threshold (#1307). "MCP/McpToolsRender.cpp" ) endif() +add_executable(OloEditor + ${EDITOR_SOURCES} + ${GLSL_SOURCE_FILES} + ${EDITOR_RESOURCES} +) + +source_group(TREE ${CMAKE_CURRENT_SOURCE_DIR} + FILES ${EDITOR_SOURCES} +) + # Deploy the FFmpeg runtime libs next to the exe (no-op when OLO_VIDEO_FFMPEG=OFF). # Must live in the same directory scope as add_executable — see the note on # olo_copy_ffmpeg_runtime in cmake/CommonProperties.cmake. diff --git a/OloEngine/src/CMakeLists.txt b/OloEngine/src/CMakeLists.txt index 03837896e2..5dcc3d7828 100644 --- a/OloEngine/src/CMakeLists.txt +++ b/OloEngine/src/CMakeLists.txt @@ -2001,16 +2001,19 @@ endif() # drop the file from SOURCES with nothing re-adding it, silently removing it from the build. if(OLO_HEAVY_COMPILE_POOL_AVAILABLE) list(REMOVE_ITEM SOURCES - "OloEngine/Scene/Prefab.cpp" - "OloEngine/Scripting/Lua/LuaScriptGlue_Core.cpp" - "OloEngine/Scripting/Lua/LuaScriptGlue_Environment.cpp" + "OloEngine/Scene/Scene.cpp" + "OloEngine/Scene/SceneSerializer.cpp" + "OloEngine/SaveGame/SaveGameSerializer.cpp" + "OloEngine/Scripting/VisualScript/ComponentFieldRegistry.cpp" + "OloEngine/Scripting/Lua/LuaScriptGlue.cpp" + "OloEngine/Scripting/Lua/LuaScriptGlue_World.cpp" + "OloEngine/Scripting/Lua/LuaScriptGlue_Gameplay.cpp" "OloEngine/Scripting/Lua/LuaScriptGlue_Characters.cpp" "OloEngine/Scripting/Lua/LuaScriptGlue_SceneGraph.cpp" "OloEngine/Scripting/Lua/LuaScriptGlue_Effects.cpp" + "OloEngine/Scripting/Lua/LuaScriptGlue_Environment.cpp" + "OloEngine/Scripting/Lua/LuaScriptGlue_Core.cpp" "OloEngine/Scripting/Lua/LuaScriptGlue_Platform.cpp" - "OloEngine/Scripting/Lua/LuaScriptGlue_World.cpp" - "OloEngine/Scripting/Lua/LuaScriptGlue_Gameplay.cpp" - "OloEngine/Scripting/Lua/LuaScriptGlue_EngineApi.cpp" ) endif() @@ -2103,26 +2106,45 @@ set(OLO_ENGINE_LIBRARY_PARTS OloEngine OloEngineRenderer OloEngineContent PARENT # OloEngineContent. Binding it to OloEngine instead would add the files to a target that no longer # compiles them — a second build edge for an output the other target already owns. # -# THE PARTS, not LuaScriptGlue.cpp: the registrations moved out of it when the glue was split -# (issue #822), and the dispatcher left behind is an ordinary small TU. Measured cold, clang-cl -# Debug, peak compiler RSS: the old single file 12,439 MB; the heaviest part now 5,043 MB -# (_World), the lightest 1,308 MB. Two concurrent from this pool is therefore ~10 GB rather than -# ~25 GB, which is what the 31 GiB CI box could not survive. +# MEMBERSHIP RULE, so this list can be re-derived instead of accumulating (issue #1307): +# every TU whose measured peak compiler RSS is >= 1.5 GiB. Re-derive it by running the weekly +# `build-memory-debug-asan` artifact's ranking (.github/workflows/build-memory.yml) through +# scripts/analyze_proc_stat.py; docs/agent-rules/build-memory-per-tu.md has the recipe. +# +# Measured 2026-09-17 on olo-ci-1, Linux clang 23.1.0, Debug + ASan, 1761 compiles at 99.9% +# coverage. 1.5 GiB is where the tail ends rather than an arbitrary round number: the 17th TU +# is 1.50 GiB and the 18th is 1.37, and p95 across the whole tree is 0.85 GiB. +# +# THE DISPATCHER IS IN THE POOL NOW, and the note it replaces was wrong. When the glue was +# split (#822) LuaScriptGlue.cpp was left out as "an ordinary small TU" — it measures +# 2.40 GiB, 9th of 1761. The per-TU figures that judgement rested on came from a survey whose +# recipe built a whole target while claiming to build one object, so they overstated by up to +# 8.7x; every number in that family is void until re-measured (#1305). +# +# Two concurrent from this pool is 8.09 + 3.85 = 11.94 GiB worst case. The pool's whole job is +# bounding that, and the depth stays 2. +# +# DROPPED from the pool by the same rule: Prefab.cpp (1.37 GiB, #18 — its committed +# "6,400 MB in 22 s" was from the broken recipe and does not reproduce) and +# LuaScriptGlue_EngineApi.cpp (1.18 GiB, #21). Both remain excluded from whole-program +# optimization below; that is a separate concern from compile scheduling. if(OLO_HEAVY_COMPILE_POOL_AVAILABLE) olo_bind_heavy_compile_pool(OloEngineContent "${CMAKE_CURRENT_SOURCE_DIR}" - # 6,400 MB peak compiler RSS in 22 s — 573 lines, so the cost is header and template - # instantiation, not file size. It ranks 17th by compile time and 4th by memory, which - # is exactly why a duration-ranked survey missed it (issue #822 follow-up). - "OloEngine/Scene/Prefab.cpp" - "OloEngine/Scripting/Lua/LuaScriptGlue_Core.cpp" - "OloEngine/Scripting/Lua/LuaScriptGlue_Environment.cpp" + # Every TU in this target measuring >= 1.5 GiB peak compiler RSS, ranked #3-#15 + # tree-wide. See the block comment above for the threshold and how to re-derive it. + "OloEngine/Scene/Scene.cpp" + "OloEngine/Scene/SceneSerializer.cpp" + "OloEngine/SaveGame/SaveGameSerializer.cpp" + "OloEngine/Scripting/VisualScript/ComponentFieldRegistry.cpp" + "OloEngine/Scripting/Lua/LuaScriptGlue.cpp" + "OloEngine/Scripting/Lua/LuaScriptGlue_World.cpp" + "OloEngine/Scripting/Lua/LuaScriptGlue_Gameplay.cpp" "OloEngine/Scripting/Lua/LuaScriptGlue_Characters.cpp" "OloEngine/Scripting/Lua/LuaScriptGlue_SceneGraph.cpp" "OloEngine/Scripting/Lua/LuaScriptGlue_Effects.cpp" + "OloEngine/Scripting/Lua/LuaScriptGlue_Environment.cpp" + "OloEngine/Scripting/Lua/LuaScriptGlue_Core.cpp" "OloEngine/Scripting/Lua/LuaScriptGlue_Platform.cpp" - "OloEngine/Scripting/Lua/LuaScriptGlue_World.cpp" - "OloEngine/Scripting/Lua/LuaScriptGlue_Gameplay.cpp" - "OloEngine/Scripting/Lua/LuaScriptGlue_EngineApi.cpp" ) endif() diff --git a/OloEngine/tests/CMakeLists.txt b/OloEngine/tests/CMakeLists.txt index 8434221fce..e652683d88 100644 --- a/OloEngine/tests/CMakeLists.txt +++ b/OloEngine/tests/CMakeLists.txt @@ -6,32 +6,32 @@ # variable holds the one line so the huge inline source list below needs a single line touched, # not restructured into a separate variable (a collision point for parallel branches on its own). # -# MEASURED, not inferred from compile time (issue #822 follow-up). Peak compiler RSS, cold, -# clang-cl Debug with no sanitizer: +# MEMBERSHIP RULE (issue #1307): every TU measuring >= 1.5 GiB peak compiler RSS. Re-derive +# from the weekly `build-memory-debug-asan` artifact via scripts/analyze_proc_stat.py — see +# docs/agent-rules/build-memory-per-tu.md. # -# ComponentRoundTripTest.cpp 12,389 MB <- the heaviest TU in the tree -# RenderGraphTest.cpp 8,619 MB -# McpToolsRender.cpp 7,838 MB -# McpFieldRegistry.cpp 5,065 MB <- the only one #822 originally caught +# Measured 2026-09-17, Linux clang 23.1.0, Debug + ASan, 1761 compiles at 99.9% coverage: # -# Compile TIME is a poor proxy for this and is how the first three were missed: they rank -# 15th, 18th and 13th by duration. The pool is what bounds how many run at once. +# McpFieldRegistry.cpp 8.09 GiB #1 of 1761, and the tree's heaviest TU +# AutomationComponentRegistry.cpp 3.85 GiB #2 +# ComponentRoundTripTest.cpp 1.63 GiB #16 +# McpToolsRender.cpp 1.50 GiB #17 +# +# THE FIGURES THIS REPLACES WERE WRONG, not stale: the #822 follow-up's survey ran +# `cmake --build --target X -- ''`, which builds the WHOLE TARGET and then credited +# the largest of a dozen concurrent unrelated compiles to whichever TU was named. It reported +# ComponentRoundTripTest.cpp at 12,389 MB and RenderGraphTest.cpp at 8,619 MB; they measure +# 1.63 GiB and 0.94 GiB. RenderGraphTest.cpp is dropped from the pool accordingly (#1305). +# +# Compile TIME remains a poor proxy for memory and is how #822 originally missed these. # oloctl's transport-free core (#1125), listed once in OloCtl/CoreSources.cmake and # compiled by both the `oloctl` executable and this target. Included here rather # than restated, so a new core source cannot reach one of the two and not the other. include(${CMAKE_SOURCE_DIR}/OloCtl/CoreSources.cmake) -set(_olo_mcp_field_registry_src ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpFieldRegistry.cpp) -set(_olo_automation_component_registry_src ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationComponentRegistry.cpp) -set(_olo_mcp_tools_render_src ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpToolsRender.cpp) set(_olo_component_round_trip_src ComponentRoundTripTest.cpp) -set(_olo_render_graph_test_src Rendering/RenderGraphTest.cpp) if(OLO_HEAVY_COMPILE_POOL_AVAILABLE) - set(_olo_mcp_field_registry_src "") - set(_olo_automation_component_registry_src "") - set(_olo_mcp_tools_render_src "") set(_olo_component_round_trip_src "") - set(_olo_render_graph_test_src "") endif() add_executable(OloEngine-Tests @@ -357,7 +357,9 @@ add_executable(OloEngine-Tests Rendering/RenderGraphParallelRecordingTest.cpp Rendering/PODRenderStateChannelMaskTest.cpp #Rendering/CommandDispatchTest.cpp # Requires full engine (calls CommandDispatch::Initialize which pulls in OpenGL statics) - ${_olo_render_graph_test_src} + # Listed directly since #1305 dropped it from the heavy-compile pool (0.94 GiB, #34); + # it no longer needs the variable indirection that held it out of this list. + Rendering/RenderGraphTest.cpp # Phase 2 ratchet for the Vulkan-RHI epic (#691, ADR 0011): counts raw # glXxx() outside Platform/OpenGL/ and fails when it rises above # Rendering/rhi_boundary_baseline.json. Also the only TU that compiles the @@ -930,8 +932,6 @@ add_executable(OloEngine-Tests # Content Browser Tests ContentBrowser/DirectoryTreeTest.cpp ContentBrowser/ContentBrowserItemTest.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/Panels/ContentBrowser/DirectoryTree.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/Panels/ContentBrowser/ContentBrowserItem.cpp # oloctl, the generated CLI frontend over the automation registry (#1125). # Its transport-free core is pulled in by path, the same way the editor MCP # dispatch core above is: the tests then drive the REAL RunCli against a bare @@ -949,58 +949,41 @@ add_executable(OloEngine-Tests # the-seam translation unit into the test binary (cpp_httplib is linked # below). McpServerPanel.* (ImGui) is NOT needed. MCP/McpDispatchTest.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpServer.cpp # The registry McpServer adapts (issue #1123). Three TUs, all # transport-free: the command registry, the result type, and the # input-schema validator that used to sit inside McpServer.cpp. - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationRegistry.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationResult.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationSchemaValidation.cpp # The one command/result serializer every frontend goes through (#1125): # tools/list, tools/search, the gateway and oloctl all call it, so none of # them can drift on what a command looks like or what a result looks like. - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationCatalogue.cpp - ${_olo_automation_component_registry_src} - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationComponentCommands.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationEntityCommands.cpp # Prefab instantiate / unpack / override query / apply / revert / create # (issue #1129). The cross-scene undo entry is the reason this has its own # test: an apply writes into the prefab asset's own Scene, so a fixture # with a real Project + EditorAssetManager is what proves one Ctrl-Z takes # the prefab, the file and every re-synced instance back together. - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationPrefabCommands.cpp - # Groom binding authoring (#1249): olo_groom_bind and the shared bind - # implementation the inspector's Build Binding button also uses. Listed - # here because the test target links McpTools.cpp, which registers it. - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationGroomCommands.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/Groom/GroomBindingAuthoring.cpp + # AutomationPrefabCommands.cpp, AutomationGroomCommands.cpp and + # Groom/GroomBindingAuthoring.cpp used to be listed here as OloEditor sources this + # target recompiled. They come from OloEditorCore now (#1314) — see the + # target_link_libraries below. The groom pair arrived on master while that + # refactor was in flight, so they were moved into OLO_EDITOR_CORE_SOURCES rather + # than re-added here; the reason they are needed is unchanged (this target links + # McpTools.cpp, which registers olo_groom_bind). # Structured build invocation (issue #1163). RegisterBuiltinCommands # composes it, so the test binary needs the TU to link at all; its pure # core is covered by McpAutomationBuildTest below. - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationBuildCommands.cpp # The editor command registry (issue #1131): the declared action table # joined with the live registry, plus pause/step/gizmo/shader-pack. The # actions reach the editor only through EditorMcpContext hooks, so the # test drives fakes and never opens a window. - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationEditorCommands.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationSceneCommands.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationSceneDocument.cpp # Asset query / safe CRUD / import (issue #1128). The index is editor-free # and AssetManager-free by construction, which is what lets the tests drive # it against a temp directory with no project and no registry. - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationAssetIndex.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationAssetCommands.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationFileWrite.cpp # Atomic multi-command transactions (issue #1127). The layer is # transport-free and editor-free at the seam -- it reaches the editor only # through IAutomationHost and CommandHistory -- so the tests drive real # batches against a scene with no server and no GL context. - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationTransaction.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationTransactionCommands.cpp # The publish side of the automation event bus (issue #1131): # AutomationRegistry::RunHandler publishes command completions through # it, so every target that links the registry needs it. - ${CMAKE_SOURCE_DIR}/OloEditor/src/Automation/AutomationEvents.cpp MCP/McpAutomationTransactionTest.cpp MCP/McpAutomationAssetIndexTest.cpp MCP/McpAutomationAssetCommandsTest.cpp @@ -1018,7 +1001,6 @@ add_executable(OloEngine-Tests # ownership members + the McpClientConnection protocol layer live here. # The Win32 pipe transport is compiled but never spawned by unit tests — # they inject in-memory fake transports (McpClientStdioTest.cpp). - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpClient.cpp # Headless-attach stretch (issue #316): McpHeadlessAttachTest hosts the # REAL tool surface (RegisterBuiltinTools) against the offscreen FB, so it # needs the actual tool TUs too. These are editor-decoupled — they include @@ -1027,39 +1009,21 @@ add_executable(OloEngine-Tests # supplies BuildScriptApiDigest, referenced by McpToolsScripting.cpp. # McpTools.cpp is the composition point; the per-domain TUs carry the # actual handlers + registrations (issue #357 split of the monolith). - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpTools.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpToolsAssets.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpToolsBenchmark.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpToolsCamera.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpToolsDiagnostics.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpToolsEditor.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpToolsInput.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpToolsPerf.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpToolsPhysics.cpp - ${_olo_mcp_tools_render_src} - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpToolsScene.cpp # The generated writable-field registry TU (McpGenericFieldWrite.h's # Registry() definition). Compiled once here instead of inline in every # includer — the inline form OOM'd clang-cl Release+ASan on this very # target's McpToolsScene.cpp.obj once the registry crossed ~100 components. # Conditionally blanked above (issue #822) so it can join the heavy-compile # file set instead when the pool is available. - ${_olo_mcp_field_registry_src} - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpToolsScripting.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpToolsShader.cpp # Structured test execution and whole-project validation (issue #1130). # RegisterBuiltinCommands composes both, so the test binary needs the TUs # to link at all; their pure cores are covered by McpTestExecutionTest / # McpProjectValidationTest below. - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpToolsTesting.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpToolsValidation.cpp # The capability-discovery gateway (#1124): olo_capability / olo_tool_search / # olo_tool_describe / olo_tool_execute. Registration-only from the tests' # point of view, like the rest of this list — but its handlers are pure # registry reads with no MarshalRead, so McpExposureProfileTest drives them # for real over the dispatch seam. - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpToolsGateway.cpp - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpScriptApi.cpp # Consented MCP synthetic-input WRITE tool: olo_input_inject (#607) — click / # drag / key / text into the running editor so an agent can verify an # interactive handler actually FIRES. Same dispatch seam (session write gate + @@ -1251,7 +1215,6 @@ add_executable(OloEngine-Tests # from a temp directory of .lua files, schema-enforced dispatch, sandbox # escape containment, the watchdog, and the read-only olo.call_tool bridge. # Pulls the editor-decoupled McpScriptTools.cpp TU (sol2 + engine only). - ${CMAKE_SOURCE_DIR}/OloEditor/src/MCP/McpScriptTools.cpp MCP/McpScriptToolsTest.cpp # The registry-merge trust seam for tools bridged from an OUTBOUND MCP # client connection (#673 Tier 1): ReplaceClientTools validate-and-reject, @@ -1535,21 +1498,18 @@ add_executable(OloEngine-Tests ) if(OLO_HEAVY_COMPILE_POOL_AVAILABLE) - olo_bind_heavy_compile_pool(OloEngine-Tests "${CMAKE_SOURCE_DIR}/OloEditor/src" - "Automation/AutomationComponentRegistry.cpp" - "MCP/McpFieldRegistry.cpp" - "MCP/McpToolsRender.cpp" - ) - # A second call for this target's OWN sources: a file set's FILES must sit under its - # BASE_DIRS, and these live here rather than under OloEditor/src. + # The editor's heavy TUs are bound on OloEditorCore now (#1314) — this target links + # that library instead of recompiling its sources, so there is nothing here to pool. olo_bind_heavy_compile_pool(OloEngine-Tests "${CMAKE_CURRENT_SOURCE_DIR}" "ComponentRoundTripTest.cpp" - "Rendering/RenderGraphTest.cpp" ) endif() target_link_libraries(OloEngine-Tests OloEngine + # The editor's headless automation/MCP core, linked rather than recompiled (#1314). + # It carries its own include directories and httplib, so neither is restated here. + OloEditorCore GTest::gtest httplib::httplib) diff --git a/OloEngine/vendor/CMakeLists.txt b/OloEngine/vendor/CMakeLists.txt index 44fe056263..623a8480c9 100644 --- a/OloEngine/vendor/CMakeLists.txt +++ b/OloEngine/vendor/CMakeLists.txt @@ -263,12 +263,28 @@ FetchContent_Declare( GIT_TAG 2b24bd3bd513f7a6c9c9c566858a48a6124ad5af # master @ 2026-06-05 GIT_SHALLOW FALSE ) +# PATCHED ON TOP OF THE PIN (issue #1306). Upstream's generator rule is four +# `COMMAND echo ...` lines, one of them `COMMAND echo ${GLAD_ARGS} > ${GLAD_ARGS_PATH}`. +# Neither survives being executed without a shell, which is exactly what CMake's +# Instrumentation API does to every custom command -- so `OLO_BUILD_INSTRUMENTATION=ON` +# could not build this project on Windows at all, dying at `glad-generate` with +# "Batch file failed at line 3 with errorcode 1" and no diagnostic. The patch makes the +# rule shell-free; see cmake/glad-patches/ and docs/agent-rules/build-memory-per-tu.md. +# +# The applier restores the tree to the pin before applying, so "tree == pin + patches" +# holds however the tree got to its current state — read the header of +# cmake/apply-vendor-patches.cmake before touching this, especially the note on bumping +# a pin while a patch is carried. FetchContent_Declare( glad GIT_REPOSITORY https://github.com/Dav1dde/glad.git GIT_TAG 73db193f853e2ee079bf3ca8a64aa2eaf6459043 # v2.0.8 GIT_SHALLOW FALSE SOURCE_SUBDIR cmake + PATCH_COMMAND "${CMAKE_COMMAND}" + "-DOLO_PATCH_DIR=${CMAKE_SOURCE_DIR}/cmake/glad-patches" + "-DOLO_PATCH_LABEL=glad" + -P "${CMAKE_SOURCE_DIR}/cmake/apply-vendor-patches.cmake" ) # pl_mpeg: single-file, public-domain MPEG-1 video + MP2 audio decoder. diff --git a/cmake/CompilerCache.cmake b/cmake/CompilerCache.cmake index 7d2a369328..8ceec8c726 100644 --- a/cmake/CompilerCache.cmake +++ b/cmake/CompilerCache.cmake @@ -237,19 +237,68 @@ if(OLO_ENABLE_COMPILER_CACHE) unset(_olo_dep_lang) endif() - # Disable precompiled headers while caching. sccache/ccache CANNOT cache a - # compile that consumes a PCH (it reports the TU non-cacheable for /Fp /Yu), - # so with PCH on the whole engine + editor + runtime + tests — the bulk of - # the build and the part that changes per push — recompiles every run and - # the cache only covers vendor libs. PCH and the cache optimize opposite - # cases: PCH speeds a *cold* compile; the cache makes a *warm* compile a - # near-instant hit. For CI's common case (incremental pushes = warm cache) - # caching the engine is the far bigger win; the occasional cold run (first - # build / toolchain bump / cache eviction) pays a re-parse penalty. - # Every engine TU #includes OloEnginePCH.h explicitly, so dropping the - # precompile (and its force-include) is safe. OLO_ENABLE_PCH is an option() - # in the top-level CMakeLists; override it here. - set(OLO_ENABLE_PCH OFF CACHE BOOL "Disabled: MSVC PCH (/Fp) is non-cacheable under sccache/ccache" FORCE) + # Disable precompiled headers while caching — but ONLY where a PCH is actually + # uncacheable, which is not everywhere (issue #1314). + # + # The rule this replaces was unconditional, and its own reason string named the + # constraint it was enforcing: "MSVC PCH (/Fp) is non-cacheable under + # sccache/ccache". That is true for the MSVC frontend — cl.exe and clang-cl both + # spell a PCH /Yc /Yu /Fp, and the cache reports those TUs non-cacheable — and it + # is NOT true for a GNU-frontend clang, where ccache caches PCH-using compiles + # given the right sloppiness. This repo already established that: + # docs/agent-rules/compiler-cache-uncacheable-compiles.md says "clang needs nothing + # beyond what CMake emits", and .github/actions/setup-linux-build ALREADY exports + # CCACHE_SLOPPINESS=pch_defines,time_macros,... on both self-hosted setups with the + # comment "every engine target uses one". The CI action was configured for a PCH + # the CMake then turned off. + # + # So the disable now tracks the reason instead of outliving it. Three cases: + # + # MSVC frontend (cl.exe, clang-cl) -> OFF. /Fp is genuinely non-cacheable. + # GNU clang/GCC WITHOUT the required ccache sloppiness -> OFF, and say why: + # every engine compile would go uncached, which is far worse than a cold + # re-parse. This is the case a Linux developer hits without the CI env. + # GNU clang/GCC WITH it -> leave OLO_ENABLE_PCH alone (defaults ON). + # + # The second half of the original argument is general rather than MSVC-specific + # and is NOT dismissed here: "PCH speeds a cold compile; the cache makes a warm + # compile a near-instant hit". The reason to expect them to compose on clang is + # that the PCH's constituent headers are inputs to the cache key either way, so + # enabling it should not add invalidation. That is a prediction, not a result — + # #1305's build-memory workflow is the instrument, and #1314 tracks pricing it. + # If a warm-cache regression shows up, this block is the one conditional to flip. + set(_olo_pch_uncacheable TRUE) + set(_olo_pch_reason "the MSVC frontend spells a PCH /Fp /Yu, which sccache/ccache report as non-cacheable") + if(NOT CMAKE_CXX_COMPILER_FRONTEND_VARIANT STREQUAL "MSVC") + # ccache needs pch_defines AND time_macros before it will cache a PCH-using + # compile (ccache manual, "Precompiled headers"). Checked rather than assumed: + # turning PCH on without them makes EVERY engine TU uncacheable, which is the + # failure docs/agent-rules/compiler-cache-uncacheable-compiles.md measured at + # 1417/1417 objects. + set(_olo_pch_sloppiness "$ENV{CCACHE_SLOPPINESS}") + if(_olo_pch_sloppiness MATCHES "pch_defines" AND _olo_pch_sloppiness MATCHES "time_macros") + set(_olo_pch_uncacheable FALSE) + else() + set(_olo_pch_reason + "CCACHE_SLOPPINESS does not contain both pch_defines and time_macros, so every PCH-using compile would be uncacheable (got '${_olo_pch_sloppiness}')") + endif() + endif() + + if(_olo_pch_uncacheable) + # Every engine TU #includes OloEnginePCH.h explicitly, so dropping the + # precompile (and its force-include) is safe. OLO_ENABLE_PCH is an option() + # in the top-level CMakeLists; override it here. + set(OLO_ENABLE_PCH OFF CACHE BOOL "Disabled: ${_olo_pch_reason}" FORCE) + message(STATUS "Precompiled headers: OFF while caching — ${_olo_pch_reason}.") + else() + message(STATUS + "Precompiled headers: left enabled alongside the compiler cache — a GNU-frontend " + "clang PCH is cacheable and CCACHE_SLOPPINESS carries pch_defines,time_macros " + "(issue #1314).") + endif() + unset(_olo_pch_uncacheable) + unset(_olo_pch_reason) + unset(_olo_pch_sloppiness) # Disable unity (jumbo) builds while caching — same reasoning as PCH above, opposite # trade-off. Unity batches 16 TUs into one object; editing a single line in any file diff --git a/cmake/LinkSemaphore.cmake b/cmake/LinkSemaphore.cmake index f55ea46fa3..258b680b0d 100644 --- a/cmake/LinkSemaphore.cmake +++ b/cmake/LinkSemaphore.cmake @@ -14,10 +14,28 @@ # compiler count was only two. N concurrent trees get up to N x OLO_LINK_JOBS # linkers, and no per-tree pool can see the others. # -# This wires every link step through .claude/skills/run-oloengine/link-semaphore.ps1, -# which takes a permit from an OS-named semaphore shared by every tree on the machine. -# The wrapper FAILS OPEN by design (see its header): if the semaphore is unavailable -# the link runs unthrottled rather than failing. +# This wires every link step through a wrapper that takes one of N permits shared by +# every tree on the machine. There are two, picked by platform, because the ownership +# property that makes a permit reclaimable when its holder is killed has to be obtained +# differently on each: +# +# Windows .claude/skills/run-oloengine/link-semaphore.ps1 — N named MUTEXES, because +# a Windows semaphore's count is owned by nobody and a killed holder leaks +# its permit forever (that script's header has the measurement). +# POSIX scripts/link-semaphore.py — flock(), where the kernel gives ownership for +# free: the lock belongs to the open file description and is released when +# the process dies, however it dies. +# +# THE POSIX HALF IS NEW (issue #1313) AND IT CLOSED A REAL HOLE. Before it, a non-Windows +# host found no pwsh, warned, and linked unthrottled — so on the olo-ci box NOTHING +# bounded a measured 11.17 GiB link (Debug+ASan; 10.03 GiB with no sanitizer) against a +# 14 GiB unit cap and a 19 GiB slice shared by both runner slots. The Ninja pools do not +# cover it there: every Linux job configures with no -G and gets Unix Makefiles, where +# `olo_link` and `olo_heavy` silently do not exist. +# +# Both wrappers FAIL OPEN by design: if the permit mechanism is unavailable, or every +# permit is busy past the timeout, the link runs unthrottled rather than failing. A +# throttle that can fail a build is worse than no throttle. # # GENERATOR SUPPORT, and why it shapes the whole policy: CMAKE__LINKER_LAUNCHER # is honoured ONLY by the Ninja and Makefile generators — the same restriction as @@ -56,27 +74,64 @@ if(OLO_ENABLE_LINK_SEMAPHORE) "the throttle off deliberately, rather than passing a value that disables it " "by accident.") else() - find_program(OLO_PWSH NAMES pwsh powershell) - set(_olo_link_wrapper "${CMAKE_SOURCE_DIR}/.claude/skills/run-oloengine/link-semaphore.ps1") + # PICK THE WRAPPER BY PLATFORM. Both implement the same contract — take one of + # OLO_LINK_SEMAPHORE_SLOTS permits, run the link, fail OPEN on any problem — but + # they cannot share an implementation: the Windows one needs N named mutexes to + # get ownership semantics a Windows semaphore does not have, while POSIX gets + # ownership from the kernel for free because an flock belongs to the open file + # description and dies with the process holding it. + # + # THE POSIX HALF IS WHY THIS BLOCK EXISTS AT ALL (issue #1313). Until it landed, + # a non-Windows host found no pwsh, warned, and linked unthrottled — which on the + # olo-ci box meant NOTHING bounded a measured 11.17 GiB link against a 14 GiB unit + # cap, because OLO_LINK_JOBS and olo_heavy are Ninja pools and every Linux job + # gets Unix Makefiles. + if(WIN32) + find_program(OLO_LINK_SEMAPHORE_RUNNER NAMES pwsh powershell) + set(_olo_link_wrapper "${CMAKE_SOURCE_DIR}/.claude/skills/run-oloengine/link-semaphore.ps1") + set(_olo_link_runner_args "-NoProfile;-File") + set(_olo_link_runner_kind "pwsh") + else() + find_program(OLO_LINK_SEMAPHORE_RUNNER NAMES python3 python) + set(_olo_link_wrapper "${CMAKE_SOURCE_DIR}/scripts/link-semaphore.py") + set(_olo_link_runner_args "") + set(_olo_link_runner_kind "python3") + endif() - if(NOT OLO_PWSH) + if(NOT OLO_LINK_SEMAPHORE_RUNNER) message(WARNING - "Link semaphore: neither 'pwsh' nor 'powershell' found on PATH — linking " - "WITHOUT cross-tree throttling. Concurrent builds in other worktrees could " - "then overlap their link steps.") + "Link semaphore: no '${_olo_link_runner_kind}' on PATH — linking WITHOUT " + "cross-tree throttling. Concurrent builds, or concurrent links inside one " + "Makefiles build, could then overlap. See issue #1313 for what that costs.") elseif(NOT EXISTS "${_olo_link_wrapper}") message(WARNING "Link semaphore: wrapper not found at '${_olo_link_wrapper}' — linking " "WITHOUT cross-tree throttling.") else() - # A launcher is a semicolon-separated LIST: each element becomes one argv - # entry, so the wrapper path is never re-parsed as a string and a path with - # spaces cannot split. The real linker and its arguments are appended by - # CMake after these. - set(CMAKE_C_LINKER_LAUNCHER "${OLO_PWSH};-NoProfile;-File;${_olo_link_wrapper}") - set(CMAKE_CXX_LINKER_LAUNCHER "${OLO_PWSH};-NoProfile;-File;${_olo_link_wrapper}") + # THE PERMIT COUNT TRAVELS IN THE ENVIRONMENT, and `cmake -E env` is what puts + # it there. Both wrappers read OLO_LINK_SEMAPHORE_SLOTS from the environment + # rather than argv, because a linker command line is full of tokens an argument + # parser would try to interpret (`-o`, `--start-group`, a bare `--`) — so every + # token after the wrapper path has to be forwarded verbatim as the command. + # + # Without this the cache variable was decorative: it changed the STATUS message + # below and nothing else, so `-DOLO_LINK_SEMAPHORE_SLOTS=4` silently kept + # throttling at the wrapper's built-in default of 2. Setting it here makes the + # documented knob the authority. + # + # A launcher is a semicolon-separated LIST: each element becomes one argv entry, + # so a path with spaces cannot split. The real linker and its arguments are + # appended by CMake after these. + set(_olo_link_launcher + "${CMAKE_COMMAND};-E;env;OLO_LINK_SEMAPHORE_SLOTS=${OLO_LINK_SEMAPHORE_SLOTS};${OLO_LINK_SEMAPHORE_RUNNER}") + if(_olo_link_runner_args) + set(_olo_link_launcher "${CMAKE_COMMAND};-E;env;OLO_LINK_SEMAPHORE_SLOTS=${OLO_LINK_SEMAPHORE_SLOTS};${OLO_LINK_SEMAPHORE_RUNNER};${_olo_link_runner_args}") + endif() + set(CMAKE_C_LINKER_LAUNCHER "${_olo_link_launcher};${_olo_link_wrapper}") + set(CMAKE_CXX_LINKER_LAUNCHER "${_olo_link_launcher};${_olo_link_wrapper}") message(STATUS - "Link semaphore: ON, ${OLO_LINK_SEMAPHORE_SLOTS} permit(s) shared across every build tree") + "Link semaphore: ON, ${OLO_LINK_SEMAPHORE_SLOTS} permit(s) shared across every build tree " + "(${_olo_link_runner_kind})") endif() endif() endif() diff --git a/cmake/ProcStatReport.cmake b/cmake/ProcStatReport.cmake new file mode 100644 index 0000000000..e6877b1005 --- /dev/null +++ b/cmake/ProcStatReport.cmake @@ -0,0 +1,280 @@ +# ProcStatReport.cmake +# Per-invocation PEAK RSS for every compile and every link — issue #1305. +# +# WHAT THIS ADDS THAT #759/#822 COULD NOT +# --------------------------------------- +# The CMake Instrumentation API wired in the root CMakeLists.txt records per-command +# TIME plus `args.dynamicSystemInformation.afterHostMemoryUsed`, which is SYSTEM-WIDE +# host memory in KiB sampled when the command finished — not the command's own RSS. +# So the existing traces cannot attribute memory to a translation unit at all, and the +# "handful of heavy TUs" #759 named were identified by compile TIME, a proxy. Every +# number that rests on per-TU memory — the Linux `--parallel 2` pins, OLO_LINK_JOBS, +# OLO_HEAVY_COMPILE_JOBS, and the box's 14 GiB unit / 19 GiB slice cgroup caps — was +# therefore derived from an unmeasured figure, and five sources disagreed by 3x. +# +# clang's `-fproc-stat-report=` closes exactly that gap: the driver appends one +# CSV record per SUBPROCESS it executes, carrying wall time, user time and that +# process's PEAK RSS in KiB, at essentially zero overhead. +# +# "clang++.exe","CMakeFiles/OloEngine.dir/src/.../Foo.cpp.o",203125,187500,132552 +# ^tool ^output file ^wall ^user ^peak KiB +# (microseconds) +# +# It covers LINKING wherever the COMPILER drives the link, which is why no linker launcher +# is involved (the issue's step 2 suggested CMAKE_CXX_LINKER_LAUNCHER): `clang++ ... -o app` +# runs `ld.lld` as a driver subprocess, so the same flag on the LINK command line reports +# the linker's own peak RSS — verified with a standalone clang-cl link, which emitted +# `"lld-link","a.obj",0,0,25812` alongside the compile record. That keeps +# LinkSemaphore.cmake's launcher chain completely untouched. +# +# CMake's own link rule only drives it that way for a GNU-frontend clang, though; under +# clang-cl it runs lld-link directly and the flag must NOT be added to the link line at +# all. See the per-language loop below — that difference is measured, and it +# is why the Linux builds (the ones whose caps are in question) get the link half and the +# local Windows trees do not. +# +# WHY THE PATH IS RELATIVE, AND WHY THAT IS NOT A STYLE CHOICE +# ------------------------------------------------------------ +# It is what keeps the compiler cache warm, and it was MEASURED, not assumed (the issue +# asked for exactly this check under a different lever). The flag's value is part of the +# command line, so ccache hashes it. ccache's `base_dir` does NOT rewrite it — base_dir +# relativises arguments ccache recognises as paths to existing files, and this one names +# a file that does not exist yet at hash time. So an ABSOLUTE path makes the hash +# tree-specific and silently destroys cross-tree sharing. +# +# Measured on ccache 4.13.6 / clang-cl 23.1.0, two source-identical trees under one +# base_dir, second tree expected to hit: +# +# no flag at all (baseline) 1 hit / 2 50% <- what sharing looks like +# -fproc-stat-report= 0 hits / 2 0% <- sharing GONE +# -fproc-stat-report= 1 hit / 2 50% <- identical to baseline +# +# A relative path is resolved by clang against the BUILD TOOL'S working directory, so each +# build tree still gets its own records file while every tree puts the SAME text on the +# command line. Do not "tidy this up" into ${CMAKE_BINARY_DIR}/...; that is the 0% row. +# +# THAT WORKING DIRECTORY IS NOT THE SAME FOR EVERY GENERATOR, and the difference decides +# how the records are read back. Confirmed by generating both and reading the emitted +# compile rule, not by assumption: +# +# Ninja every command runs from the top build dir +# -> one /olo-proc-stat.csv +# Unix Makefiles the rule is literally `cd / && clang++ ... -c ...` +# -> one //olo-proc-stat.csv PER SUBDIRECTORY +# +# Every Linux job in this repo configures with no -G and so gets Unix Makefiles (see +# docs/agent-rules/build-trees-and-windows-asan.md §5e). So ALWAYS hand the analyser the +# build TREE, not one file — scripts/analyze_proc_stat.py walks it and gathers every +# records file. Reading only the top-level file on a Linux build would rank a fraction of +# the TUs, and the fraction it omits is the engine and test subdirectories: the heavy ones. +# +# The other half of the same measurement, and the reason the report is honest about +# coverage rather than silent: on a cache HIT ccache does not run the compiler, so NO +# record is appended. A warm build therefore yields a PARTIAL ranking. That is correct +# behaviour — a hit costs no memory — but it means the ranking must come from a cold +# build, and scripts/analyze_proc_stat.py prints the recorded-vs-expected count so a +# partial run can never be mistaken for a complete one. Under ccache's preprocessor mode +# a miss also emits a SECOND record for the `-E` pass (output under ccache's tmp dir, +# ~30 MiB); the analyser classifies and counts those separately instead of dropping them. +# +# The records file is APPENDED to, never truncated, and the records carry no timestamp. +# The analyser therefore reports the MAXIMUM peak RSS seen per output, and CI deletes the +# file before building so a published ranking covers one build. + +# TWO SWITCHES, AND THE SECOND ONE IS STILL WORTH HAVING. +# +# OLO_BUILD_INSTRUMENTATION is the umbrella the issue asked this to ride: turning it on +# turns this on. OLO_PROC_STAT_REPORT is the per-TU-memory half ON ITS OWN: +# +# cmake --preset dev-cached -DOLO_PROC_STAT_REPORT=ON +# +# It is a compiler flag — no launcher, no CMake version floor, no generator restriction — +# so it has none of the Instrumentation API's constraints, and it is the cheaper switch +# when memory is the only question. The API additionally wraps every compile, link and +# custom command in `ctest --instrument`, which costs a process per build step. +# +# HISTORICAL NOTE, because the split was originally forced rather than chosen: the +# umbrella could not build this project on Windows AT ALL. cmake_instrumentation() runs +# each custom command through a launcher that EXECUTES argv instead of handing it to a +# shell, and vendored glad's generator rule was four `COMMAND echo ...` lines, one of them +# `COMMAND echo ${GLAD_ARGS} > ${GLAD_ARGS_PATH}`. Under cmd.exe `echo` is a shell builtin +# with no executable and `>` is redirection, so the build died at `glad-generate` with +# "Batch file failed at line 3 with errorcode 1" and no diagnostic. That is FIXED (#1306): +# cmake/glad-patches/ makes the rule shell-free on top of the pin. Both switches now work +# on Windows, and this one remains because it is the narrower tool, not the fallback. +# NOT option() / set(... CACHE ...): that would make the umbrella's value STICKY. The +# first configure with OLO_BUILD_INSTRUMENTATION=ON would write OLO_PROC_STAT_REPORT=ON +# into the cache, and every later configure — including one that turns the umbrella back +# OFF — would keep reading the stale ON and quietly go on instrumenting. That is the +# "set forever after the first configure" trap spelled out in cmake/CompilerCache.cmake's +# OLO_COMPILER_CACHE_TOOL comment, and this is the same shape. +# +# `if(NOT DEFINED ...)` is what makes both paths work: an explicit +# -DOLO_PROC_STAT_REPORT=ON creates a real cache entry, so it is DEFINED and wins; with +# no explicit value the variable is re-derived from the umbrella on EVERY configure and +# can never go stale. +if(NOT DEFINED OLO_PROC_STAT_REPORT) + set(OLO_PROC_STAT_REPORT ${OLO_BUILD_INSTRUMENTATION}) +endif() + +if(NOT OLO_PROC_STAT_REPORT) + return() +endif() + +# Clang only, and stated rather than left to be discovered. MSVC has no equivalent flag, +# and the `msvc` preset is not the host whose caps are in question — the self-hosted Linux +# builds are clang, and they are. GCC is not a toolchain this repo builds with. +# +# CMAKE_CXX_COMPILER_FRONTEND_VARIANT splits the two clang spellings: a GNU-frontend +# clang/clang++ takes the flag directly, while clang-cl parses MSVC-style options and +# needs it handed to the clang driver through the `/clang:` pass-through. Getting that +# wrong is not a build failure — clang-cl would treat a bare `-fproc-stat-report=` as an +# input filename and the records file would simply never appear. +# CHECKED PER LANGUAGE, because the flag is applied per language below. This project +# compiles C as well as C++ (vendored glad, lua, bc7enc, ...), and CMAKE_C_COMPILER and +# CMAKE_CXX_COMPILER are separate cache entries that a toolchain file or a `-D` can point +# at different compilers. Validating only the C++ id and then adding the flag to C +# compilations too would hand an unsupported option to a non-clang C compiler and fail the +# build — so each language is gated on its own id, and a mixed toolchain simply gets the +# flag on whichever half is clang. +if(NOT CMAKE_CXX_COMPILER_ID MATCHES "^(Clang|AppleClang)$" + AND NOT CMAKE_C_COMPILER_ID MATCHES "^(Clang|AppleClang)$") + message(WARNING + "Per-invocation peak-RSS reporting was requested but it needs clang: neither the C " + "compiler ('${CMAKE_C_COMPILER_ID}') nor the C++ compiler " + "('${CMAKE_CXX_COMPILER_ID}') has an equivalent of -fproc-stat-report (issue #1305). " + "Timing and host-memory instrumentation (if the generator supports it) is unaffected; " + "only the per-TU memory ranking is unavailable. Configure a clang preset — dev-cached, " + "clangcl, or a Linux preset — to get it.") + return() +endif() + +set(OLO_PROC_STAT_FILE "olo-proc-stat.csv" CACHE STRING + "Records file for per-invocation peak RSS (issue #1305). MUST stay RELATIVE — an absolute path is hashed by the compiler cache and drops cross-tree sharing to zero; see the measurement in cmake/ProcStatReport.cmake.") + +# Reject a value that would quietly cost the cache, rather than accepting it and letting +# a future cold-cache CI job look like a cache-configuration problem. Same "fail rather +# than silently disable" rule OLO_LINK_JOBS and OLO_HEAVY_COMPILE_JOBS follow. +if(OLO_PROC_STAT_FILE STREQUAL "") + message(FATAL_ERROR + "OLO_PROC_STAT_FILE is empty — clang would write the records to a file named '' and the " + "measurement would silently produce nothing. Unset it to take the default, or name a file.") +endif() +if(IS_ABSOLUTE "${OLO_PROC_STAT_FILE}") + message(FATAL_ERROR + "OLO_PROC_STAT_FILE must be a RELATIVE path (got '${OLO_PROC_STAT_FILE}'). An absolute " + "path goes into the compiler cache's hash and cannot be relativised by ccache's base_dir, " + "which drops cross-tree cache sharing from the measured 50% baseline to 0%. It is resolved " + "against the build tool's working directory (CMAKE_BINARY_DIR) either way, so a relative " + "path already gives this tree its own file.") +endif() +# A BASENAME, not just any relative path. clang would happily honour +# `reports/rss.csv` (if the directory exists), but the analyser discovers records by +# walking the build tree and comparing directory entries, which are basenames — so a +# value with a directory component writes records the reader can never find, and the +# only symptom is an empty ranking. Rejected here rather than half-supported. +# string(FIND) rather than a regex, because the regex form is a trap here and the obvious +# repair does not work. A CMake string literal consumes one level of backslash escaping and +# the regex engine consumes another, so "[/\]" has a VALUE of [/] and silently matches only +# the forward slash — a Windows separator sailed straight through. Measured: both the +# one-backslash and the two-backslash spellings ALLOW `sub ss.csv`; it takes FOUR in the +# source to mean one literal backslash to the matcher. Two plain substring searches say what +# they mean and cannot be "simplified" into a broken state. +string(FIND "${OLO_PROC_STAT_FILE}" "/" _olo_psr_fwd) +string(FIND "${OLO_PROC_STAT_FILE}" "\\" _olo_psr_bwd) +if(NOT _olo_psr_fwd EQUAL -1 OR NOT _olo_psr_bwd EQUAL -1) + message(FATAL_ERROR + "OLO_PROC_STAT_FILE must be a bare FILENAME with no directory component (got " + "'${OLO_PROC_STAT_FILE}'). The build tool's working directory already differs per " + "generator — Ninja runs from the top build dir, Unix Makefiles from each target's " + "subdirectory — so the records land in several places by design, and " + "scripts/analyze_proc_stat.py finds them by matching this basename while walking the " + "build tree. A path with a directory component would be written but never found.") +endif() +unset(_olo_psr_fwd) +unset(_olo_psr_bwd) + +# BUILT PER LANGUAGE. The two clang spellings differ — a GNU-frontend clang/clang++ takes +# the flag directly, while clang-cl parses MSVC-style options and needs it handed to the +# clang driver through the `/clang:` pass-through — and C and C++ can be different +# compilers with different frontends. Getting the spelling wrong is not a build failure: +# clang-cl would treat a bare `-fproc-stat-report=` as an input filename and the records +# file would simply never appear. +# +# LINKS ARE NOT MEASURABLE UNDER clang-cl, and that is a property of CMake's link rule +# rather than of the flag. With a GNU-frontend clang, CMAKE__LINK_EXECUTABLE runs the +# COMPILER as the link driver, which spawns ld.lld as a subprocess — so the flag on the link +# line reports the linker's own peak RSS, which is exactly what issue #1305's step 2 wanted. +# Under clang-cl, CMake invokes `lld-link` DIRECTLY; there is no clang driver in the link +# step to take the option and no subprocess to report on, and `/clang:` is a COMPILER +# option, so lld-link reads it as an input filename and the link fails outright: +# +# lld-link: error: could not open '/clang:-fproc-stat-report=olo-proc-stat.csv' +# +# Said out loud rather than silently skipped, because "no link records on Windows" +# otherwise looks like a measurement that found links to be free. +set(_olo_proc_stat_compile_langs "") +set(_olo_proc_stat_link_langs "") +foreach(_olo_psr_lang C CXX) + if(NOT CMAKE_${_olo_psr_lang}_COMPILER_ID MATCHES "^(Clang|AppleClang)$") + continue() + endif() + if(CMAKE_${_olo_psr_lang}_COMPILER_FRONTEND_VARIANT STREQUAL "MSVC") + set(_olo_psr_flag "/clang:-fproc-stat-report=${OLO_PROC_STAT_FILE}") + set(_olo_psr_links FALSE) + else() + set(_olo_psr_flag "-fproc-stat-report=${OLO_PROC_STAT_FILE}") + set(_olo_psr_links TRUE) + endif() + + # Scoped with COMPILE_LANGUAGE so a non-clang sibling language never sees the option. + add_compile_options("$<$:${_olo_psr_flag}>") + list(APPEND _olo_proc_stat_compile_langs ${_olo_psr_lang}) + + if(_olo_psr_links) + # LINK_LANGUAGE, not COMPILE_LANGUAGE: a target links with one language's driver, + # and that is the process whose subprocess (ld.lld) we want reported. Static + # ARCHIVING is not covered either way — it goes through CMAKE__ARCHIVE_* + # rules, which take neither a launcher nor the compiler's own flags. Acceptable, + # and the same scope note LinkSemaphore.cmake makes: the measured spike is the + # linker, not the archiver. + add_link_options("$<$:${_olo_psr_flag}>") + list(APPEND _olo_proc_stat_link_langs ${_olo_psr_lang}) + endif() +endforeach() +unset(_olo_psr_lang) +unset(_olo_psr_flag) +unset(_olo_psr_links) + +# Reported so a build log states which languages are instrumented and whether links are +# covered, rather than leaving "no link records" to be misread as "links are free". +if(NOT _olo_proc_stat_compile_langs) + message(WARNING + "Per-invocation peak-RSS reporting is ON but no language was instrumented — no clang " + "compiler was found for C or C++ after the guard above. This is a bug in " + "cmake/ProcStatReport.cmake, not a configuration you can fix.") + return() +endif() + +list(JOIN _olo_proc_stat_compile_langs "/" _olo_psr_compile_note) +if(_olo_proc_stat_link_langs) + list(JOIN _olo_proc_stat_link_langs "/" _olo_psr_link_note) + set(_olo_psr_note "${_olo_psr_compile_note} compiles and ${_olo_psr_link_note} links") +else() + set(_olo_psr_note + "${_olo_psr_compile_note} compiles ONLY - links are not measurable under clang-cl, where CMake invokes lld-link directly instead of through the clang driver (see cmake/ProcStatReport.cmake). Use a Linux clang build for link peak RSS") +endif() + +# --name is part of the command because OLO_PROC_STAT_FILE is configurable: without it a +# custom basename would be written by clang and then not found by the analyser, whose +# default is the stock name. +message(STATUS + "Per-invocation peak RSS: ON for ${_olo_psr_note}. Appending to " + "'${OLO_PROC_STAT_FILE}' in each build directory the build tool runs from. Rank with: " + "python scripts/analyze_proc_stat.py ${CMAKE_BINARY_DIR} --name ${OLO_PROC_STAT_FILE}") + +unset(_olo_psr_compile_note) +unset(_olo_psr_link_note) +unset(_olo_psr_note) +unset(_olo_proc_stat_compile_langs) +unset(_olo_proc_stat_link_langs) diff --git a/cmake/SplitDwarf.cmake b/cmake/SplitDwarf.cmake new file mode 100644 index 0000000000..f9263f1faa --- /dev/null +++ b/cmake/SplitDwarf.cmake @@ -0,0 +1,121 @@ +# SplitDwarf.cmake +# Split debug info out of the objects so the LINKER stops loading it — issue #1313. +# +# WHY THIS IS A CANDIDATE AT ALL. #1305 measured the build's memory ceiling and it is +# not a compile: one `ld.lld` link of OloEngine-Tests peaks at 11.17 GiB under +# Debug + ASan and 10.03 GiB with no sanitizer, against a heaviest compile of 8.09 / +# 4.65 GiB. The evidence that the link is DEBUG-INFO bound rather than instrumentation +# bound is the ratio between those two cells: ASan adds +74% to the heaviest compile but +# only +11% to the link, so whatever dominates those 10-11 GiB is present without any +# sanitizer. `-gsplit-dwarf` puts `.debug_info` in a side-car `.dwo` the linker never +# reads, which is the most direct attack on that number available. +# +# DEFAULT OFF, AND THAT IS A MEASURED POSITION, NOT CAUTION. +# +# This repo maps `DW_AT_comp_dir` to `/olo` via `-ffile-prefix-map` whenever the compiler +# cache is on — which is every CI job — so that objects are path-independent and the two +# olo-ci runner slots can share one ccache (see cmake/CompilerCache.cmake). Split DWARF +# resolves its side-car through exactly that field: the skeleton records +# `DW_AT_comp_dir=/olo` plus a bare `DW_AT_dwo_name`, so a symbolizer looks for +# `/olo/.dwo`, which does not exist. +# +# Measured with llvm-symbolizer, clang 23.1.0, cross-compiling to x86_64-linux, one +# control per variable: +# +# -gsplit-dwarf alone, .dwo present inline frames RESOLVE +# -ffile-prefix-map=/olo alone inline frames RESOLVE +# both together, .dwo present inline frames LOST +# +# So it is the COMBINATION that breaks, not either half. What survives in every arm is +# the function name (from `.symtab`) and file:line (from `.debug_line`, which stays in the +# object) — so an ASan report keeps its frames and their locations, and loses the inlined +# ones. Packaging the side-cars into a `.dwp` did not restore it in that probe either, but +# that probe symbolized a `.o` rather than an executable, where the lookup differs — treat +# the `.dwp` route as UNPROVEN rather than ruled out. +# +# Mitigating, and the reason this is a small loss rather than a blocker: every sanitizer +# job is `CMAKE_BUILD_TYPE=Debug`, where there is very little inlining to lose. +# +# `--gdb-index` IS DELIBERATELY NOT PAIRED WITH THIS, though the issue's table pairs them. +# That pairing is the standard *debugging* recipe: it builds a `.gdb_index` section so GDB +# starts quickly. For a *memory* goal it works against us — lld constructs that index by +# reading the debug info it would otherwise skip, which is the very work this option exists +# to avoid. If it is ever wanted, price it in the same measurement cell rather than +# assuming it is free. +# +# WHAT IT BUYS, measured on olo-ci, both cells from ONE run so they are comparable +# (1783 compiles each, cache off, PCH and unity off, Debug + ASan): +# +# baseline split DWARF delta +# heaviest ld.lld link 11.23 GiB 8.75 GiB -2.47 GiB (-22.0%) +# heaviest compile 8.18 GiB 8.18 GiB +0.00 GiB +# median / p95 compile 0.32/0.86 0.32/0.86 unchanged +# total compile wall 2.06 h 1.99 h -4.2 min +# +# So the saving is real and it lands exactly where the mechanism says it should: 22% off +# the link, nothing measurable on any compile. Against the runner unit's 14 GiB cap that +# moves the headroom over one link from 2.77 GiB to 5.25 GiB, and link + heaviest compile +# from 19.41 GiB to 16.93 GiB — under the 19 GiB slice cap rather than at it. +# +# IT STAYS OFF ANYWAY, and the reason is the trade above, not the size of the saving: +# 2.47 GiB of headroom against inlined frames in every sanitizer stack. Since #1313 the +# link is bounded by a real semaphore, so it runs alone and 11.23 GiB already fits the cap +# — the saving is margin, not a fix for a live failure, and margin is worth less than a +# readable stack. Flip it if the margin is ever what is short. +# +# THE VERSION WORTH HAVING costs nothing: make the `.dwo` resolvable and the symbolization +# loss goes away with it. The cause is `DW_AT_comp_dir=/olo`, so `-fdebug-compilation-dir` +# pointing somewhere real, or a `.dwp` beside the executable, would plausibly give the 22% +# AND keep the frames. Neither is verified — the `.dwp` probe in this file's measurement +# used a `.o`, where the lookup differs. +# +# Re-derive any of this from `.github/workflows/build-memory.yml`'s +# `Debug + ASan + split DWARF` cell, which runs on the schedule and on demand. + +option(OLO_SPLIT_DWARF + "Emit debug info into side-car .dwo files so the linker never loads it (issue #1313). OFF by default: it costs inlined frames in sanitizer stacks while this repo maps DW_AT_comp_dir to /olo — see the measurement in cmake/SplitDwarf.cmake." + OFF) + +if(NOT OLO_SPLIT_DWARF) + return() +endif() + +# DWARF only. The MSVC frontend (cl.exe and clang-cl alike) emits CodeView, where this +# flag has no meaning at all — a request for it there is a configuration mistake worth +# naming rather than a no-op to discover later. +if(CMAKE_CXX_COMPILER_FRONTEND_VARIANT STREQUAL "MSVC" + OR NOT CMAKE_CXX_COMPILER_ID MATCHES "^(Clang|AppleClang|GNU)$") + message(WARNING + "OLO_SPLIT_DWARF is ON but this toolchain does not produce DWARF " + "('${CMAKE_CXX_COMPILER_ID}', frontend '${CMAKE_CXX_COMPILER_FRONTEND_VARIANT}') — " + "ignoring it. Split debug info is a DWARF feature; the MSVC frontend emits CodeView " + "and already keeps debug info out of the objects via a separate .pdb.") + return() +endif() + +# Per language, and only for the languages whose compiler actually takes the flag — the +# same rule cmake/ProcStatReport.cmake follows, and for the same reason: C and C++ are +# separate cache entries that a toolchain file can point at different compilers, so +# validating one and flagging both hands an unsupported option to the other. +set(_olo_split_dwarf_langs "") +foreach(_olo_sd_lang C CXX) + if(CMAKE_${_olo_sd_lang}_COMPILER_ID MATCHES "^(Clang|AppleClang|GNU)$" + AND NOT CMAKE_${_olo_sd_lang}_COMPILER_FRONTEND_VARIANT STREQUAL "MSVC") + add_compile_options("$<$:-gsplit-dwarf>") + list(APPEND _olo_split_dwarf_langs ${_olo_sd_lang}) + endif() +endforeach() +unset(_olo_sd_lang) + +if(NOT _olo_split_dwarf_langs) + message(WARNING "OLO_SPLIT_DWARF is ON but no C or C++ compiler accepts -gsplit-dwarf.") + return() +endif() + +list(JOIN _olo_split_dwarf_langs "/" _olo_sd_note) +message(STATUS + "Split DWARF: ON for ${_olo_sd_note} — debug info goes to .dwo side-cars the linker " + "does not read. Inlined frames will NOT symbolize while -ffile-prefix-map is also in " + "effect (issue #1313); function names and file:line still will.") +unset(_olo_sd_note) +unset(_olo_split_dwarf_langs) diff --git a/cmake/fsr2-apply-patches.cmake b/cmake/apply-vendor-patches.cmake similarity index 65% rename from cmake/fsr2-apply-patches.cmake rename to cmake/apply-vendor-patches.cmake index 62cdcf3c94..a334378ee2 100644 --- a/cmake/fsr2-apply-patches.cmake +++ b/cmake/apply-vendor-patches.cmake @@ -1,10 +1,15 @@ # ============================================================================= -# FetchContent PATCH_COMMAND for the FSR2 OpenGL tree. +# FetchContent PATCH_COMMAND for a fetched vendor tree. +# +# Shared by every dependency this repo patches (FSR2's OpenGL tree, glad). Each +# caller passes OLO_PATCH_DIR and a human-readable OLO_PATCH_LABEL; the contract +# and every failure mode below are identical regardless of which tree it is, which +# is why there is one script rather than one per dependency. # # Run in script mode (`cmake -P`) with the source tree as the working directory, # which is what FetchContent's patch step gives us. # -# THE CONTRACT IS "TREE == PIN + cmake/fsr2-patches/*.patch", and it is enforced +# THE CONTRACT IS "TREE == PIN + ${OLO_PATCH_DIR}/*.patch", and it is enforced # by RESTORING the tracked files to the pin before applying anything. Three # things fall out of that, all of which an apply-only script gets wrong: # @@ -13,10 +18,10 @@ # plain `git apply` would fail the second time and take the configure with it. # * DELETING a patch file actually un-patches the tree. Without the restore, # removing a patch whose fix has landed upstream leaves the tree carrying -# shader edits that exist in no tracked file in this repo -- the worst +# edits that exist in no tracked file in this repo -- the worst # possible state, because nothing reports it. # * A hand edit to the fetched tree is reverted rather than colliding. Debug in -# the patch file; edits under OloEngine/vendor/clang/fsr2gl-src do not +# the patch file; edits inside the fetched source tree do not # survive a configure. # # The restore only covers TRACKED files, so a patch that ADDS a file would leave @@ -30,8 +35,11 @@ # whose failure modes are listed above. # ============================================================================= -if(NOT DEFINED OLO_FSR2_PATCH_DIR) - message(FATAL_ERROR "OLO_FSR2_PATCH_DIR must be set") +if(NOT DEFINED OLO_PATCH_DIR) + message(FATAL_ERROR "OLO_PATCH_DIR must be set") +endif() +if(NOT DEFINED OLO_PATCH_LABEL) + message(FATAL_ERROR "OLO_PATCH_LABEL must be set — it names the tree in every message below") endif() find_package(Git QUIET REQUIRED) @@ -45,15 +53,15 @@ execute_process( if(NOT _restore EQUAL 0) message(FATAL_ERROR - "FSR2: could not restore the fetched tree to its pin\n" + "${OLO_PATCH_LABEL}: could not restore the fetched tree to its pin\n" "Tree: ${CMAKE_CURRENT_SOURCE_DIR}\n" "${_restore_err}") endif() -file(GLOB OLO_FSR2_PATCHES "${OLO_FSR2_PATCH_DIR}/*.patch") -list(SORT OLO_FSR2_PATCHES) +file(GLOB OLO_PATCHES "${OLO_PATCH_DIR}/*.patch") +list(SORT OLO_PATCHES) -foreach(_patch IN LISTS OLO_FSR2_PATCHES) +foreach(_patch IN LISTS OLO_PATCHES) get_filename_component(_name "${_patch}" NAME) execute_process( @@ -64,17 +72,17 @@ foreach(_patch IN LISTS OLO_FSR2_PATCHES) if(NOT _result EQUAL 0) message(FATAL_ERROR - "FSR2 patch failed to apply: ${_name}\n" + "${OLO_PATCH_LABEL} patch failed to apply: ${_name}\n" "Tree: ${CMAKE_CURRENT_SOURCE_DIR}\n" "${_stderr}\n" - "If the pin in cmake/fsr2.cmake just moved, the fix is probably upstream now — " + "If the pin just moved, the fix is probably upstream now — " "delete the patch file, or rebase it onto the new pin.") endif() - message(STATUS "FSR2 patch applied: ${_name}") + message(STATUS "${OLO_PATCH_LABEL} patch applied: ${_name}") endforeach() -list(LENGTH OLO_FSR2_PATCHES _count) +list(LENGTH OLO_PATCHES _count) if(_count EQUAL 0) - message(STATUS "FSR2: no local patches; the fetched tree is the pin as-is") + message(STATUS "${OLO_PATCH_LABEL}: no local patches; the fetched tree is the pin as-is") endif() diff --git a/cmake/fsr2.cmake b/cmake/fsr2.cmake index cb76097cfd..2fe75ea872 100644 --- a/cmake/fsr2.cmake +++ b/cmake/fsr2.cmake @@ -52,7 +52,7 @@ include(FetchContent) # ----------------------------------------------------------------------------- # Local patches on top of the pin. # -# The pinned tree is patched in place by cmake/fsr2-apply-patches.cmake, which +# The pinned tree is patched in place by cmake/apply-vendor-patches.cmake, which # restores the tree to the pin before applying so that "tree == pin + patches" # holds however the tree got to its current state. Each patch file names the # upstream PR it carries and says when to drop it. @@ -111,8 +111,9 @@ FetchContent_Declare(fsr2gl GIT_SUBMODULES "" SOURCE_SUBDIR olo-does-not-build-upstream-cmake PATCH_COMMAND "${CMAKE_COMMAND}" - "-DOLO_FSR2_PATCH_DIR=${OLO_FSR2_PATCH_DIR}" - -P "${CMAKE_CURRENT_LIST_DIR}/fsr2-apply-patches.cmake") + "-DOLO_PATCH_DIR=${OLO_FSR2_PATCH_DIR}" + "-DOLO_PATCH_LABEL=FSR2" + -P "${CMAKE_CURRENT_LIST_DIR}/apply-vendor-patches.cmake") FetchContent_MakeAvailable(fsr2gl) diff --git a/cmake/glad-patches/0001-run-the-generator-without-a-shell.patch b/cmake/glad-patches/0001-run-the-generator-without-a-shell.patch new file mode 100644 index 0000000000..7c21df99e5 --- /dev/null +++ b/cmake/glad-patches/0001-run-the-generator-without-a-shell.patch @@ -0,0 +1,44 @@ +diff --git a/cmake/GladConfig.cmake b/cmake/GladConfig.cmake +index c16b75c..31ce7df 100644 +--- a/cmake/GladConfig.cmake ++++ b/cmake/GladConfig.cmake +@@ -238,16 +238,35 @@ function(glad_add_library TARGET) + string(REPLACE "${GLAD_DIR}" GLAD_DIRECTORY GLAD_ARGS_UNIVERSAL "${GLAD_ARGS}") + set(GLAD_ARGS_PATH "${GLAD_DIR}/args.txt") + ++ # OLO PATCH (issue #1306): every COMMAND below must run WITHOUT a shell. ++ # ++ # CMake's Instrumentation API -- cmake_instrumentation(), which this project turns on ++ # with OLO_BUILD_INSTRUMENTATION -- wraps every custom command in ++ # `ctest --instrument -- ` and EXECUTES argv directly rather than handing it to ++ # a shell. Under cmd.exe `echo` is a shell builtin with no executable on disk, and `>` ++ # is shell redirection, so the original four lines could not run: the build died at ++ # `glad-generate` with "Batch file failed at line 3 with errorcode 1" and no ++ # diagnostic of any kind. POSIX shells hid it because /usr/bin/echo is a real binary. ++ # ++ # `cmake -E echo` is a real process, so it survives the launcher. `cmake -E` has no ++ # redirect, so the args record is written at configure time and copied into place -- ++ # it is only ever written, never read back, but it is an OUTPUT of this rule and so ++ # must exist or the rule is considered dirty on every build. ++ string(REPLACE ";" " " GLAD_ARGS_TEXT "${GLAD_ARGS}") ++ set(GLAD_ARGS_RECORD "${CMAKE_CURRENT_BINARY_DIR}/glad-args-${TARGET}.txt") ++ file(WRITE "${GLAD_ARGS_RECORD}" "${GLAD_ARGS_TEXT} ++") ++ + # add make custom target + add_custom_command( + OUTPUT ${GLAD_FILES} ${GLAD_ARGS_PATH} +- COMMAND echo Cleaning ${GLAD_DIR} ++ COMMAND ${CMAKE_COMMAND} -E echo Cleaning ${GLAD_DIR} + COMMAND ${CMAKE_COMMAND} -E remove_directory ${GLAD_DIR} + COMMAND ${CMAKE_COMMAND} -E make_directory ${GLAD_DIR} +- COMMAND echo Generating with args ${GLAD_ARGS} ++ COMMAND ${CMAKE_COMMAND} -E echo Generating with args ${GLAD_ARGS} + COMMAND ${Python_EXECUTABLE} -m glad ${GLAD_ARGS} +- COMMAND echo Writing ${GLAD_ARGS_PATH} +- COMMAND echo ${GLAD_ARGS} > ${GLAD_ARGS_PATH} ++ COMMAND ${CMAKE_COMMAND} -E echo Writing ${GLAD_ARGS_PATH} ++ COMMAND ${CMAKE_COMMAND} -E copy ${GLAD_ARGS_RECORD} ${GLAD_ARGS_PATH} + WORKING_DIRECTORY $<$:${GLAD_SOURCES_DIR}> + COMMENT "${TARGET}-generate" + USES_TERMINAL diff --git a/docs/agent-rules/README.md b/docs/agent-rules/README.md index 843cd59a63..7914f9d0c5 100644 --- a/docs/agent-rules/README.md +++ b/docs/agent-rules/README.md @@ -44,6 +44,7 @@ Each entry is one sentence stating the rule. The story that taught it is inside ## Build and dependencies - [build-trees-and-windows-asan.md](build-trees-and-windows-asan.md): never build msvc and clangcl trees together; caches, link bounds, memory, the local ASan recipe. +- [build-memory-per-tu.md](build-memory-per-tu.md): set `--parallel` and every memory cap from the published per-TU peak-RSS ranking, not from a remembered number. - [concurrent-cmake-configure.md](concurrent-cmake-configure.md): one configure at a time per build tree; the error blames your CMakeLists.txt and LTO instead. - [static-archive-4gib-ceiling.md](static-archive-4gib-ceiling.md): a .lib cannot exceed 4 GiB, and `LNK1248` under-reports the overshoot. - [vcpkg-dependency-management.md](vcpkg-dependency-management.md): read before adding, bumping or removing a dependency; the CRT triplet mismatch is heap corruption. @@ -305,6 +306,7 @@ The same fact written in more than one place, with nothing enforcing agreement. | [runtime-scene-switching.md](runtime-scene-switching.md) | The build pipeline and the runtime must agree on an asset layout. | | [audio-voice-budget.md](audio-voice-budget.md) | One config field costs four edits, one of them silent. | | [build-trees-and-windows-asan.md](build-trees-and-windows-asan.md) | Two build trees writing the same generated files. | +| [build-memory-per-tu.md](build-memory-per-tu.md) | A build-memory number nobody re-measures: five sources disagreed by 3x while `--parallel` and two cgroup caps rested on it. An absolute records path silently zeroes the compiler cache's cross-tree hit rate; a relative one yields one file per subdirectory under Makefiles, and reading only the top-level file ranks the wrong fraction of the build. | | [build-trees-and-windows-asan.md §4b](build-trees-and-windows-asan.md#4b-live-toolchain-bug-a-throw-from-inside-a-catch-handler-avs-clang-cl--asan) | A throw executed inside a `catch` handler AVs under clang-cl ASan; the catch type and rethrow form are irrelevant. | | [thread-local-lifetime-at-exit.md](thread-local-lifetime-at-exit.md) | A static destructor reading a `thread_local` that `__dyn_tls_dtor` already destroyed; 219 failures, one bug. | | [concurrent-cmake-configure.md](concurrent-cmake-configure.md) | Two configures sharing one `CMakeFiles/`; the nested `try_compile` that loses reports the error. | diff --git a/docs/agent-rules/build-memory-per-tu.md b/docs/agent-rules/build-memory-per-tu.md new file mode 100644 index 0000000000..6239ed1c5c --- /dev/null +++ b/docs/agent-rules/build-memory-per-tu.md @@ -0,0 +1,256 @@ +# Per-TU build memory: measuring it, and the four traps + +**Set `--parallel`, `OLO_HEAVY_COMPILE_JOBS`, `OLO_LINK_JOBS` and any cgroup cap from the +published ranking, not from a remembered number.** Configure with clang and +`-DOLO_PROC_STAT_REPORT=ON` (or `-DOLO_BUILD_INSTRUMENTATION=ON`, the umbrella, on Linux), +build, then hand `scripts/analyze_proc_stat.py` the **build tree**: + +```powershell +# OLO_PROC_STAT_REPORT is the memory half alone; the umbrella adds timing and tracing. +cmake --preset dev-cached -DOLO_PROC_STAT_REPORT=ON +pwsh -NoProfile -File .claude/skills/run-oloengine/build-lock.ps1 -Command ` + 'cmake --build build-cached --target OloEditor --target OloEngine-Tests --config Debug --parallel 6' +python scripts/analyze_proc_stat.py build-cached --cap-gib 14 +``` + +`OLO_PROC_STAT_REPORT` is the per-TU-memory half on its own — a compiler flag, no launcher, +no CMake floor, no generator restriction — and the umbrella turns it on along with the +Instrumentation API's timing and tracing. Use the narrow switch when memory is the only +question; it costs nothing per build step, where the API spends a process on every one. + +> **Fixed, and worth knowing if you read an older note:** `OLO_BUILD_INSTRUMENTATION=ON` +> used to be unable to build this project on **Windows** at all. The API runs every custom +> command through a launcher that *executes* argv rather than handing it to a shell, and +> vendored glad's generator rule needed a shell for both `echo` and `>`; the build died at +> `glad-generate` with `Batch file failed at line 3 with errorcode 1` and no diagnostic. +> `cmake/glad-patches/` makes that rule shell-free on top of the pin (#1306). Both switches +> work on Windows now. + +CI publishes the same report weekly as the `build-memory-*` artifact +(`.github/workflows/build-memory.yml`), for `Debug` and `Debug + ASan`. That artifact +exists because the alternative already failed: #759 measured this once by hand in 2026-08, +the number was never re-measured, and by 2026-09 five sources disagreed about it by 3x +while `--parallel 2`, a 14 GiB unit cap and a 19 GiB slice cap all rested on it (#1305). + +The mechanism is clang's `-fproc-stat-report=`: the driver appends one CSV record per +subprocess it runs, carrying wall time, user time and **that process's peak RSS in KiB**. + + "clang++","CMakeFiles/OloEngine.dir/src/.../Foo.cpp.o",203125,187500,132552 + tool output file wall user peak KiB + +## Why not the instrumentation API from #759/#822 + +It cannot answer this question, and it looks like it can. Its per-command memory field is +`args.dynamicSystemInformation.afterHostMemoryUsed` — **system-wide** host memory in KiB, +sampled when that command finished. It cannot attribute memory to a TU at all. Everything +before #1305 therefore ranked "heavy TUs" by compile **time**, a proxy. Use the API for +time and the flag for memory; they are complementary, and `OLO_BUILD_INSTRUMENTATION` +turns on both — `OLO_PROC_STAT_REPORT` is the memory half alone, for the Windows case +above. + +## Trap 1 — the records path must stay RELATIVE, or every other job loses its cache + +The flag's value is part of the command line, so the compiler cache hashes it. ccache's +`base_dir` does **not** rewrite it — base_dir relativises arguments ccache recognises as +paths to *existing* files, and this one names a file that does not exist yet at hash time. +An absolute path therefore makes the hash tree-specific and silently ends cross-tree +sharing, including between the two `olo-ci` slots, which share one `CCACHE_DIR` under +different `GITHUB_WORKSPACE` paths. + +Measured, ccache 4.13.6 / clang-cl 23.1.0, two source-identical trees under one `base_dir`, +the second expected to hit: + +| command line | hit rate | +|---|---| +| no flag at all (baseline) | 1/2 — **50%** | +| `-fproc-stat-report=` | 0/2 — **0%** | +| `-fproc-stat-report=` | 1/2 — **50%**, identical to baseline | + +`cmake/ProcStatReport.cmake` rejects an absolute `OLO_PROC_STAT_FILE` with a +`FATAL_ERROR`, and `build-memory.yml` greps the generated build system for one. Both +guards are there because the failure is invisible: the cache stays *alive*, just never +warm. + +## Trap 2 — there is one records file per SUBDIRECTORY on Linux + +A relative path is resolved against the **build tool's** working directory, and that is +not the same directory for every generator. Confirmed by generating both and reading the +emitted compile rule: + +| generator | rule | result | +|---|---|---| +| Ninja | runs every command from the top build dir | one `build/olo-proc-stat.csv` | +| Unix Makefiles | `cd / && clang++ … -c …` | **one file per subdirectory** | + +Every Linux job in this repo configures with no `-G`, so it gets Unix Makefiles +([build-trees-and-windows-asan.md §5e](build-trees-and-windows-asan.md#5e-both-memory-pools-are-ninja-only--so-on-linux-ci---parallel-n-is-the-only-cap-issue-796)). +**Always pass the build tree, never a single file** — the analyser walks it and gathers +every records file, re-rooting the outputs so two targets cannot collide. A reader that +opened only the top-level file on a Linux build would rank a fraction of the TUs, and the +fraction it omits is the engine and test subdirectories: the heavy ones. A Ninja-only +check does not catch this. + +## Trap 3 — a warm compiler cache makes a partial ranking look like a cheap build + +On a cache **hit** ccache does not run the compiler, so it appends no record. That is +correct — the record measures compiler RSS and no compiler ran — but it is **not** the same +as "a hit is free": ccache still does the lookup and decompresses the result, and that costs +memory this instrument cannot see. Either way a warm build ranks only whatever missed. So: CI uses +`CCACHE_DISABLE=1`; locally prefer **`CCACHE_RECACHE=1`**, which also forces every TU +through the compiler but *populates* the shared cache instead of discarding the work. The +analyser always prints coverage (records vs. objects in the tree — not +`compile_commands.json`, which counts the whole project while CI builds one target) and +`--fail-under-coverage` fails rather than publishing a lower bound as a census. + +Under ccache's preprocessor mode a **miss** emits a second record for the `-E` pass (~30 MiB, +in ccache's tmp dir). Those are classified `intermediate` and counted separately, not +dropped silently. + +**Measuring the hit RATE needs `CCACHE_STATSLOG`, never before/after `ccache -s`:** every +worktree here shares one `CCACHE_DIR`, so the global counters move with the siblings' builds +(a 28-step build of mine showed a 2378-call delta). The statslog writes one line per +invocation but is **build-scoped, not self-resetting**: it appends, so create or truncate a +unique file per build or you will read the previous build's results back. + +## Trap 4 — compare only against a build with the same PCH and unity settings + +`cmake/CompilerCache.cmake` **forces** `OLO_ENABLE_PCH=OFF` and +`OLO_ENABLE_UNITY_BUILD=OFF` whenever the cache is on, which is what every cached CI job +builds with. Turning the cache off to get a clean measurement therefore silently +re-enables PCH (default `ON`) — and a unity build would merge 16 TUs into one object and +destroy the per-TU ranking outright. Pass `-DOLO_ENABLE_PCH=OFF +-DOLO_ENABLE_UNITY_BUILD=OFF` explicitly on any cache-off measurement run, as +`build-memory.yml` does, or the numbers are not comparable to CI's. + +## The measurement (2026-09-17): the LINK is the ceiling, not any compile + +Linux, `/opt/llvm-23.1.0/bin/clang++`, `olo-ci-1`, cache off, PCH and unity off, target +`OloEngine-Tests`, **1761 compiles at 99.9% coverage** in each cell. This is the +configuration the caps were derived from, so these are the numbers to use. + +| | plain `Debug` | `Debug + ASan` | +|---|---:|---:| +| median compile | 0.31 GiB | 0.32 GiB | +| p95 compile | 0.84 GiB | 0.85 GiB | +| compiles over 2 GiB | 10 | 12 | +| **heaviest compile** | 4.65 GiB | **8.09 GiB** | +| **heaviest link (`ld.lld`)** | **10.03 GiB** | **11.17 GiB** | +| link ÷ heaviest compile | **2.16x** | **1.38x** | + +**One `ld.lld` link of `OloEngine-Tests` uses 11.17 GiB — 80% of the runner unit's 14 GiB +`memory.max`, and more than any translation unit in the build.** The belief it replaces was +8.7 GB; it is 38% higher than that, and it is 10.03 GiB even with no sanitizer. The heaviest +compile, meanwhile, is 8.09 GiB — the folklore "~9.0 GB for one `clang++` TU" was close, and +the "one TU hit 13.9 GB" claim does **not** reproduce. + +This does not contradict #759, it completes it. #759 found `OLO_LINK_JOBS=2` is not the +**wall-clock** serialisation point — still true, links are off the critical path. For +**memory** the link is the single largest consumer in both configurations. + +**Nothing used to bound it on Linux, and that is now fixed (#1313).** `OLO_LINK_JOBS` and +`olo_heavy` are Ninja job pools, and every Linux CI job gets Unix Makefiles +([§5e](build-trees-and-windows-asan.md#5e-both-memory-pools-are-ninja-only--so-on-linux-ci---parallel-n-is-the-only-cap-issue-796)), +where neither exists. `cmake/LinkSemaphore.cmake` did not cover it either: it shelled out +to `pwsh`, which the box does not have, so it warned and linked unthrottled. `make -j2` was +therefore free to run that 11.17 GiB link beside a compile — **19.26 GiB** with the heaviest +one, past the 14 GiB unit cap and at the 19 GiB slice cap — and two concurrent links across +the two runner slots is 22.3 GiB against that same slice. It had not been failing constantly +only because the tests link lands at the end of the build, which is scheduling luck rather +than a guarantee. + +`scripts/link-semaphore.py` is the POSIX half of that throttle: N permits via `flock`, where +the kernel releases the lock when the holder dies, so a killed linker cannot leak a permit. +It **fails open** — no permit mechanism, or every permit busy past the timeout, and the link +runs anyway, because a throttle that can fail a build is worse than no throttle. The permit +count now genuinely comes from `OLO_LINK_SEMAPHORE_SLOTS`: it is passed through `cmake -E +env`, where previously the cache variable only changed a status message and the wrapper's +built-in default of 2 was the real value on both platforms. + +### What to do with `--parallel`, stated as a recommendation not a change + +- **Sanitizer jobs: keep `--parallel 2`.** Three lanes is a 15.24 GiB worst case, past the + cap. The pin is correct and now has evidence. +- **Non-sanitizer Linux Debug: 4 lanes fits on compile grounds** (the derivation's largest + fitting N), but the gain is bounded by the same link, so raise it only if the queue + actually needs it. +- **The lever worth having is a link bound that works under Makefiles.** A 14 GiB unit cap + leaves 2.8 GiB of headroom over a single link; that, not `--parallel`, is what is tight. + +None of these were applied here — changing a live runner's configuration is out of #1305's +scope and is the maintainer's call. + +### The `olo_heavy` pool is mispopulated in both directions + +Confirmed independently on Linux+ASan and on a Windows clang-cl census — same names, same +direction, so it is not an artefact of either host. On the Linux+ASan ranking it holds +`AbilityComponentRoundTripTest.cpp` (#281), `RenderGraphTest.cpp` (#34), +`LuaScriptGlue_EngineApi.cpp` (#21), `Prefab.cpp` (#18) and `McpToolsRender.cpp` (#17), +while `Scene.cpp` (**#3**), `ComponentFieldRegistry.cpp` (**#5**), `SceneSerializer.cpp` +(#7), `LuaScriptGlue.cpp` (#9) and `SaveGameSerializer.cpp` (#11) sit outside it. + +`LuaScriptGlue.cpp` was deliberately excluded when the glue was split, on the recorded +grounds that "the dispatcher left behind is an ordinary small TU" +(`OloEngine/src/CMakeLists.txt`). It ranks **9th**. What the pool gets right: +`McpFieldRegistry.cpp` is #1 in every cell and the `LuaScriptGlue_*` parts cluster in the +top 15 — #822's split worked. Re-populating it is **issue #1307**; the pool is Ninja-only, +so this costs nothing on Linux CI and everything locally. + +## Scope: what this cannot see + +- **MSVC.** `-fproc-stat-report` is clang-only and there is no equivalent; the `msvc` + preset is not the host whose caps are in question. The option must still *configure and + build cleanly* under it, which it does — `cmake/ProcStatReport.cmake` warns and returns. +- **vcpkg ports and FetchContent dependencies.** They build outside this project's + `add_compile_options()` reach — the same blind spot #759's trace had. +- **Static archiving.** `CMAKE__ARCHIVE_*` rules take neither a launcher nor the + compiler's own flags. Same scope note `cmake/LinkSemaphore.cmake` makes: the measured + spike is the linker, not the archiver. +- **Links, but only on Windows.** With a GNU-frontend clang, `CMAKE_CXX_LINK_EXECUTABLE` + runs the *compiler* as the link driver, which spawns `ld.lld` as a subprocess — so the + flag on the link line reports the linker's own peak RSS, no linker launcher needed, and + Linux gets the link half of #1305 for free. Under **clang-cl** CMake invokes `lld-link` + **directly**: there is no clang driver in the link step to pass the option to, and + `/clang:…` is a compiler option, so lld-link takes it as an input file and the link + fails outright with `could not open '/clang:-fproc-stat-report=…'`. The module therefore + adds it to compiles only there, and *says* so at configure time — "no link records on + Windows" would otherwise read as a measurement that found links to be free. For link + peak RSS, use a Linux clang build, which is the host whose caps are in question anyway. + +## The cheap config levers, priced + +The three levers #1305 listed, checked rather than assumed: + +| lever | verdict | +|---|---| +| ThinLTO instead of full LTO | **Cannot help the sanitizer jobs.** `cmake/CommonProperties.cmake` sets `INTERPROCEDURAL_OPTIMIZATION_RELEASE`/`_DIST` only, and every sanitizer job is `CMAKE_BUILD_TYPE=Debug`. No LTO is enabled there to convert. | +| `-gsplit-dwarf` (**not** `--gdb-index`) | **Priced: −2.47 GiB on the link, −22%.** Both cells from one run on olo-ci, Debug + ASan, 1783 compiles each: heaviest `ld.lld` link **11.23 → 8.75 GiB**, heaviest compile **8.18 → 8.18** (unchanged), median/p95 compile unchanged, total compile wall −4.2 min. Headroom over one link against the 14 GiB unit cap goes 2.77 → 5.25 GiB. ccache caches and restores the `.dwo` correctly. **The cost:** inlined frames stop symbolizing — measured with a control per variable, `-gsplit-dwarf` alone resolves, `-ffile-prefix-map=/olo` alone resolves, together they are **lost**; function names and file:line survive. `OLO_SPLIT_DWARF` is therefore **OFF by default**: since #1313 the link is bounded and 11.23 GiB already fits, so the saving is margin rather than a fix, and margin is worth less than a readable stack. The version worth having is free — make the `.dwo` resolvable (`-fdebug-compilation-dir`, or a `.dwp` beside the executable) and you keep both; neither is verified yet. `--gdb-index` is deliberately not paired: lld builds that index by reading the debug info this flag exists to skip. | +| `-g1` on sanitizer CI only | Keeps line tables and function names — what ASan needs to symbolise — and drops variable/type DWARF. Price it on the TUs the ranking names, not on the tree average: the peak is set by a handful. | + +### Appendix: the committed per-TU figures were wrong when written, and three methods now agree + +`OloEngine/src/CMakeLists.txt`, `OloEditor/src/CMakeLists.txt` and issue #1113 carry per-TU +figures up to **8.7x too high** — a recipe bug, not decay. That survey ran +`cmake --build --target X -- ''`, which passes *both* the target and the object to +ninja and so builds the **whole target**, then credited the largest of a dozen concurrent +unrelated compiles to whichever TU was named. An isolated re-measurement on 2026-09-08 +(per-PID `PeakWorkingSet64`, one `clang-cl` asserted) corrected it; this census is a third, +independent method and agrees with that correction within **1.11-1.23x** on all five +spot-checked TUs. Three of them, as committed → isolated → census: `Scene.cpp` +2,498 → 2,499 → 2,785 MB; `ComponentRoundTripTest.cpp` 12,389 → 1,415 → 1,599 MB; +`Prefab.cpp` 6,400 → 1,282 → 1,488 MB. The other two were `McpToolsRender.cpp` +(7,838 → 1,369 → 1,680 MB) and `RenderGraphTest.cpp` (8,619 → 949 → 1,053 MB). + +`Scene.cpp` is the control — the one row that reproduced exactly under the buggy recipe — +and it lands at the same 1.11x, so the residual is the parallel-build environment rather +than a systematic error in either method. **Census for ranking, the isolated recipe for one +TU's absolute figure.** Treat every number in those comments as void until re-measured. + +## History + +- **#759** (2026-08) — wired the instrumentation API; found compilation, not linking, sets + the build's memory ceiling, and that a handful of TUs set it rather than the `-j` width. + Identified that handful by compile time. +- **#822** — native CMake 4.3/4.4 instrumentation and the `olo_heavy` per-file-set compile + job pool, populated from the same time-based ranking. +- **#1305** — this file. Added per-invocation peak RSS for compiles and links, and the + weekly artifact that keeps it from decaying again. diff --git a/docs/agent-rules/build-trees-and-windows-asan.md b/docs/agent-rules/build-trees-and-windows-asan.md index 38ef28c604..303ab5f770 100644 --- a/docs/agent-rules/build-trees-and-windows-asan.md +++ b/docs/agent-rules/build-trees-and-windows-asan.md @@ -533,6 +533,22 @@ per this section's rule in one pass regardless, because the flag masks and the r ## 5. Instrumenting a build, and a per-file-set compile job pool (issues #759, #822) +**For per-TU MEMORY, read [build-memory-per-tu.md](build-memory-per-tu.md) instead of this +section.** Everything below measures per-command *time* plus a system-wide host-memory +sample, and §5c/§5d's "heavy TUs" were identified by compile time, which is a proxy. The +field this API cannot record — each compile's and each link's own peak RSS — is what +`-fproc-stat-report` and `scripts/analyze_proc_stat.py` add (#1305). +`OLO_BUILD_INSTRUMENTATION` turns both on **by default**, and the per-TU memory half is +**also available on its own** as `OLO_PROC_STAT_REPORT` — a plain compiler flag, so it needs +no launcher and costs nothing per build step. The precedence is worth knowing: an explicitly +supplied `OLO_PROC_STAT_REPORT` always wins, so `-DOLO_BUILD_INSTRUMENTATION=ON +-DOLO_PROC_STAT_REPORT=OFF` gives you the timing and tracing with **no** RSS reporting, and +`-DOLO_PROC_STAT_REPORT=ON` alone gives RSS with no launcher. Only when it is left unset does +it follow the umbrella — and it follows it on *every* configure rather than latching, so +turning the umbrella back off turns this off with it. (The umbrella used to be unusable on Windows because its +launcher could not run vendored glad's shell-dependent generator rule; fixed in #1306 by +patching that rule on top of the pin.) + ### 5a. Use the native recipe (CMake 4.3+), not the manual gate below As of #822 this machine is on CMake 4.4.2. The repo's own `cmake_minimum_required` floor diff --git a/scripts/analyze_proc_stat.py b/scripts/analyze_proc_stat.py new file mode 100644 index 0000000000..29e68861f0 --- /dev/null +++ b/scripts/analyze_proc_stat.py @@ -0,0 +1,711 @@ +#!/usr/bin/env python3 +"""Rank a build's compiles and links by PEAK RSS, and derive a safe -j width from it. + +Reads the CSV records clang's ``-fproc-stat-report=`` appends, which +``cmake/ProcStatReport.cmake`` wires up behind ``OLO_BUILD_INSTRUMENTATION`` +(issue #1305). Each record is one driver SUBPROCESS: + + "clang++","CMakeFiles/OloEngine.dir/src/.../Foo.cpp.o",203125,187500,132552 + tool output file wall user peak KiB + (microseconds) + +Why this script exists rather than a `sort -t, -k5`: the ranking on its own does +not answer the question the issue asks. ``--parallel`` and the cgroup caps are +bounded by the SUM of the N heaviest concurrent compiles, not by the mean and not +by a single worst TU, so the derivation table at the bottom is the actual +deliverable. And the records need three kinds of care that a sort does not give: +ccache's preprocessor passes have to be classified out of the compile ranking +without being silently dropped, the file is APPENDED to across builds so an output +can appear more than once, and a warm compiler cache records nothing at all for a +cache hit — which makes a partial run look exactly like a cheap one unless the +coverage is stated. + +Usage: + python scripts/analyze_proc_stat.py build-cached/olo-proc-stat.csv + python scripts/analyze_proc_stat.py build/olo-proc-stat.csv --top 20 \ + --cap-gib 14 --json report.json --markdown report.md +""" + +from __future__ import annotations + +import argparse +import datetime +import json +import os +import re +import sys +from collections import defaultdict + +# The record is emitted by clang's Job.cpp with llvm::format, so the shape is fixed: +# two quoted fields then three unsigned integers. Both the separator spacing and the +# backslash escaping in Windows paths have varied across clang versions, so parse +# tolerantly (optional spaces) rather than splitting on "," and trusting the count. +RECORD_RE = re.compile( + r'^\s*"(?P[^"]*)"\s*,\s*"(?P[^"]*)"\s*,\s*' + r"(?P\d+)\s*,\s*(?P\d+)\s*,\s*(?P\d+)\s*$" +) + +OBJECT_SUFFIXES = (".o", ".obj") +# Directories under the build tree whose objects were not produced by a compile in this +# build, and so must not reach the coverage denominator. +EXCLUDED_OBJECT_DIRS = frozenset({"vcpkg_installed", "CMakeScratch", "Testing"}) +# A preprocessor or intermediate output, NOT a translation unit's real compile. ccache +# in preprocessor mode runs `-E` into its own tmp dir, and that pass carries the same +# flag, so a single cache MISS appends TWO records. Counted and reported separately — +# folding them into the compile ranking would understate nothing but would inflate the +# record count, and dropping them silently would hide that ccache changed modes. +# `.pch`/`.gch`/`.pcm` are here for a specific reason, not for completeness: a precompiled +# header or a module is produced by a COMPILER invocation whose output has no object +# suffix, so without them it falls through to the `link` fallback below. That would report +# a PCH as a link — and worse, would let `--require-link-records` pass on a build where no +# linker ever ran. OLO_ENABLE_PCH defaults ON whenever the compiler cache is off, so this +# is the default local configuration, not an exotic one. +INTERMEDIATE_SUFFIXES = ( + ".ii", ".i", ".s", ".bc", ".tmp", ".d", + ".pch", ".gch", ".pcm", +) +# CMake's object path for target T and source S is CMakeFiles/T.dir/. +# (multi-config generators insert the config: CMakeFiles/T.dir//.). +OBJECT_PATH_RE = re.compile( + r"CMakeFiles[\\/](?P[^\\/]+)\.dir[\\/](?P.+)$" +) + +KIB_PER_GIB = 1024 * 1024 + + +class Record: + __slots__ = ("tool", "output", "wall_us", "user_us", "peak_kib", "kind") + + def __init__(self, tool: str, output: str, wall_us: int, user_us: int, peak_kib: int): + self.tool = tool + # Windows paths arrive with escaped separators; normalise to forward slashes so + # the same TU compares equal whichever host produced the records. + self.output = output.replace("\\\\", "/").replace("\\", "/") + self.wall_us = wall_us + self.user_us = user_us + self.peak_kib = peak_kib + self.kind = classify(self.output) + + @property + def peak_gib(self) -> float: + return self.peak_kib / KIB_PER_GIB + + @property + def wall_s(self) -> float: + return self.wall_us / 1_000_000 + + +def classify(output: str) -> str: + """One of 'compile', 'link', 'intermediate'.""" + lowered = output.lower() + # Order matters: ccache writes its preprocessor output as .ii inside its own + # tmp dir, and an intermediate check must win over the extension-less 'link' guess. + if lowered.endswith(INTERMEDIATE_SUFFIXES) or "/ccache/tmp/" in lowered or "/tmp/cpp_stdout" in lowered: + return "intermediate" + if lowered.endswith(OBJECT_SUFFIXES): + return "compile" + # clang writes "-" for output on stdout (e.g. a bare -E), which is not a link. + if output == "-" or output == "": + return "intermediate" + return "link" + + +def describe(output: str) -> str: + """A readable 'target: source' label for an object path, or the path itself.""" + match = OBJECT_PATH_RE.search(output) + if not match: + return output + target = match.group("target") + rest = match.group("rest") + # Strip the object suffix CMake appended to the source name. + for suffix in OBJECT_SUFFIXES: + if rest.lower().endswith(suffix): + rest = rest[: -len(suffix)] + break + # A multi-config generator inserts the config as a path component + # (CMakeFiles/T.dir/Debug/src/Foo.cpp.obj). Drop it so the label names the source, + # matched against the known config names rather than "strip the first component" — + # a single-config tree has a real source directory there. + for config in ("Debug", "Release", "Dist", "RelWithDebInfo", "MinSizeRel"): + if rest.startswith(config + "/"): + rest = rest[len(config) + 1 :] + break + return f"{target}: {rest}" + + +def collect_record_files(paths: list[str], name: str) -> list[str]: + """Expand the given paths into the list of records files to read. + + A directory is searched RECURSIVELY, and that is not a convenience — it is + required for correctness on the Linux CI generator. The flag's path must stay + relative (see cmake/ProcStatReport.cmake for the ccache measurement that forces + it), and clang resolves a relative path against the build tool's working + directory — which is NOT the same directory for every generator: + + Ninja runs every command from the top build dir + -> ONE build/olo-proc-stat.csv + Unix Makefiles `cd`s into each target's own subdirectory build dir first + -> build/olo-proc-stat.csv, build/OloEngine/olo-proc-stat.csv, + build/OloEngine/tests/olo-proc-stat.csv, ... + + Verified by generating both and reading the emitted compile rule; the Makefiles + rule is literally `cd /sub && clang++ ... -c ...`. Every Linux job in this + repo configures with no -G and therefore gets Unix Makefiles, so a reader that + only opened the top-level file would silently rank a fraction of the build — and + a fraction that happens to EXCLUDE the engine and test subdirectories, which is + where the heavy TUs live. + """ + found: list[str] = [] + for path in paths: + if os.path.isdir(path): + for root, _dirs, files in os.walk(path): + if name in files: + found.append(os.path.join(root, name)) + else: + found.append(path) + return found + + +def parse(path: str) -> tuple[list[Record], int]: + """Returns (records, unparsable_line_count). + + Unparsable lines are counted rather than ignored: several clang processes append + to this file concurrently, and although each writes a single short line, a torn + write is the one failure mode that would silently bias a ranking downward. If the + count is ever non-zero the report says so instead of looking clean. + """ + records: list[Record] = [] + malformed = 0 + with open(path, "r", encoding="utf-8", errors="replace") as handle: + for line in handle: + if not line.strip(): + continue + match = RECORD_RE.match(line) + if not match: + malformed += 1 + continue + records.append( + Record( + match.group("tool"), + match.group("output"), + int(match.group("wall_us")), + int(match.group("user_us")), + int(match.group("peak_kib")), + ) + ) + return records, malformed + + +def collapse(records: list[Record]) -> list[Record]: + """One entry per output, holding its MAXIMUM observed peak RSS. + + The records file is appended to and never truncated, and the records carry no + timestamp, so an incremental build on top of an earlier one leaves several + records for the same object. Max is the right reduction for a memory ceiling — + and it is also why CI deletes the file before building, so a published ranking + describes exactly one build. + """ + best: dict[str, Record] = {} + for record in records: + existing = best.get(record.output) + if existing is None or record.peak_kib > existing.peak_kib: + best[record.output] = record + return sorted(best.values(), key=lambda r: r.peak_kib, reverse=True) + + +def expected_compiles(build_dir: str | None, explicit: int | None) -> tuple[int | None, str]: + """How many compiles this build PRODUCED, for the coverage line. + + Returns (count, provenance). None when it cannot be established — stated as + unknown rather than guessed, because the coverage percentage is the one number + that tells a reader whether the ranking is complete. + + The denominator is the OBJECT FILES sitting in the build tree, deliberately NOT + ``compile_commands.json``: that file describes every TU in the PROJECT, while a + CI job builds one target (``--target OloEngine-Tests``), so comparing against it + would report a large, entirely legitimate shortfall and train a reader to ignore + the one line that distinguishes a partial ranking from a complete one. Objects on + disk are what the build actually compiled, whether from the compiler or from the + cache — which is exactly the comparison that exposes a warm cache. + """ + if explicit is not None: + return explicit, "--expected" + if build_dir is None: + return None, "no --build-dir given" + if not os.path.isdir(build_dir): + return None, f"'{build_dir}' is not a directory" + count = 0 + for root, dirs, files in os.walk(build_dir): + # Prune directories holding objects this build did not compile. vcpkg installs + # prebuilt artifacts into /vcpkg_installed and some ports ship .o files; + # counting those would inflate the denominator and fail CI's coverage gate for a + # reason that has nothing to do with the measurement. Pruned in `dirs` rather + # than filtered afterwards so the walk does not descend a large tree at all. + dirs[:] = [d for d in dirs if d not in EXCLUDED_OBJECT_DIRS] + for name in files: + if name.lower().endswith(OBJECT_SUFFIXES): + count += 1 + return count, f"object files under {build_dir}" + + +# The build settings that change what a TU costs, and therefore decide whether two +# rankings are comparable at all. Recorded WITH the numbers, because the figures this work +# replaced — per-TU peak RSS in MB, committed as CMakeLists.txt comments by an #822 +# follow-up — say nothing about how they were taken, and are up to 8.7x too high. The +# cause was a recipe that built a whole target while claiming to build one object and then +# credited the largest concurrent compile to the named TU; an isolated re-measurement +# caught it, and this census independently agrees with that correction to within 1.11-1.23x +# (docs/agent-rules/build-memory-per-tu.md). None of that could be established FROM the +# committed numbers — only by re-running them. An artifact that does not say how it was +# produced cannot be checked, and decays exactly like a comment. +PROVENANCE_KEYS = ( + "CMAKE_CXX_COMPILER", + "CMAKE_BUILD_TYPE", + "CMAKE_GENERATOR", + "OLO_ENABLE_PCH", + "OLO_ENABLE_UNITY_BUILD", + "OLO_ENABLE_COMPILER_CACHE", + "OLO_ENABLE_ASAN", + "OLO_ENABLE_UBSAN", + "OLO_ENABLE_TSAN", + "OLO_ENABLE_LTO", + "OLO_HEAVY_COMPILE_JOBS", + "OLO_LINK_JOBS", +) + + +def read_provenance(build_dir: str) -> dict: + """Pull the comparability-relevant settings out of the build tree's CMakeCache.txt.""" + found: dict[str, str] = {} + cache = os.path.join(build_dir, "CMakeCache.txt") + try: + with open(cache, "r", encoding="utf-8", errors="replace") as handle: + for line in handle: + if ":" not in line or "=" not in line or line.startswith(("#", "//")): + continue + name = line.split(":", 1)[0].strip() + if name in PROVENANCE_KEYS: + found[name] = line.split("=", 1)[1].strip() + except OSError: + # Not fatal, and not silently blank either: the report says it could not read + # the settings, so a reader knows the comparison is unsupported rather than + # assuming the defaults. + return {} + # The clang version is not a cache entry; it lives in the compiler-detection module. + for candidate in ("CMakeFiles", os.path.join("CMakeFiles", "4.4.2")): + probe = os.path.join(build_dir, candidate) + if not os.path.isdir(probe): + continue + for root, _dirs, files in os.walk(probe): + if "CMakeCXXCompiler.cmake" in files: + try: + text = open(os.path.join(root, "CMakeCXXCompiler.cmake"), encoding="utf-8").read() + except OSError: + break + for key in ("CMAKE_CXX_COMPILER_ID", "CMAKE_CXX_COMPILER_VERSION"): + marker = f'set({key} "' + if marker in text: + found[key] = text.split(marker, 1)[1].split('"', 1)[0] + break + if "CMAKE_CXX_COMPILER_VERSION" in found: + break + return found + + +def derive_parallel(compiles: list[Record], cap_gib: float, max_lanes: int) -> list[dict]: + """The worst-case cost of N concurrent compiles, for N = 1..max_lanes. + + The bound is the SUM of the N heaviest TUs, not N x mean: a build scheduler is + free to start the heavy ones together, and the whole point of the olo_heavy pool + is that it usually will not. So this is the pessimistic ceiling a cgroup cap has + to survive, which is the number a cap should be set from. + """ + # Capped at the number of compiles actually measured. Past that point there is no Nth + # heaviest TU to add, so every further row would repeat the previous total and + # `largest_fitting_lanes` would claim a width the data cannot support — 16 "safe" lanes + # from three measured compiles. A row we cannot substantiate is not printed. + rows = [] + running = 0 + for lanes in range(1, min(max_lanes, len(compiles)) + 1): + running += compiles[lanes - 1].peak_kib + worst_gib = running / KIB_PER_GIB + rows.append( + { + "lanes": lanes, + "worst_case_gib": round(worst_gib, 2), + "fits": worst_gib <= cap_gib if cap_gib > 0 else None, + } + ) + return rows + + +def render(report: dict, top: int) -> str: + """The Markdown report — also what lands in the CI job summary.""" + out: list[str] = [] + add = out.append + totals = report["totals"] + + add("## Build memory: per-invocation peak RSS") + add("") + add(f"Measured {report['measured_utc']}.") + add("") + provenance = report["provenance"] + if provenance: + # A ranking is only comparable to another taken with the same settings, so these + # travel WITH the numbers rather than in whatever prose cites them later. + add("
Build configuration (a ranking is only comparable to one taken the same way)") + add("") + add("| setting | value |") + add("|---|---|") + for key in sorted(provenance): + add(f"| `{key}` | `{provenance[key]}` |") + add("") + add("
") + else: + add( + "> **Build configuration unknown** — no readable `CMakeCache.txt` in the build " + "tree, so this ranking cannot be compared against another run. Pass `--build-dir`." + ) + add("") + files = report["records_files"] + if len(files) == 1: + add(f"Records file: `{files[0]}`") + else: + # Say how many rather than listing dozens: on the Makefiles generator there is + # one per subdirectory, and the count is the useful signal (a count of 1 on a + # Linux build would mean the gather missed the subdirectories). + add(f"Records files: **{len(files)}** (one per build subdirectory, Makefiles generator)") + add("") + add("| | count | max peak RSS | sum of peaks |") + add("|---|---:|---:|---:|") + for kind in ("compile", "link", "intermediate"): + group = totals[kind] + add( + f"| {kind} | {group['count']} | {group['max_gib']:.2f} GiB | {group['sum_gib']:.2f} GiB |" + ) + add("") + + coverage = report["coverage"] + if coverage["expected"] is None: + add( + f"**Coverage: {coverage['recorded']} compiles recorded; the build's total TU count could " + f"not be established ({coverage['provenance']}).** A ranking from a build with a warm " + "compiler cache is PARTIAL by construction — a cache hit runs no compiler and so " + "records nothing. Treat this as a lower bound unless the cache was cold." + ) + else: + add( + f"**Coverage: {coverage['recorded']} of {coverage['expected']} compiles recorded " + f"({coverage['percent']:.1f}%, from `{coverage['provenance']}`).**" + ) + if coverage["percent"] < 95.0: + add("") + add( + "> Under 95% — the missing TUs were compiler-cache hits (a hit runs no compiler, " + "so it appends no record) or were not built by this target selection. The ranking " + "is a LOWER BOUND on the build's peak, not a complete census." + ) + add("") + + if report["malformed_lines"]: + add( + f"> **{report['malformed_lines']} unparsable line(s)** in the records file — concurrent " + "appends can tear. The ranking is missing those invocations." + ) + add("") + + add(f"### Top {top} compiles by peak RSS") + add("") + add("| # | peak RSS | wall | target: source |") + add("|---:|---:|---:|---|") + for index, entry in enumerate(report["compiles"][:top], start=1): + add( + f"| {index} | **{entry['peak_gib']:.2f} GiB** | {entry['wall_s']:.1f} s | " + f"`{entry['label']}` |" + ) + add("") + + if report["links"]: + add("### Link steps by peak RSS") + add("") + add("| peak RSS | wall | tool | output |") + add("|---:|---:|---|---|") + for entry in report["links"]: + add( + f"| **{entry['peak_gib']:.2f} GiB** | {entry['wall_s']:.1f} s | " + f"`{entry['tool']}` | `{entry['output']}` |" + ) + add("") + else: + # NOT the same statement as "links are cheap", and the difference matters: a + # 0.00 GiB row in the totals table above would otherwise read as a measurement + # that found linking free. Under clang-cl there are no link records at all + # because CMake drives lld-link directly (cmake/ProcStatReport.cmake), so the + # honest report is an absence, not a zero. + add("### Link steps") + add("") + add( + "**No link records — links were NOT MEASURED in this build, which is not the " + "same as links being free.** Read the 0.00 GiB link row above as 'no data'. " + "The usual cause is a clang-cl build: CMake invokes `lld-link` directly there, " + "so there is no clang driver on the link line to carry `-fproc-stat-report`. " + "Use a Linux clang build, where the compiler drives the link and `ld.lld` is " + "reported as a driver subprocess." + ) + add("") + + derivation = report["derivation"] + add("### Worst-case concurrent compile cost") + add("") + add( + f"The N heaviest TUs summed — the pessimistic ceiling `--parallel N` has to survive, " + f"against a cap of **{derivation['cap_gib']:.0f} GiB**." + ) + add("") + add("| --parallel N | worst-case peak | fits the cap |") + add("|---:|---:|:---:|") + for row in derivation["rows"]: + verdict = "—" if row["fits"] is None else ("yes" if row["fits"] else "**NO**") + add(f"| {row['lanes']} | {row['worst_case_gib']:.2f} GiB | {verdict} |") + add("") + if derivation["largest_fitting_lanes"] is not None: + if report["links"]: + link_note = ( + "Compiles only — a link can run concurrently with compiles, and the " + f"heaviest link measured here is {totals['link']['max_gib']:.2f} GiB, " + "which this bound does not include." + ) + else: + link_note = ( + "Compiles only, and **links were not measured in this build** (see above), " + "so the real ceiling is higher by whatever a concurrent link costs." + ) + add( + f"**Largest N whose worst case fits {derivation['cap_gib']:.0f} GiB: " + f"{derivation['largest_fitting_lanes']}.** {link_note}" + ) + add("") + # Say plainly how pessimistic this is. The bound assumes the scheduler may start + # the N heaviest TUs together; the olo_heavy Ninja pool exists precisely to stop + # that, so a tree with a correctly-populated pool sits well under these numbers. + # Without this caveat the table argues for a lower -j than the evidence supports. + add( + "> This is a deliberately PESSIMISTIC bound: it assumes the build scheduler " + "may start the N heaviest TUs at the same moment. The `olo_heavy` job pool " + "(`OLO_HEAVY_COMPILE_JOBS`, Ninja only) exists to prevent exactly that, so a " + "tree whose pool membership matches this ranking runs below these figures — " + "and on the Makefiles generator every Linux CI job uses, the pool does not " + "exist at all and this bound is the operative one." + ) + add("") + + return "\n".join(out) + "\n" + + +def main(argv: list[str]) -> int: + parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) + parser.add_argument( + "records", + nargs="+", + help="records files, and/or build trees to search recursively for them. Pass the BUILD " + "TREE rather than a single file: the Unix Makefiles generator (every Linux CI job) " + "writes one file per subdirectory — see collect_record_files().", + ) + parser.add_argument( + "--name", + default="olo-proc-stat.csv", + help="records file name to look for inside a directory argument (default olo-proc-stat.csv; " + "match OLO_PROC_STAT_FILE if it was overridden)", + ) + parser.add_argument("--top", type=int, default=20, help="how many compiles to rank (default 20)") + parser.add_argument( + "--cap-gib", + type=float, + default=14.0, + help="memory cap to derive a safe -j against, in GiB (default 14 — the olo-ci runner unit's MemoryMax)", + ) + parser.add_argument("--max-lanes", type=int, default=16, help="highest -j to tabulate (default 16)") + parser.add_argument( + "--build-dir", + help="build tree to count object files in, for the coverage denominator " + "(default: the records file's own directory, which is where the build tool ran)", + ) + parser.add_argument( + "--expected", + type=int, + help="override the coverage denominator with an exact TU count", + ) + parser.add_argument( + "--require-link-records", + action="store_true", + help="exit non-zero unless at least one LINK was recorded. Use it on a Linux clang " + "build, where the compiler drives the link: it asserts the OUTCOME rather than the " + "generated build-system text, which is the only generator-independent check — the " + "Makefiles generator puts an executable's link command in link.txt on some platforms " + "and inlines it into build.make on others.", + ) + parser.add_argument( + "--fail-under-coverage", + type=float, + default=0.0, + help="exit non-zero when coverage is below this percentage. Use it in CI: an artifact " + "published from a warm cache is a PARTIAL ranking, and a partial ranking that nobody " + "notices is how this measurement decayed the first time (issue #1305).", + ) + parser.add_argument("--json", dest="json_out", help="also write the full report as JSON here") + parser.add_argument("--markdown", dest="markdown_out", help="also write the Markdown report here") + args = parser.parse_args(argv) + + missing = [p for p in args.records if not os.path.exists(p)] + if missing: + print(f"error: no such path: {', '.join(missing)}", file=sys.stderr) + return 2 + + record_files = collect_record_files(args.records, args.name) + if not record_files: + # A loud, actionable failure rather than an empty report: the overwhelmingly + # likely cause is a build configured without the option, and an empty ranking + # would read as "nothing used much memory". + print( + f"error: found no '{args.name}' under {', '.join(args.records)}.\n" + "The build was probably configured without -DOLO_BUILD_INSTRUMENTATION=ON, or " + "ran under a non-clang toolchain (the flag is clang-only — see " + "cmake/ProcStatReport.cmake).", + file=sys.stderr, + ) + return 2 + + # With several records files the outputs inside them are each relative to their OWN + # directory, so they must be re-rooted before being compared or de-duplicated — + # otherwise two different targets' objects could share a key and one would silently + # overwrite the other in collapse(). Re-rooting against the common build tree also + # makes the ranking readable: the label then says which subdirectory a TU came from. + common_root = os.path.commonpath([os.path.dirname(os.path.abspath(p)) for p in record_files]) + records: list[Record] = [] + malformed = 0 + for path in sorted(record_files): + file_records, file_malformed = parse(path) + prefix = os.path.relpath(os.path.dirname(os.path.abspath(path)), common_root).replace(os.sep, "/") + if prefix != ".": + for record in file_records: + record.output = f"{prefix}/{record.output}" + records.extend(file_records) + malformed += file_malformed + if not records: + print( + f"error: the {len(record_files)} records file(s) hold no parseable records " + f"({malformed} unparsable line(s)).", + file=sys.stderr, + ) + return 2 + + collapsed = collapse(records) + by_kind: dict[str, list[Record]] = defaultdict(list) + for record in collapsed: + by_kind[record.kind].append(record) + + compiles = by_kind["compile"] + links = by_kind["link"] + + build_dir = args.build_dir or common_root + expected, provenance = expected_compiles(build_dir, args.expected) + rows = derive_parallel(compiles, args.cap_gib, args.max_lanes) + fitting = [row["lanes"] for row in rows if row["fits"]] + + report = { + "measured_utc": datetime.datetime.now(datetime.timezone.utc).strftime("%Y-%m-%d %H:%M UTC"), + "provenance": read_provenance(build_dir), + "records_files": sorted(os.path.abspath(p) for p in record_files), + "malformed_lines": malformed, + "totals": { + kind: { + "count": len(by_kind[kind]), + "max_gib": max((r.peak_gib for r in by_kind[kind]), default=0.0), + "sum_gib": sum(r.peak_gib for r in by_kind[kind]), + } + for kind in ("compile", "link", "intermediate") + }, + "coverage": { + "recorded": len(compiles), + "expected": expected, + "provenance": provenance, + "percent": (100.0 * len(compiles) / expected) if expected else 0.0, + }, + "compiles": [ + { + "peak_gib": r.peak_gib, + "peak_kib": r.peak_kib, + "wall_s": r.wall_s, + "output": r.output, + "label": describe(r.output), + "tool": r.tool, + } + for r in compiles + ], + "links": [ + { + "peak_gib": r.peak_gib, + "peak_kib": r.peak_kib, + "wall_s": r.wall_s, + "output": r.output, + "tool": r.tool, + } + for r in links + ], + "derivation": { + "cap_gib": args.cap_gib, + "rows": rows, + "largest_fitting_lanes": max(fitting) if fitting else None, + }, + } + + markdown = render(report, args.top) + sys.stdout.write(markdown) + + if args.markdown_out: + with open(args.markdown_out, "w", encoding="utf-8", newline="\n") as handle: + handle.write(markdown) + if args.json_out: + with open(args.json_out, "w", encoding="utf-8", newline="\n") as handle: + json.dump(report, handle, indent=2) + handle.write("\n") + + if args.require_link_records and not report["links"]: + print( + "error: --require-link-records was given but NO link was recorded. On a Linux " + "clang build the compiler drives the link and ld.lld is reported as a driver " + "subprocess, so zero link records means the link half of issue #1305 is not " + "being measured — check that cmake/ProcStatReport.cmake added the flag to the " + "link line (it deliberately does not under clang-cl).", + file=sys.stderr, + ) + return 1 + + # The coverage gate is the anti-decay mechanism, so it fails LOUDLY on an + # unestablished denominator too rather than passing on a missing check — a report + # that cannot state its own completeness is the state this issue exists to end. + if args.fail_under_coverage > 0.0: + coverage = report["coverage"] + if coverage["expected"] is None: + print( + f"error: --fail-under-coverage {args.fail_under_coverage} was requested but the " + f"denominator could not be established ({coverage['provenance']}). Pass " + "--build-dir or --expected.", + file=sys.stderr, + ) + return 1 + if coverage["percent"] < args.fail_under_coverage: + print( + f"error: only {coverage['percent']:.1f}% of compiles were recorded " + f"({coverage['recorded']} of {coverage['expected']}), below the required " + f"{args.fail_under_coverage:.1f}%. This ranking is a LOWER BOUND, not a census: " + "a compiler-cache hit runs no compiler and appends no record, so build with the " + "cache off (-DOLO_ENABLE_COMPILER_CACHE=OFF) for a measurement run.", + file=sys.stderr, + ) + return 1 + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/scripts/link-semaphore.py b/scripts/link-semaphore.py new file mode 100644 index 0000000000..3fb77938f1 --- /dev/null +++ b/scripts/link-semaphore.py @@ -0,0 +1,233 @@ +#!/usr/bin/env python3 +"""Cross-tree link throttle for POSIX hosts. Wraps ONE linker invocation. + +The POSIX half of `.claude/skills/run-oloengine/link-semaphore.ps1`, wired by +cmake/LinkSemaphore.cmake. It exists because on Linux **nothing bounds linking at +all** (issue #1313), and that is not a theoretical gap: + + heaviest ld.lld link 11.17 GiB (Debug+ASan) / 10.03 GiB (plain Debug) + heaviest compile 8.09 GiB / 4.65 GiB + runner unit cap 14 GiB -- one link is 80% of it + account slice cap 19 GiB -- shared by BOTH olo-ci runner slots + +Measured by .github/workflows/build-memory.yml on the box, 1761 compiles at 99.9% +coverage. The link is the single largest memory consumer in the build, larger than +any translation unit, and it is 10 GiB even with no sanitizer. + +Three things that should have bounded it do not: + + * `OLO_LINK_JOBS` (the `olo_link` Ninja job pool) is guarded by + `if(CMAKE_GENERATOR MATCHES "Ninja")`. Every Linux job configures with no -G and + gets Unix Makefiles, where the pool silently does not exist. + * `olo_heavy` is a Ninja pool too, and throttles compiles rather than links. + * `cmake/LinkSemaphore.cmake` shells out to `pwsh`, which is not installed on the + box. It says so and fails open -- confirmed in the runner's own log: + Link semaphore: neither 'pwsh' nor 'powershell' found on PATH -- linking + WITHOUT cross-tree throttling. + +So `make -j2` was free to run that 11.17 GiB link beside a compile (19.26 GiB with +the heaviest one, past the unit cap and at the slice cap), and two concurrent links +across the two runner slots is 22.3 GiB against a 19 GiB slice. In practice the +OloEngine-Tests link lands at the end of the build when little else is left, which +is why this has not been failing constantly -- luck of scheduling, not a guarantee. + +WHY flock, AND WHY THE WINDOWS SCRIPT'S REASONING TRANSFERS. Its header explains at +length why it uses N named MUTEXES rather than one counting semaphore: a Windows +semaphore's count is owned by nobody, so a holder that is killed never gives its +permit back, and the throttle silently degrades to nothing. Only a mutex has an +owner, so an abandoned one is handed to the next waiter rather than leaked. + +`flock()` has exactly that ownership property, and gets it from the kernel: the lock +belongs to the open file description, so when the holder dies -- for any reason, by +any signal -- the descriptor closes and the lock is released. That is why this is a +handful of lines where the Windows version needs a careful argument, and it is also +why a PID-file scheme is not used here: it would have to answer "is the holder still +alive?", and every wrong answer either steals a permit or leaks one forever. + +FAILS OPEN, ALWAYS, for the same reason the Windows script does. If the lock +directory cannot be made, if fcntl is unavailable, if every permit is busy past the +timeout -- the link runs anyway. Unthrottled means "slower, maybe tight on memory". +A stuck throttle means no build on this machine ever finishes again, which is +strictly worse than the problem being solved. + +CONFIGURATION COMES FROM THE ENVIRONMENT, not from argv, and that is deliberate: +a linker command line is full of tokens an argument parser would try to interpret +(`-o`, `--start-group`, a bare `--`). Everything in argv after this script's own path +is the command, forwarded verbatim. The Windows script has no param() block for the +same reason. + + OLO_LINK_SEMAPHORE_SLOTS permits; default 2, <=0 disables the throttle + OLO_LINK_SEMAPHORE_TIMEOUT seconds to wait before linking anyway; default 1800 + OLO_LINK_SEMAPHORE_DIR where the permit files live; default /olo-link-semaphore + +The permit directory must be SHARED by everything that should contend. Both olo-ci +runner slots run as the same user on one machine, so the default temp directory is +the right scope: per-machine, not per-workspace. +""" + +from __future__ import annotations + +import math +import os +import subprocess +import sys +import tempfile +import time + +try: + import fcntl # POSIX only; absent on Windows, which uses the .ps1 wrapper instead. +except ImportError: # pragma: no cover - exercised on Windows + fcntl = None + +TAG = "[link-semaphore]" +DEFAULT_SLOTS = 2 +DEFAULT_TIMEOUT = 1800.0 +POLL_SECONDS = 0.25 + + +def note(message: str) -> None: + """Progress goes to stderr so it cannot be mistaken for linker output.""" + print(f"{TAG} {message}", file=sys.stderr, flush=True) + + +def env_number(name: str, default: float) -> float: + """A malformed value must not fail the link — it degrades to the default. + + NaN and infinity are rejected alongside the unparseable ones, and that is not + tidiness: `float("nan")` parses happily, and a NaN timeout makes every + `time.monotonic() >= deadline` comparison False, so the acquire loop never reaches + its deadline and waits forever. `float("inf")` does the same by construction. Either + one turns the fail-OPEN guarantee — the whole reason a stuck throttle cannot wedge + the machine — into a hang, from nothing worse than a typo in the environment. + """ + raw = os.environ.get(name) + if raw is None or raw.strip() == "": + return default + try: + value = float(raw) + except ValueError: + note(f"{name}={raw!r} is not a number — using {default}") + return default + if not math.isfinite(value): + note(f"{name}={raw!r} is not finite — using {default}") + return default + return value + + +class Permit: + """One held permit, released when the process exits or this is closed. + + Nothing here tries to release on a crash path: that is the kernel's job. The + lock lives on the open file description, so it goes away with the process no + matter how the process goes away. + """ + + def __init__(self) -> None: + self.fd: int | None = None + self.index: int | None = None + + def acquire(self, directory: str, slots: int, timeout: float) -> bool: + if fcntl is None: + note("fcntl unavailable (not a POSIX host) — linking unthrottled") + return False + try: + os.makedirs(directory, exist_ok=True) + except OSError as exc: + note(f"cannot create permit directory {directory!r} ({exc}) — linking unthrottled") + return False + + deadline = time.monotonic() + timeout + announced_wait = False + while True: + for index in range(slots): + path = os.path.join(directory, f"slot{index}") + try: + fd = os.open(path, os.O_CREAT | os.O_RDWR, 0o666) + except OSError as exc: + note(f"cannot open permit {path!r} ({exc}) — linking unthrottled") + return False + try: + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + except OSError: + os.close(fd) + continue # Held by someone else; try the next permit. + except Exception as exc: # pragma: no cover - defensive + os.close(fd) + note(f"flock failed ({exc}) — linking unthrottled") + return False + self.fd = fd + self.index = index + return True + + if time.monotonic() >= deadline: + note(f"timed out after {timeout:g}s waiting for a permit — linking anyway (fail-open)") + return False + if not announced_wait: + note(f"all {slots} permits busy — waiting") + announced_wait = True + time.sleep(POLL_SECONDS) + + def close(self) -> None: + if self.fd is not None: + try: + os.close(self.fd) # Releases the flock with the descriptor. + except OSError: + pass + self.fd = None + + +def run(command: list[str]) -> int: + """Run the link and return an exit status that distinguishes every outcome. + + The Windows script needs a sentinel to tell "the linker ran and returned N" from + "the linker never started", because a failed launch leaves $LASTEXITCODE holding + whatever it had — possibly 0, reporting success for a link that never happened. + Python raises instead, so the distinction is structural here; what still needs + care is not collapsing a real status into a boolean, and not losing a signal. + """ + try: + status = subprocess.call(command) + except FileNotFoundError: + note(f"failed to launch {command[0]!r}: not found") + return 1 + except OSError as exc: + note(f"failed to launch {command[0]!r}: {exc}") + return 1 + if status < 0: + # Killed by a signal. Report it the way a shell does rather than as a + # negative number, which an exit status cannot carry: a link killed by the + # OOM killer must not look like success or like an ordinary linker error. + signal_number = -status + note(f"{command[0]!r} was killed by signal {signal_number}") + return 128 + signal_number + return status + + +def main(argv: list[str]) -> int: + command = argv[1:] + if not command: + note("no command given; this is a CMake linker launcher, not a standalone tool") + return 2 + + slots = int(env_number("OLO_LINK_SEMAPHORE_SLOTS", DEFAULT_SLOTS)) + if slots <= 0: + # Deliberate opt-out, not a failure: say so, because a silently absent + # throttle is the state issue #1313 is about. + note("OLO_LINK_SEMAPHORE_SLOTS <= 0 — throttle disabled, linking unthrottled") + return run(command) + + directory = os.environ.get("OLO_LINK_SEMAPHORE_DIR") or os.path.join( + tempfile.gettempdir(), "olo-link-semaphore" + ) + timeout = env_number("OLO_LINK_SEMAPHORE_TIMEOUT", DEFAULT_TIMEOUT) + + permit = Permit() + try: + permit.acquire(directory, slots, timeout) + return run(command) + finally: + permit.close() + + +if __name__ == "__main__": + sys.exit(main(sys.argv))