Repository navigation
Conversation
The renderLaunchdPlist and renderSystemdUnit tests check fragments: a key/value pair, an ExecStart= line, the absence of an Environment= line. They would not notice a renderer that moved a newline, changed an indent or dropped the trailing newline, which launchd and systemd would load differently or not at all. Pin both unit files whole, with and without captured environment variables, including the XML escapes in the plist and the systemd quoting and %-escaping of Environment= and log paths. The expected output is what the current text/template rendering produces.
renderLaunchdPlist and renderSystemdUnit filled the launchd plist and the systemd unit with text/template. Executing a template evaluates each field reference through reflect.Value.MethodByName with a name known only at run time. The compiler flags such a call, and once one is reachable the linker stops pruning methods: any exported method of any reachable type might be looked up, so it keeps all of them. These two renderers were the only reachable template execution in cmd/gortex. Without them the binary drops from 315,899,314 to 309,820,674 bytes (-6,078,640, -1.92%), nearly all of it methods nothing calls and more than half of the release-size growth since v0.64.10 tracked in #1116. That was measured on a stripped darwin/arm64 build with the linux release's build tags plus -trimpath (-trimpath -tags netgo,osusergo,static_link -ldflags "-s -w -X ..."), not on the linux release binaries. Projected onto those, main would be about 1.70% (linux/amd64) and 1.39% (linux/arm64) larger than v0.64.10; the linux release build has to confirm that. Render with fmt over the same document text instead. The output is byte-identical: the golden tests from the previous commit and the existing render tests pass unchanged, and a one-off differential run against the template renderers over 20,000 generated inputs (XML and systemd metacharacters, percent signs, quotes, backslashes, newlines, template-like text) matched exactly. The functions keep their (string, error) signatures in this commit so callers and tests stay as they are; the error is now always nil.
One reachable call to reflect.Value.Method, or to MethodByName with a non-constant name, makes the linker keep every exported method of every reachable type. Executing a text/template or html/template makes that call. Nothing in a diff shows the cost: the template rendering removed in the previous commit held about 6 MB of the stripped binary. scripts/ci-dumpdep reads the reachability listing a -dumpdep link prints and fails when a reachable symbol carries the linker's <ReflectMethod> flag, or when text/template's executor is reachable (Execute is usually inlined into its caller, so the executor is what the listing names). For each one it prints the chain of symbols that reached it, ending at the first-party caller. It fails closed: input without a reachable main.main is rejected, with its first lines echoed, and so is a listing in which no symbol carries the <UsedInIface> flag every real program has. A failed or skipped link, or a listing format the tool cannot read, cannot pass as clean. The build-linux-static job now relinks the release build with -dumpdep and pipes the listing through the tool under pipefail. Compilation is cached from the job's build step, so the step costs one link; locally a darwin/arm64 relink with -dumpdep took 3-5 s and printed ~36 MB. build-native does the same on macOS and Windows, linked the way release.yml links those binaries: no build tags, and on Windows the mingw runtime linked in statically. A wiring test keeps the step in both jobs. On the parent of the previous commit the check reports 10 symbols, starting with text/template.(*Template).execute reached through runDaemonInstallService -> installLaunchd -> renderLaunchdPlist; on this tree it passes.
The NoXDG, PropagatesXDG and EscapesXML tests for the launchd plist and the NoXDG and PropagatesXDG tests for the systemd unit each check a few substrings of a rendered file. The golden tests render the same cases, with and without captured variables, XML escapes, systemd quoting and an empty PATH, and compare the whole file, so every fragment these checked is pinned there too. The PATH capture, empty PATH and daemon log override tests stay: they exercise how the installing environment reaches the renderers, which the golden tests hold fixed (empty PATH, variables passed in directly).
renderLaunchdPlist and renderSystemdUnit returned an error only because executing a template can fail. Rendered with fmt they cannot, so the error was always nil and installLaunchd and installSystemd carried a "render plist"/"render unit" failure branch that never ran. Return the string alone and drop those branches and the NoError checks in the render tests.
StubKind had no direct tests: a few packages spot-check IsStub on one id each. The resolver calls IsStub on every name-lookup candidate, and a profile of a large cold index put StubKind at 13% of CPU, most of it in runtime.concatstring2 building "<kind>::" for each candidate kind. Pin the stub-id grammar in a table (kind as the first or second segment, only when "::" follows it, empty repo prefix, near-miss kinds, third-segment kinds), and check StubKind against a copy of the current implementation on every prefix of generated ids, so a faster version has to agree at each truncation point around the separators. BenchmarkStubKind covers a first-party id (the common candidate), a per-repo stub and a legacy stub. On the current implementation (Apple M5 Pro, GOMAXPROCS=2) they take ~72, ~54 and ~33 ns/op with no heap allocation: the concatenations use a stack buffer, so the cost is the calls, not garbage.
StubKind looped over the four kinds and tested strings.HasPrefix(id, k+"::"), twice over for prefixed ids: up to eight run-time string concatenations for every id that is not a stub, which is most of what the resolver's IsStub filter sees. Match each kind in a switch on constant "<kind>::" prefixes instead. The concatenation is folded at compile time and the comparisons are inlined, so no call remains. The order (stdlib, external_call, builtin, module) and the grammar are unchanged; the table and the every-prefix comparison against the previous implementation from the previous commit pass. BenchmarkStubKind, 10 runs each (Apple M5 Pro, GOMAXPROCS=2): first_party ~72 ns/op -> ~8.1 ns/op repo_stub ~54 ns/op -> ~7.2 ns/op legacy_stub ~33 ns/op -> ~2.1 ns/op still with 0 B/op and 0 allocs/op.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two of the regressions since v0.64.10 tracked in #1116 have a cause in the code: release binary size and a hot spot in cold indexing. This draft fixes both and adds a CI guard so the size regression cannot come back unnoticed.
Draft status: the code and its tests are done. A fix-up round after review was interrupted before it finished. The cold-index check and the linux size confirmation are still open (see "Still open" below).
Changes
Release size
text/template.renderLaunchdPlistandrenderSystemdUnitwere the only reachable template execution incmd/gortex.reflect.Value.MethodByNamewith a name known only at run time. Once such a call is reachable, the linker stops pruning unused methods and keeps every exported method of every reachable type.fmtover the same text drops the stripped binary from 315,899,314 to 309,820,674 bytes: -6,078,640 bytes, -1.92%.-trimpath. Projected onto the linux release binaries, main would be about +1.70% (amd64) and +1.39% (arm64) over v0.64.10. The linux release build still has to confirm this.installLaunchd/installSystemdare gone.scripts/ci-dumpdep). The build relinks the release binary with-dumpdepand fails when a reachable symbol carries the linker's<ReflectMethod>flag, or when thetext/templateexecutor is reachable. On failure it prints the chain of symbols from the first-party caller to that symbol.main.main, or with no<UsedInIface>flag, is rejected, so a failed link or an unreadable listing cannot pass as clean.build-linux-static, and inbuild-nativeon macOS and Windows with the flagsrelease.ymlships. A wiring test keeps the step in both jobs.text/template.(*Template).executereached throughinstallLaunchd -> renderLaunchdPlist. It passes at this head.Cold index
StubKindwithout string building. A CPU profile of a large cold index putgraph.StubKindat about 13% of CPU, most of it building"<kind>::"for every candidate the resolver filters. A switch on constant prefixes does the same check.BenchmarkStubKind(10 runs, GOMAXPROCS=2): first-party id ~72 -> ~8.1 ns/op, per-repository stub ~54 -> ~7.2 ns/op, legacy stub ~33 -> ~2.1 ns/op. Still 0 allocations.Tested at this head (70c88e7)
go vetpasses on the touched packages at every commit in the series.go vetare clean;ci-test --check-workflowandcheck-cgo-flagspass;scripts/ci-dumpdeptests pass, including the flagless-listing rejection and the wiring test;internal/graphTestStubKind*passes, and so does thecmd/gortexrender, service and install subset;build-nativestep was run locally on macOS and passes. A real-dumpdeplisting with its flags stripped is rejected.cmd/gortexrun (without race) and aninternal/graphrace run passed. The full suites at this head run in CI.Still open
Interrupted fix-up items. The fix-up round had six items. Five are done:
The sixth is not finished: the cold-index check of the
StubKindchange.StubKind's share of the profile can explain, so this PR claims nothing from it.Cold-index comparison. A paired v0.64.10 vs this-head cold-index run on the reference host, using Commit an agent-profile load client and record v0.64.10 vs main baselines with cgroup, crash and CPU profiles #996's harness, has not been done.
Linux size. The size saving has not been measured on the linux release build.
Budget. No budget decision is recorded yet. The proposal: a +2% review line that requires a same-image attribution, and a +3% hard limit per architecture, both against v0.64.10, confirmed on the release candidate's linux build.
Refs #1116