Skip to content

build(cmake): compile the Windows test exit wrapper once for every test - #10124

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
LeandroLP23:perf/build-windows-test-exit-once
Oct 24, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
LeandroLP23:perf/build-windows-test-exit-once

Conversation

@LeandroLP23

@LeandroLP23 LeandroLP23 commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

What

On MinGW, add_test_executable adds cmake/WindowsTestExit.cpp (the --wrap=main wrapper) as a source of every test executable. With BUILD_TESTING=ON the build compiles that file once for each of the 1,223 test executables. This PR compiles it once, in an anyps5_windows_test_exit OBJECT library defined in the top-level CMakeLists.txt after the global compile options. Every test executable then links its object through $<TARGET_OBJECTS:...>. The wrapper uses no target-specific definitions or include directories, so one object fits every test. The --wrap=main link option is unchanged. target_sources is used instead of target_link_libraries, so targets that use the plain target_link_libraries signature are not affected.

Cost removed per full build. On a Ryzen AI 7 350 (Windows 11, WinLibs GCC 15.2, Release):

  • Without a ccache hit: one compile of the wrapper takes 2.1 to 3.0 s (5 runs on an idle machine, median 2.6 s). That is about 50 CPU minutes over 1,223 executables, about 3 minutes of wall time at 16 jobs.
  • With ccache hits: the 1,208 wrapper compiles in this build directory's .ninja_log add up to 8.6 CPU minutes (median 0.38 s).

Builds after editing a source file or header don't compile the wrapper on main either, so this only shortens full builds: a new build directory, a compiler or flag change, or a CI runner without a cache.

Tested

Windows 11 26300, WinLibs GCC 15.2.0, Release, BUILD_TESTING=ON, Radeon 860M (AMD driver 32.0.13058.8001). Rebased onto main 32d86275b7 (tools/check_main_sync.py: 1 ahead, 0 behind, clean), full build there:

Linux and macOS: MINGW is false there, so they define no target and add nothing.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none (build time only)
  • GPUs tested: n/a (build change; the GPU tests ran on AMD Radeon 860M, driver 32.0.13058.8001, Windows 11)
  • AI-assisted: yes (Claude Code)

@github-actions github-actions Bot added perf Performance change perf numbers missing perf pull request without a before and after summary line labels Oct 23, 2026
@github-actions

Copy link
Copy Markdown

This pull request is labeled perf, so it needs the summary line of a capture on main and one with the change, from the same title, scene and machine, as the Performance report form asks. Please paste both aps5-summary.jsonl lines, attach the two traces and add the output of tools/trace_digest.py.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the exact head 196f87f. The OBJECT-library approach appears sound for the described MinGW setup: the wrapper has no per-target definitions/includes, add_link_options is established before the object target, and the --wrap=main option remains on each executable. The reported Ninja rule count and before/after build measurements are useful and directly measure this build-time change; the scene-trace request from the generic perf label does not fit a build-only performance claim. Local checks on this exact head: git diff --check upstream/main...HEAD and python3 tools/check_conventions.py --base upstream/main pass. The host CI had Linux/macOS relinker and ASan pass when checked; Windows and macOS x86_64 relinker jobs were still running, while full Linux/Windows jobs were skipped by path policy. No local MinGW build was run. I found no blocking issue in the inspected diff.

@LeandroLP23
LeandroLP23 force-pushed the perf/build-windows-test-exit-once branch from 196f87f to 64da59d Compare October 23, 2026 10:21
@LeandroLP23 LeandroLP23 changed the title perf(build): compile the Windows test exit wrapper once for every test build(cmake): compile the Windows test exit wrapper once for every test Oct 23, 2026
@LeandroLP23

LeandroLP23 commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor Author

@MarcZX10 thank you for the review. Since this is a build-time change and has no runtime path to trace, I retitled it build(cmake): (commit amended, same diff, now 64da59d09f) so it doesn't carry the perf label's capture requirement. The measurement in the description is the evidence for it: the build.ninja rule count, and the cold and cached compile times.

AI-assisted: yes (Claude Code)

@github-actions github-actions Bot removed perf Performance change perf numbers missing perf pull request without a before and after summary line labels Oct 23, 2026
@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.

Pull request Shared files not compared
PR 674 CMakeLists.txt
PR 1835 CMakeLists.txt
PR 5021 CMakeLists.txt
PR 6044 CMakeLists.txt
PR 7201 CMakeLists.txt

@github-actions github-actions Bot added the build label Oct 23, 2026

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The re-reviewed head is 64da59d. It still applies cleanly, but the repository now has main at e3d03fb and python3 tools/check_main_sync.py reports this branch is 694 commits behind its merge base. Since the contribution checklist requires current main, please rebase this build-system change onto the current tip and rerun the conventions check and relevant build/relinker validation. The earlier Windows full-build evidence covers the former base; it does not validate integration with the current tree. The latest hosted relinker matrix on the former head passes, while the regular Linux/Windows jobs were skipped. No MinGW build was independently run here.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@LeandroLP23
LeandroLP23 force-pushed the perf/build-windows-test-exit-once branch from 64da59d to 94d9c6a Compare October 23, 2026 12:15
@LeandroLP23

Copy link
Copy Markdown
Contributor Author

@MarcZX10 rebased onto main 32d86275b7, now 94d9c6a73f (check_main_sync.py: 1 ahead, 0 behind, clean). check_conventions.py --base origin/main passes. I did a full Windows build on that base and ran the whole suite: 2,013 of 2,016 pass. The 3 failures are the known AMD Windows sample_order failures (#9507). The 18 skipped tests are the ones that also skip on the former base: no display, long paths disabled, or features this device lacks. build.ninja still has a single compile rule for the wrapper. I updated the Tested section to match.

AI-assisted: yes (Claude Code)

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head 94d9c6a73f74ed1caf9a3f4ee6068d3a9055cb30. The change remains limited to compiling the Windows test-exit wrapper as a shared object and linking it to test targets. Exact-head conventions and diff checks pass. The author reports one wrapper compile rule and a full Windows run of 2,013/2,016 tests; the three failures are the already-known AMD sample_order failures and 18 expected feature/display/path skips. That run used main 32d86275b7; I checked the intervening current-main paths and found no changes to the root/library CMake or test-registration files used by this change. GitHub reports the PR clean and mergeable; hosted product jobs are path-skipped. Approved.

@oneandonlydean oneandonlydean self-assigned this Oct 23, 2026
@oneandonlydean
oneandonlydean merged commit 225f3d4 into boykopovar:main Oct 24, 2026
14 of 16 checks passed
@LeandroLP23
LeandroLP23 deleted the perf/build-windows-test-exit-once branch October 24, 2026 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants