benchmark: use PR merge-base for baseline comparison and add LTO workloads - #2373
benchmark: use PR merge-base for baseline comparison and add LTO workloads#2373cpunion wants to merge 7 commits into
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The benchmark changes themselves are sound: PR merge-base baseline selection avoids base drift, and the -lto=full workloads extend the workload struct cleanly (buildArgs is built once per workload and correctly does not alias item.flags). The bulk of this diff, however, is the internal/packages/load.go loader rewrite and the cl/static_init.go composite-literal static-init work carried in from the #2371 dependency. Those are where the review concerns concentrate.
Verified before flagging:
- The compiler's own path (
internal/build/build.go:454) setspackages.NeedDeps, so normal builds are unaffected by the loader change. benchmark/baseline/README.md(default and -lto=full)notes accurately match the new workloads.collectAllocStorescycle protection (visitedmap) is correct and covered by the new unit tests.
Notes not placed inline:
- Parallel typechecking removed (perf). The old loader typechecked the import DAG in parallel (
loadRecursiveExgoroutines +WaitGroup); the newLoadExrunspackages.Visit(...)sequentially, one package at a time. For large import graphs on multi-core hosts this is a real wall-clock regression, and the tiny hello-world benchmarks in this PR will not surface it. Appears to be a deliberate simplicity/correctness tradeoff — worth confirming it's acceptable. - Lost parse cache (perf, minor). The removed loader had a
parseCache;typecheckContext.parseFilenow does an unconditionalos.ReadFile+parser.ParseFileper file. Largely mitigated by theDeduper, but note the loss for repeated in-process loads. - Docs. New
cl/static_init.gofunctions (collectAllocStores,handleStoreVal,appendStaticInitPath,staticInitStorePathToAlloc) have non-trivial recursive/cycle/aliasing contracts but no doc comments. Separately, the rewrite deleted the explanatory comment abovenormalizeEmbedDriverDiagnosticswhose rationale still applies to the surviving conditional logic — consider restoring a condensed version. Also confirm the removal of the go-version-skew diagnostic (oldruntimeVersion < lpkg.goVersionblock) was intentional.
| // packages.Load typecheck or parse directly. We request files, imports, embed patterns, | ||
| // and module metadata from packages.Load (go list driver), and perform custom parsing | ||
| // and typechecking ourselves. | ||
| driverCfg.Mode = (origMode &^ (NeedTypes | NeedTypesSizes | NeedTypesInfo | NeedSyntax)) | NeedCompiledGoFiles | NeedImports | NeedName | NeedFiles |
There was a problem hiding this comment.
[P1] LoadEx no longer adds NeedDeps; non-NeedDeps callers lose transitive types
driverCfg.Mode derives from origMode and adds NeedImports but never NeedDeps. Per go/packages, with NeedImports but not NeedDeps the Imports map contains ID-only placeholder packages with no CompiledGoFiles/Types, so the subsequent packages.Visit(initial, nil, ...) cannot descend into transitive dependencies and the importerFunc returns "package %q without types was imported from %q" for any real import.
The compiler path (internal/build/build.go:454) sets NeedDeps, so builds are fine. But callers that request types without NeedDeps regress: internal/build/clean.go:57 (loadSyntax | NeedExportFile) and internal/plan9asm/bytealg_sigs_test.go:29 (NeedTypes | NeedImports, package internal/bytealg has imports). The removed refineEx materialized and typechecked the graph off NeedImports alone, so these used to work.
Suggest OR-ing NeedDeps into driverCfg.Mode whenever types/syntax are requested (or documenting the requirement and updating callers). Please confirm the plan9asm test still passes.
| hasCompilerSyntaxError := false | ||
| for _, err := range lpkg.Errors { | ||
| for _, err := range pkg.Errors { | ||
| if strings.Contains(err.Msg, "syntax error") { |
There was a problem hiding this comment.
[P2] Broad "syntax error" substring match can suppress real diagnostics
This new check flips hasCompilerSyntaxError for any error of any Kind whose message merely contains the substring "syntax error", after which appendError drops all subsequent scanner.ErrorList and types.Error diagnostics. The existing specific check (Kind == ListError && HasPrefix("# ")) already covered the compiler-diagnostic case it was designed for. The broad substring test risks hiding legitimate parse/type errors when an unrelated driver error happens to contain that phrase. Consider dropping the broad check and keeping only the specific one.
| if: github.event_name == 'pull_request' | ||
| id: merge-base | ||
| run: | | ||
| git fetch https://github.com/${{ github.event.pull_request.base.repo.full_name }}.git ${{ github.event.pull_request.base.ref }} |
There was a problem hiding this comment.
[P2] Untrusted pull_request context interpolated into run: (script injection)
github.event.pull_request.base.ref (and base.repo.full_name, head.sha on the following lines) are interpolated as raw text into a run: shell script. GitHub substitutes context expressions before the shell parses the script, so shell metacharacters in these values execute as code — the classic Actions script-injection pattern. A fork PR can use a branch name containing such characters.
Mitigating factor: the trigger is pull_request (not pull_request_target), so the token is read-only and fork PRs get no secrets, bounding the blast radius to runner code execution / cache poisoning. Still worth fixing by routing the values through env: and quoting them:
env:
PR_BASE_REPO: ${{ github.event.pull_request.base.repo.full_name }}
PR_BASE_REF: ${{ github.event.pull_request.base.ref }}
PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }}
run: |
git fetch "https://github.com/${PR_BASE_REPO}.git" "$PR_BASE_REF"
base_sha=$(git merge-base FETCH_HEAD "$PR_HEAD_SHA")|
|
||
| // Init initializes the global variable with the given value. | ||
| func (g Global) Init(v Expr) { | ||
| if g.impl.Name() == moduleZeroName { |
There was a problem hiding this comment.
[P3] Init/InitNil silently no-op for moduleZeroName sentinel
The guard correctly avoids overwriting the shared zero-sized-alloc sentinel's ConstNull initializer, but the early return is silent and matched by name string. Add a brief comment (mirroring the one on NewThreadLocalVar) on both Init and InitNil explaining why the sentinel is skipped, so the dropped write is discoverable by future maintainers.
| // package declarations are inconsistent. | ||
| lpkg.Types = types.NewPackage(lpkg.PkgPath, lpkg.Name) | ||
| lpkg.Fset = ld.Fset | ||
| func (tc *typecheckContext) computedSizes(pkg *Package) types.Sizes { |
There was a problem hiding this comment.
[P3] computedSizes silently substitutes sizes; hardcoded wasm StdSizes
The old LoadExWithGoVersion returned an error when a non-external driver couldn't supply type sizes. computedSizes now always returns non-nil via silent fallbacks, and the wasm fallback changed from types.SizesFor("gc", runtime.GOARCH) to a hardcoded &types.StdSizes{WordSize: 4, MaxAlign: 4}. Silently substituting sizes can mask misconfiguration and, for uncommon arches, produce subtly wrong layout. Consider a comment documenting the intentional fallback and confirming the hardcoded wasm sizes match the targeted ABI.
a994bcb to
e573753
Compare
…oad and internal typechecking
…res, and update test fixtures for x/tools v0.49.0
e573753 to
bbe6556
Compare
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Closing in favor of #2371 which combines the dependency bump, static init optimization, and the benchmark merge-base/LTO workloads into a unified PR on top of the latest main. |
1 participant
Note
Depends on #2371: This PR is built on top of #2371 (
all: bump goplus/mod to 0.22.0, decouple loader, and fold composite literal static inits).Summary
Accurate Baseline via PR
merge-base(.github/workflows/benchmark.yml):git merge-basecommon ancestor rather than the latest floatingmainHEAD.mainafter branch creation from polluting or distorting PR performance and binary size diffs (base drift).LTO Workload Comparisons (
benchmark/baseline/main.go):-lto=fullcompilation workloads forcprintf_lto,println_lto, andfmtprintf_ltoalongside standard builds.workloaddefinition to support arbitrary compiler flags.Verification
go test -v ./benchmark/baseline(PASS)bash test/buildcache/test.sh(18/18 PASS)