Repository navigation
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code review summary
Reviewed the full diff (477f959...6c127d4) across code quality, performance, security, and documentation. This is a carefully reasoned CI-overhead reduction, and it's backed by an unusually thorough set of contract tests (test_ci_workflows.py, test_run_go_tests.py) that lock in the exact invariants the refactor must preserve. The key risk areas all hold up:
- SIMD consolidation — folding
internal/buildinto one run with-coverpkg=./...is a strict superset of the formerinternal/build,cl,ssacoverpkg, so compiler coverage is preserved (and broadened) while the second filtered run is removed. -covermode=set— sound for these non--racejobs; the covered-statement set Codecov consumes is identical. (Keepsetand-raceapart if-raceis ever added to these lines, sincesetcounters aren't goroutine-safe.)- Matrix restructuring —
windows-gnu-crosspreserves all 6 host/target pairs andembedded-debugkeeps both backends observable (gdb/lldb as separate steps gated on!cancelled() && steps.compiler.outcome == 'success', nocontinue-on-error), matching the oldfail-fast: falsebehavior. - Windows quarantine —
run_go_tests.pyre-emitsgo test -jsonOutputverbatim, sodev/go_test_windows.sh's narrow golang/go#81238 signature match still works;test_windows_quarantine_still_rejects_unrelated_failures_after_json_renderingcovers this. - Security — no attacker-controllable context is interpolated into any
run:script;ci-gategates on the non-spoofableneeds.prepare.result;use_oidccorrectly disables OIDC for fork PRs;run_go_tests.pyruns its command as an argv list (noshell=True) and HTML-escapes step-summary cells. No concerns.
Two minor inline notes below, both non-blocking.
Worth watching (not inline)
.github/workflows/llgo.ymlmacOSwindows-gnu-crossalllane — collapsing the three macOS cross-targets into one job runs amd64/arm64/386 sequentially under a single 120-min timeout instead of in parallel. The shared host-compiler/llvm-mingw/vcpkg setup is a genuine saving and the per-arch steps still run after an earlier failure, but this is the tightest margin introduced by the PR. Worth confirming the macOSalljob's wall-clock on the first few real runs.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cpunion
force-pushed
the
codex/ci-test-efficiency-20261021
branch
from
October 21, 2026 02:03
24b9468 to
136ca25
Compare
cpunion
changed the title
cpunion
changed the title
Reuse the callback-store fix from c87f706 in PR xgo-dev#2895 without the unrelated linkname changes. Clearing or restoring a C callback must not write a two-word Go funcval over adjacent native storage. Add deterministic adjacent-slot and closure-rejection regressions, plus pointer-width IR checks on Linux, Darwin and all six Windows ABI targets.
cpunion
changed the title
Avoid nested Go callbacks exhausting the JS/Asyncify stack. Keep synchronous host operations for worker RPC and Sync methods, and exercise filesystem GC stress with both single and multiple workers.
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
Collaborator
Author
|
Validation is complete for
The native logging fix preserves live diagnostics, every timing row, failure propagation, and the existing narrow runtime quarantine. The GC patch prepares the collecting thread's CRT signal state before suspending other threads, addressing the heap-lock cycle documented in BDWGC #924. Follow-up validation for b0f7f57: both isolated build shards exit with zero on Linux (799 selected tests), macOS (796), Windows MSVC (768), and Windows MinGW (768). The executor verifies that every selected top-level test reports an outcome exactly once. All four complete Go jobs and the LLGo test matrix pass; patch coverage is 100.00% after all Go reports arrive. Validation is now complete for this head: 118 checks succeeded and the tag-only release job was skipped. All release-artifact validation jobs, including all six Windows packages and WinGet candidate generation, pass. |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
cpunion
changed the title
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.
Reduce compiler-test and CI time while preserving test cases, host/target pairs, and coverage. LLGo and test binaries continue to build from the checkout under test.
internal/buildtests into two processes inside each existing Go job, with isolated source trees/package caches andGOMAXPROCS=2per process. Discover tests dynamically, verify exactly-once execution, retain every case duration, and union set-mode coverage, including helper processes. Preserve Windows overlays and linker flags.Completed Go jobs, excluding queueing (single samples on the same runner labels):
c171abfa3793c636b0f7f57cThe four-job total is 32.4% below main. Sources: main, previous, new. Setup/cache variance remains visible, particularly in MSVC; these are observations, not isolated measurements of every change. Across all 12 comparable workflows, total job execution time is 17:34:33 → 16:34:11 → 16:09:51 (main → previous → new): −8.0% vs main, −2.4% vs previous. This excludes the benchmark workflow because PRs measure both base and head while main push measures one revision. Including benchmarks, the two PR runs total 19:24:13 → 19:00:09.
Elapsed time from trigger to the last job, including queueing, is 2:23:25, versus 2:19:19 previously. The macOS artifact job queued for 1:58:29; the unchanged WinGet-candidate job spent 19:48 downloading six current-run archives. These delays and Windows dependency-install variance offset part of the Go savings; end-to-end latency has not improved in this sample.
Coverage and test retention:
Validation: 37 workflow contract tests, three executor tests, actionlint, full local suites, and all four Go CI jobs pass. Native Linux/Windows qualification passes partitioning, coverage union, helper coverage, isolated outputs, Unicode logging, paths/flags with spaces, and failure propagation. All 118 checks pass; the tag-only release job is skipped. Codecov passes. Cold library parallelism remains separate in #2898.