Repository navigation
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: parallelize C/asm compilation within a compile group
The parallelization is well-executed and correct. I looked closely at the concurrency:
- No data races. Each worker writes a distinct, pre-sized
objFiles[index];cfgis copied by value into eachclang.NewCompiler, and its slices are only read (mergeCompilerFlagsappends into fresh slices).next/firstErrare consistently mutex-guarded, andworkers.Wait()establishes the happens-before needed for the archive step. - Source order preserved. Writing at the original index keeps archive member order independent of completion order — relevant for duplicate-symbol resolution — and is covered by the new ordering test.
- Clean lifecycle.
workers.Wait()joins all in-flight compiles before the caller'sdefer os.RemoveAll(tmpCompileDir)runs, so no worker can write into a removed temp dir.count == 0spawns zero workers and returns nil. - Tests are a highlight. The
testing/synctesttests assert the exact worker-pool bounds and failure-join behavior deterministically, without sleeps or wall-clock thresholds.
Two inline notes below (worker cap, interleaved output). Minor/optional items:
- The
objFilescomment explains ordering but not the concurrency invariant (each index is written by exactly one worker, and the backing array is never resized). A one-line note there would keep a future maintainer from "fixing" a perceived concurrent-write race. - The new integration tests hard-depend on
clang/llvm-ar/llvm-nmand will fail (rather thant.Skip) when the toolchain is absent. This matches the existing tests in this package, so it's a pre-existing pattern, not a regression. - Out of scope, as context for the throughput goal: groups are still compiled sequentially, so a config of many small groups never exceeds the per-group cap in aggregate.
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.
Collaborator
Author
|
Added the requested concurrency invariant beside objFiles in 0bf4ad5: each worker owns one distinct index, the backing array is never resized, and the archive reads it only after all workers join. The existing worker cap and per-compile buffered diagnostics already address the other review notes. Validation is complete for 0bf4ad5: 114 checks succeeded. Tag-only publishing and the unchanged-documentation link check were skipped by their workflow conditions. |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
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.
Cold embedded builds compile newlib, picolibc and compiler-rt sources serially. Compile each library with at most
min(GOMAXPROCS, 4)Clang processes, preserving archive member order and unique object names. Stop assigning work after failure, drain active compiles before cleanup, and buffer each compiler's diagnostics.Final CI comparison: main
c171abfaversus this PR's0bf4ad51, excluding queueing:Board sources: main, PR. The two board jobs retain the same matrix and Qiniu runner labels. These are single CI samples with variable setup/cache/download conditions. The full-run total excludes Baseline benchmarks because PRs measure base and head while main push measures one revision; Installer shells did not run for this PR. Board builds improve, but overall CI execution time has not improved in this sample. Combined gains with #2896 remain unmeasured.
Local cold ESP32 + ESP32-C3 library builds (macOS arm64, Go 1.27.0, Espressif Clang 22.1.4,
GOMAXPROCS=4): serial samples 102.14s / 105.53s, final buffered parallel samples 44.85s / 34.02s. All four runs compile the same 2,447 sources with identical normalized arguments. Parallel builds use more simultaneous CPU and memory; cached libraries bypass this work.The macOS core suite also exposed a native callback store overwriting adjacent C memory. Include the focused one-pointer store fix from #2895/#2896, with adjacent-slot, closure-rejection, and eight-target IR regressions.
Validation: macOS full crosscompile race suite and Linux amd64 compile race suite; failure draining/cleanup, archive order, duplicate basenames, and archive linking/execution; callback tests repeated 20 times on macOS and eight-target IR checks. No existing tests or matrix entries are removed. Final CI: 114 checks passed, two conditionally skipped.