Skip to content

fix(pgo): Allow building multiple versions of the package to support PGO#722

Merged
kakkoyun merged 5 commits into
mainfrom
kakkoyun/fix_pgo_issues
Oct 22, 2025
Merged

fix(pgo): Allow building multiple versions of the package to support PGO#722
kakkoyun merged 5 commits into
mainfrom
kakkoyun/fix_pgo_issues

Conversation

@kakkoyun

@kakkoyun kakkoyun commented Oct 16, 2025

Copy link
Copy Markdown
Member

An attempt to fix PGO issues

orchestrion go build -pgo=default.pgo failed with "mismatched build ID" errors.

The Never-Build-Twice (NBT) service used only importPath as the cache key, but Profile-Guided Optimization (PGO) causes the same package to have different build IDs in different compilation contexts. The NBT service incorrectly rejected these as conflicts.

Changed the NBT cache to use a composite key of importPath + buildID instead of just importPath, allowing the same package to be compiled with different build IDs when PGO (or other build configurations) change the inputs.

Changes

Fixes #653

@kakkoyun kakkoyun changed the title 'fix(pgo): WIP' fix(pgo): WIP Oct 21, 2025
@kakkoyun
kakkoyun requested a review from Copilot October 22, 2025 09:36

This comment was marked as resolved.

@kakkoyun
kakkoyun force-pushed the kakkoyun/fix_pgo_issues branch from 7cad0c8 to ca471dc Compare October 22, 2025 10:51
@kakkoyun kakkoyun changed the title fix(pgo): WIP fix(pgo): Allow building multiple versions of the package to support PGO Oct 22, 2025
@kakkoyun
kakkoyun marked this pull request as ready for review October 22, 2025 11:00
@kakkoyun
kakkoyun requested a review from a team as a code owner October 22, 2025 11:00
@kakkoyun
kakkoyun force-pushed the kakkoyun/fix_pgo_issues branch from ca471dc to dec245a Compare October 22, 2025 11:10
Comment thread .github/workflows/validate.yml
@kakkoyun
kakkoyun force-pushed the kakkoyun/fix_pgo_issues branch from ed34813 to 29e42e6 Compare October 22, 2025 11:44
@kakkoyun kakkoyun added the AI Assisted AI/LLM assistance used in this PR (partially or fully) label Oct 22, 2025
@kakkoyun
kakkoyun force-pushed the kakkoyun/fix_pgo_issues branch from 77072f5 to 96343cc Compare October 22, 2025 12:04
@kakkoyun
kakkoyun force-pushed the kakkoyun/fix_pgo_issues branch from 8a1ff9c to a884096 Compare October 22, 2025 12:55
@kakkoyun
kakkoyun force-pushed the kakkoyun/fix_pgo_issues branch 5 times, most recently from a7d4f61 to 4355255 Compare October 22, 2025 15:20
@darccio

darccio commented Oct 22, 2025

Copy link
Copy Markdown
Member

@kakkoyun We also have a long standing issue #639 that I wonder if it might be fixed by your PR. I assume that parallel builds for multiple GOARCH might have different build IDs. If that's true, your PR should fix that scenario too.

@kakkoyun

Copy link
Copy Markdown
Member Author

@kakkoyun We also have a long standing issue #639 that I wonder if it might be fixed by your PR. I assume that parallel builds for multiple GOARCH might have different build IDs. If that's true, your PR should fix that scenario too.

I did a bit of digging but I guess it won't help. Go tries to have reproducible builds across the platforms so it doesn't actually consider GOARCH and GOOS while calculating the buildID.

// A final twist is that we'd prefer to have reproducible builds for release toolchains.
// It should be possible to cross-compile for Windows from either Linux or Mac
// or Windows itself and produce the same binaries, bit for bit. If the tool ID,
// which influences the action ID half of the build ID, is based on the content ID,
// then the Linux compiler binary and Mac compiler binary will have different tool IDs
// and therefore produce executables with different action IDs.
// To avoid this problem, for releases we use the release version string instead
// of the compiler binary's content hash. This assumes that all compilers built
// on all different systems are semantically equivalent, which is of course only true
// modulo bugs. (Producing the exact same executables also requires that the different
// build setups agree on details like $GOROOT and file name paths, but at least the
// tool IDs do not make it impossible.)

https://tip.golang.org/src/cmd/go/internal/work/buildid.go

So this issue might still persist. However, the fix maybe just another twist on the cacheKey calculation.

@kakkoyun
kakkoyun force-pushed the kakkoyun/fix_pgo_issues branch from 4355255 to cc8fe4e Compare October 22, 2025 15:51
@kakkoyun
kakkoyun enabled auto-merge October 22, 2025 15:52
@kakkoyun
kakkoyun force-pushed the kakkoyun/fix_pgo_issues branch from cc8fe4e to 0677d84 Compare October 22, 2025 16:00
@kakkoyun
kakkoyun added this pull request to the merge queue Oct 22, 2025
Merged via the queue into main with commit 69c6ada Oct 22, 2025
61 checks passed
@kakkoyun
kakkoyun deleted the kakkoyun/fix_pgo_issues branch October 22, 2025 16:57
@codecov

codecov Bot commented Oct 22, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 56.96203% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.14%. Comparing base (e061d12) to head (0677d84).
⚠️ Report is 30 commits behind head on main.

Files with missing lines Patch % Lines
internal/jobserver/client/client.go 42.22% 25 Missing and 1 partial ⚠️
internal/goproxy/proxy.go 70.83% 5 Missing and 2 partials ⚠️
internal/jobserver/nbt/nbt.go 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #722      +/-   ##
==========================================
+ Coverage   65.72%   67.14%   +1.42%     
==========================================
  Files         113      116       +3     
  Lines        7926     8541     +615     
==========================================
+ Hits         5209     5735     +526     
- Misses       2192     2259      +67     
- Partials      525      547      +22     
Components Coverage Δ
Generators 80.24% <ø> (ø)
Instruments ∅ <ø> (∅)
Go Driver 74.61% <68.29%> (-1.20%) ⬇️
Toolexec Driver 70.87% <100.00%> (+3.34%) ⬆️
Aspects 74.16% <75.32%> (+2.25%) ⬆️
Injector 74.80% <75.70%> (+2.00%) ⬆️
Job Server 66.00% <50.00%> (+0.08%) ⬆️
Other 67.14% <63.91%> (+1.42%) ⬆️
Files with missing lines Coverage Δ
internal/jobserver/server.go 42.71% <100.00%> (ø)
internal/toolexec/proxy/compile.go 62.29% <100.00%> (+5.15%) ⬆️
internal/jobserver/nbt/nbt.go 76.97% <87.50%> (-1.55%) ⬇️
internal/goproxy/proxy.go 75.38% <70.83%> (-1.89%) ⬇️
internal/jobserver/client/client.go 56.41% <42.22%> (-14.65%) ⬇️

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Assisted AI/LLM assistance used in this PR (partially or fully) conventional-commit/fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Errors with orchestrion go build when profile guided optimizations are enabled

4 participants