perf(sg): skip sagefile rebuild when nothing changed#802
Draft
perf(sg): skip sagefile rebuild when nothing changed#802
Conversation
The generated Makefile marks $(sagefile) as .PHONY, which forces Make to rebuild the sagefile binary on every invocation. In practice this means every `make` target pays a ~2s tax for `go mod tidy && go run .` even when nothing in .sage/ has changed, while the actual work (e.g. running mdformat) often takes under 200ms. An earlier version of the Makefile generator used file-based deps but this was replaced with .PHONY in b9faf86, likely because tracking sage's own library source is hard when developing sage itself (where a local `replace` directive means changes don't flow through go.mod). We fold dependency resolution into the sagefile binary itself as a "--deps" flag. The generated init() intercepts "--deps" before the normal target dispatch and calls SagefileDeps(), which uses go/build.ImportDir to resolve exactly which .go files participate in the build for the current platform — respecting build tags and excluding _test.go files, without the ~350ms cost of `go list`. The Makefile passes includePath as an argument so the output is relative to the Makefile's directory, handling nested namespace Makefiles correctly. For the bootstrap case where the sagefile does not exist yet, an ifneq/wildcard guard leaves sage_source_files empty and Make builds unconditionally since the target file is missing. If --deps returns empty on an existing binary (suggesting a stale or broken sagefile), Make errors out with a message pointing the user to `make sage`. To close the local-replace gap that motivated the original .PHONY, we also parse go.mod for local replace directives and walk each target directory for non-test .go files. This means that when developing sage itself, touching sg/path.go or tools/sgmdformat/tools.go correctly triggers a rebuild. Consumer repos have no local replaces, so their hot path is just ImportDir plus a go.mod read. Signed-off-by: Erik Cervin-Edin <erik.cervin-edin@einride.tech>
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
1 participant
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.
Problem
Every
makeinvocation rebuilds the sagefile binary (~1.9s) even when nothing in.sage/changed. The actual task (e.g.mdformat) takes ~180ms. The generated Makefile marks$(sagefile)as.PHONY, forcing unconditional rebuilds.Solution
Fold dependency resolution into the sagefile binary as a
--depsflag. Usesgo/build.ImportDirto resolve which.gofiles participate in the build (respecting build tags, excluding_test.go), and prints them so Make can track timestamps.How it works
$(shell $(sagefile) --deps .sage)returns the dep list, Make checks timestampsifneq/wildcardguard leaves deps empty, Make builds unconditionally (target missing)--depsfails/empty:$(error)tells user to runmake sagego.modfor localreplacetargets, walks each for.gofiles — so developing sage itself correctly triggers rebuildsResults
sg/path.gotouched (local replace)