From 79747620a1937c8a147d7f57ca87a920f4822e77 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Thu, 5 Nov 2026 21:10:51 -0700 Subject: [PATCH] perf(ci): skip LLVM AOT on the iOS gate, cache packages, drop the redundant main build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pull request took ~7m30s wall clock and the iOS head was ~7m04s of it; every other job finished inside 3m40s. So the iOS job was the only thing that decided how long CI took, and inside it a single phase decided the job: 312 of the build step's 354 seconds passed between "Optimizing assemblies for size" and the next line of output. That phase is LLVM. The iOS SDK enables it for every Release mobile build, and the comment sitting directly above the property that does so claims the value "is ignored when using the simulator" — which is wrong for this build. A default simulator build writes aot-instances.dll.s.bc (61 MB), .s.opt.bc (58 MB) and .llvm.o (30 MB), and pushing that bitcode through LLVM is where the five minutes went. Passing -p:MtouchUseLlvm=false in the workflow, measured on clean builds on one machine (rm -rf bin obj on all three projects, RID-less restore, then the exact command the workflow runs): default (LLVM on) 162.7 s -p:MtouchUseLlvm=false 43.1 s AOT itself still runs, which is the coverage this job exists for (#105): the Mono backend still emits aot-instances.dll.o and System.Private.CoreLib.dll.o, only the LLVM optimisation pipeline is skipped. Both builds produce 148 warnings and 0 errors, a 136 MB .app, a 254 KB Assets.car and 6 CFBundleIcon* keys, so the asset-catalogue assertion still proves what it proved before, and the lock files are untouched. What is given up is LLVM's optimisation of native code this job compiles and throws away — a 32.7 MB app binary instead of 35.5 MB. The flag is on the command line rather than in the csproj so local and device builds keep LLVM. Two smaller things, neither on the critical path: Nothing in this repo cached NuGet packages — all four jobs logged cache: false and re-downloaded the whole graph every run, and the macos and ios jobs did it twice because their from-scratch-derivation guard restores again with --force-evaluate. The eight committed packages.lock.json files make the cache key exact rather than approximate, since locked mode already pins every resolved version to those same files. The push trigger on main rebuilt a tree this workflow had already built. The ruleset on main carries merge_queue with max_entries_to_merge 1 and SQUASH, so the merge_group run builds main-plus-that-one-PR, which is the commit that lands; over one week that was 13 push runs against 13 merge_group runs, perfectly paired, each re-spending ~10 minutes of macOS at this repository's 10x billing. What makes dropping it safe is specifically that bypass_actors is empty, so nothing can reach main outside the queue — noted in both places, because if a bypass actor is ever added the trigger has to come back. The cost is that commits on main no longer carry their own check status. The merge-queue README still said the queue was blocked on plan; it has been live since September. Replaced the enabling instructions with the settings actually in force, and recorded which two of them the push-trigger removal depends on. Co-Authored-By: Claude Opus 5 --- .github/dependency-updates/README.md | 7 +- .github/merge-queue/README.md | 104 ++++++++++++++------------- .github/workflows/build.yml | 100 +++++++++++++++++++++++--- 3 files changed, 149 insertions(+), 62 deletions(-) diff --git a/.github/dependency-updates/README.md b/.github/dependency-updates/README.md index 902eb61e..90e8cb09 100644 --- a/.github/dependency-updates/README.md +++ b/.github/dependency-updates/README.md @@ -151,9 +151,10 @@ invocation: success is the exact failure mode this whole file is about: silence that looks like health. Both workflows fail the job on 2. -Every self-test in `.github/scripts` also runs on every push and PR via the -`scripts` job in `build.yml` — without it, these two guards would only be exercised -on a Dependabot PR or on a Monday, and a regression could sit unnoticed for a week. +Every self-test in `.github/scripts` also runs on every pull request and every +merge-queue entry via the `scripts` job in `build.yml` — without it, these two guards +would only be exercised on a Dependabot PR or on a Monday, and a regression could sit +unnoticed for a week. `refresh_lock_files.sh` is not a guard — it is a remedy, and the only shell script here — but it follows the same shape for the same reason: its refusals are the part diff --git a/.github/merge-queue/README.md b/.github/merge-queue/README.md index d9dd22c0..d0e3da41 100644 --- a/.github/merge-queue/README.md +++ b/.github/merge-queue/README.md @@ -7,8 +7,10 @@ the queued PRs in order — and merges only if that tentative result is green. I means you can queue a stack of PRs and walk away, rather than babysitting rebase-and-wait for each one. -**Status: the repository side is wired, the GitHub side is blocked on plan.** See -[Enabling it](#enabling-it). +**Status: live.** The ruleset on `main` is active and every merge goes through the +queue. See [How it is configured](#how-it-is-configured) for the settings actually in +force, and [What it cost](#what-it-cost) for the one workflow change that depended on +it. ## What is already done @@ -29,6 +31,11 @@ preserving: - **`cancel-in-progress` is keyed on `pull_request` only.** A cancelled `merge_group` run reports as a *failed* required check, which ejects that PR from the queue and forces everything behind it to rebuild. Never broaden that condition to `true`. +- **There is no `push: branches: [main]` trigger, and that is load-bearing on the + queue.** The `merge_group` run builds main-plus-one-PR squashed, which is exactly the + commit that lands, so a push-triggered run rebuilt an already-built tree. It was + removed. This is only safe while nothing can reach `main` outside the queue — see the + `bypass_actors` note below. If that ever changes, the trigger must come back. ## Checks that are safe to require @@ -50,52 +57,47 @@ pull-request-time check. Requiring it would hang every queue entry until timeout same reasoning applies to any future check whose input is the PR itself rather than the resulting tree. -## Enabling it - -GitHub exposes merge queue only through a branch ruleset or branch protection rule on -`main`, and **neither is available on this repository today**: - -``` -$ gh api repos/daqifi/daqifi-avalonia/rulesets -Upgrade to GitHub Pro or make this repository public to enable this feature. (HTTP 403) -``` - -`daqifi` is on the **free** plan and this repository is **private**. Merge queues are -available on public organization repositories, and on private repositories only under -GitHub Enterprise Cloud — the Team plan does not include them for private repos. So -there are two routes: - -1. **Make the repository public.** Unblocks merge queue and makes Actions minutes free - (relevant below). Worth noting that this repo is the private outlier among its own - dependencies — [`daqifi-desktop`](https://github.com/daqifi/daqifi-desktop), the WPF - app it ports, and [`daqifi-core`](https://github.com/daqifi/daqifi-core), the library - it wraps, are both already public. Still a product decision, not a CI one. -2. **Move the org to GitHub Enterprise Cloud.** Note that GitHub **Team** would not - help: the availability line is "any public repository owned by an organization, or - ... private repositories owned by organizations using GitHub Enterprise Cloud". - -Once one of those is true, in **Settings → Rules → Rulesets** (or branch protection) -for `main`: - -- Enable **Require merge queue**. -- Add the six checks above under **Require status checks to pass**. -- **Maximum pull requests to build concurrently**: start at **1–2**, not the default 5 - — see the cost note. -- **Merge method**: `Squash`, to match this repo's history. -- **Maximum PRs to merge / batch size**: `1` while the queue is new. Batching merges a - group as one unit, so one bad PR fails the whole batch and everything re-forms; - batch only once green runs are the norm. -- **Build timeout**: the full build is ~6–11 minutes wall clock, so the 60-minute - default is generous. Leave it. - -Also consider turning on **Allow auto-merge** (`Settings → General`), currently off. -With a queue, auto-merge is how a PR joins the queue on approval instead of waiting -for someone to click. - -### Cost note - -The queue rebuilds *per entry*, so N queued PRs cost roughly N full builds. `build.yml` -has two `macos-latest` jobs (`Desktop head on macOS`, `iOS head`), and macOS runners -bill at **10×** on private repositories. Concurrency 5 on a private repo can burn a -free-plan month of Actions minutes quickly — hence the recommendation to start at 1–2. -If the repository goes public instead, this stops mattering. +## How it is configured + +Settled in **Settings → Rules → Rulesets** on `main` (ruleset `main`, enforcement +`active`). The settings in force, which are the ones the notes above reason about: + +| Setting | Value | +| --- | --- | +| Merge method | `SQUASH` | +| Max entries to build concurrently | `2` | +| Min / max entries to merge | `1` / `1` | +| Min wait before merging | 5 minutes | +| Grouping strategy | `ALLGREEN` | +| Status-check timeout | 60 minutes | +| Required checks | the six in the table above | +| `bypass_actors` | **empty** | + +Two of those rows are relied on elsewhere and should not be changed casually: + +- **`max_entries_to_merge: 1`** keeps a queue entry equal to one PR. Batching merges a + group as one unit, so one bad PR fails the whole batch and everything re-forms. It is + also what makes the entry's build identical to the commit that lands, which is the + premise of dropping the `push` trigger. +- **`bypass_actors: []`** is what makes `main` fully covered without a `push` trigger. + Adding a bypass actor — an admin, an app, a deploy key — reopens the hole that trigger + used to close. Add the trigger back in the same change, or do not add the actor. + +Alongside the ruleset, the repository also forbids `non_fast_forward` on `main` and +requires a pull request (0 approvals, squash only). + +## What it cost + +Before the queue existed, every merge produced three full runs of `build.yml`: the PR's +own, then — once the queue arrived — the `merge_group` entry, then a `push` run on +`main`. Measured over one week: 58 `pull_request`, 13 `merge_group`, 13 `push`. The last +two are perfectly paired because they are the same tree. + +That mattered because `build.yml` has two `macos-latest` jobs (`Desktop head on macOS`, +`iOS head`) and macOS runners bill at **10×** on private repositories, so each redundant +run was roughly 10 minutes of wall clock at 10× — about a third of this workflow's spend +for no new information. Dropping the `push` trigger removed it. + +The queue still rebuilds *per entry*, so N queued PRs cost roughly N full builds. That +is why concurrency is 2 rather than the default 5; if the repository ever goes public, +Actions minutes become free and this stops mattering. diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 1ac360d7..fdcb4065 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -24,8 +24,23 @@ name: Build # all. It is also opted out of the lock-file policy — see its csproj. on: - push: - branches: [main] + # No `push: branches: [main]`. It was removed once the merge queue went live, + # because by then it rebuilt a tree this workflow had ALREADY built: the ruleset on + # main carries merge_queue with max_entries_to_merge 1 and SQUASH, so the + # merge_group run below builds main-plus-that-one-PR — which is byte-for-byte the + # commit that then lands. Over one week that was 13 push runs against 13 merge_group + # runs, perfectly paired, each one re-spending ~10 minutes of macOS at this + # repository's 10x private-repo billing for no new information. + # + # What makes this safe is specifically that NOTHING can reach main except through + # that queue. The same ruleset requires a pull request, forbids non-fast-forward, + # and — the part that actually matters — has an EMPTY bypass_actors list, so there + # is no admin or app push that could land unbuilt. Check that before re-deriving + # this: if a bypass actor is ever added, or the queue is turned off, main stops + # being covered the moment this trigger is gone, and it must come back. + # + # The cost, stated plainly: commits on main no longer carry their own check status. + # The green tick lives on the queue entry that produced them instead. pull_request: # Required for the merge queue. A queue entry is built on a synthetic # `refs/heads/gh-readonly-queue/main/pr-N-` ref, and a required check with no @@ -42,9 +57,9 @@ permissions: contents: read concurrency: - # Cancel superseded runs on a PR branch, but never on main and never in the merge - # queue. This condition must stay keyed on `pull_request` alone: a cancelled - # `merge_group` run reports as a FAILED required check, which ejects that pull + # Cancel superseded runs on a PR branch, but never in the merge queue and never on + # a manual dispatch. This condition must stay keyed on `pull_request` alone: a + # cancelled `merge_group` run reports as a FAILED required check, which ejects that pull # request from the queue and forces every entry queued behind it to rebuild. The # group is safe as-is because `github.ref` is already unique per queue entry, so # two queue runs can never collide in the first place. @@ -61,8 +76,9 @@ jobs: # only on Dependabot's own pull requests, and check_core_drift.py only on a # weekly schedule. A change that broke either would sit undetected until the next # Dependabot PR or the next Monday — and both guards exist precisely because - # nobody was watching. This job runs every self-test on every push and PR, so a - # broken guard fails here instead of going quiet where it matters. + # nobody was watching. This job runs every self-test on every pull request and + # every merge-queue entry, so a broken guard fails here instead of going quiet + # where it matters. # # The other two are ALSO invoked in place, next to the step whose verdict they # gate. That duplication is deliberate: this job proves the guard works, the @@ -102,9 +118,23 @@ jobs: # makes a floating SDK an input to locked mode that nobody controls. '10.0.x' # floated across a whole feature band (to 10.0.400, runtime 10.0.11) and broke # restore with NU1004 on a repo nobody had touched in 12 days. See #107. + # + # The cache is the global packages folder, keyed on every committed lock file. + # Nothing in this repo was cached before, so all four jobs re-downloaded the + # whole graph on every run — and the macos and ios jobs do it TWICE, because + # their "must equal a from-scratch derivation" step restores a second time with + # --force-evaluate. + # + # Keying on '**/packages.lock.json' rather than the csproj files is what makes + # this exact rather than approximate: locked mode already pins every resolved + # version to those files, so a key derived from them cannot serve a cache that + # disagrees with what restore is about to ask for. A lock-file change misses the + # cache and repopulates it, which is the correct behaviour. - uses: actions/setup-dotnet@v4 with: global-json-file: global.json + cache: true + cache-dependency-path: '**/packages.lock.json' # Fail loudly if the locked-mode gate is NOT actually armed. Without this the # whole workflow could go green while restores silently ran unlocked — the @@ -480,9 +510,13 @@ jobs: - uses: actions/checkout@v4 # Same pin as every other job — see the comment on the desktop job, and #107. + # + # Same package cache as the desktop job — see the comment there. - uses: actions/setup-dotnet@v4 with: global-json-file: global.json + cache: true + cache-dependency-path: '**/packages.lock.json' # No -r, for the reason the ios job gives at length and Directory.Build.props # gives at more: an explicit-RID restore REWRITES the lock file to hold only @@ -910,9 +944,13 @@ jobs: # Same pin as the desktop job — see the comment there, and #107. This job is where # the drift actually bit: the Android head trims, so it carries the implicit # Microsoft.NET.ILLink.Tasks reference whose version the SDK's bundled runtime picks. + # + # Same package cache as the desktop job — see the comment there. - uses: actions/setup-dotnet@v4 with: global-json-file: global.json + cache: true + cache-dependency-path: '**/packages.lock.json' # Not preinstalled with the SDK; the Android SDK itself ships on the runner # image and is pointed at by ANDROID_HOME. @@ -1019,9 +1057,13 @@ jobs: # Same pin as the other two jobs — see the comment on the desktop job, and #107. # It matters more here than anywhere: the SDK band also decides which iOS workload # is installed below, and therefore which Xcode this job has to select. + # + # Same package cache as the desktop job — see the comment there. - uses: actions/setup-dotnet@v4 with: global-json-file: global.json + cache: true + cache-dependency-path: '**/packages.lock.json' # Mirrors the android job's workload step. Not preinstalled with the SDK, and # setup-dotnet installs into its own prefix, so whatever the runner image ships @@ -1161,10 +1203,52 @@ jobs: # # --no-restore is load-bearing, not a speed-up: it is what keeps the -r above out # of restore, and therefore out of the lock file. + # + # MtouchUseLlvm=false is a CI-only build-time knob, and it is the single + # biggest thing in this workflow. The SDK turns LLVM on for every Release + # mobile build (Xamarin.Shared.Sdk.props, 'MtouchUseLlvm == "" And + # Configuration == Release And SdkIsMobile'), and the comment sitting directly + # above that property says the value "is ignored when using the simulator, so + # it doesn't matter if we set it in all cases". That comment is WRONG for this + # build, and expensively so: a default simulator build writes + # aot-instances.dll.s.bc (61 MB), .s.opt.bc (58 MB) and .llvm.o (30 MB), and + # grinding that bitcode through LLVM is 312 of the step's 354 seconds. Do not + # re-derive this from the SDK comment; derive it from the files in + # obj/.../nativelibraries/aot-output/arm64. + # + # Measured, clean builds, same machine (rm -rf bin obj on all three projects, + # RID-less restore, then this exact command): + # default (LLVM on) 162.7 s + # -p:MtouchUseLlvm=false 43.7 s + # + # What this does NOT give up, which is the whole reason it is acceptable here: + # AOT still runs. The Mono backend still emits aot-instances.dll.o and + # System.Private.CoreLib.dll.o; only the LLVM optimisation pipeline is skipped. + # Both builds produce 148 warnings and 0 errors, a 136 MB .app, a 254 KB + # Assets.car and 6 CFBundleIcon* keys — so the asset-catalogue step below still + # asserts the same thing, and the three aot-only walls the csproj documents are + # frontend behaviour that LLVM never sees. + # + # What it DOES give up: LLVM's optimisation of native code this job never runs. + # The app binary is 32.7 MB instead of 35.5 MB. This job compiles a simulator + # build and throws it away, so that optimisation has no observer. + # + # Be honest about the cost, because it is the same shape as the one the csproj + # already records: CI's configuration now differs from the shipping one on a + # SECOND axis (codegen backend, on top of trimming). If an iOS bug ever turns + # out to be LLVM-specific, this flag is why CI did not see it — and the fix is + # a device build, not switching this back on, because the simulator head is not + # the shipping configuration either way. + # + # On the command line rather than in the csproj deliberately: a local or device + # build should keep LLVM. Unlike MtouchInterpreter (see the csproj's comment on + # why a -p: there behaves differently from a project property) nothing reads + # MtouchUseLlvm before Sdk.props, so a global property is simply honoured. - name: Build run: | dotnet build Daqifi.Avalonia.iOS/Daqifi.Avalonia.iOS.csproj \ - -c Release -r iossimulator-arm64 --no-restore + -c Release -r iossimulator-arm64 --no-restore \ + -p:MtouchUseLlvm=false # A second look, because this is the ONLY job that passes -r at all. The check # after restore cannot see a lock file rewritten by the build, and the whole point