build: report final Wasm, ELF and PE sizes - #2719
Merged
cpunion merged 12 commits intoOct 4, 2026
Merged
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Size-report rework review
Solid, well-tested change. The ELF interval sweep, LEB128 decoding (signed + unsigned with overflow/bounds checks), and Wasm byte-accounting are correct and backed by strong tests, including a fuzz harness that enforces code + data + custom_bytes + structure_bytes == file_size. Documentation matches the implemented behavior. Findings below are a few cleanups and one design decision worth confirming; nothing blocking.
Design decision to confirm — report failure now aborts a successful build
build.go propagates reportFinalSize errors instead of printing Warning: size report failed, and the doc states this intentionally. The format/level are pre-validated in ensureSizeReporting, but reader/os.Stat/stdout-write failures in the diagnostic path are not — so a transient failure while reporting can now discard the exit status of an otherwise-successful compile (the binary is still on disk, but the command exits non-zero). If that is the intended contract ("a requested report that cannot be produced fails the build"), no change needed; if purely diagnostic I/O failures should not fail a good build, consider distinguishing them.
Performance — symbol attribution is O(symbols x packages) on large binaries
nameResolver.matchModule/matchPackage (internal/build/resolver.go, unchanged here but now exercised per-symbol by both the ELF and Wasm readers) linearly scan every package in link.allPkgs (the full transitive dependency set — often hundreds to thousands) and only cache positive matches. Symbols that match no package (runtime/C/asm/mangled symbols — frequently a large share of a linked binary) re-scan all packages on every call and are never memoized, because "" doubles as both "absent" and the stored value. On a large binary this is the dominant cost. Consider caching misses (sentinel distinct from a real value) and/or using the precomputed pkgPrefixes with a sorted-prefix binary search instead of a per-symbol linear scan. Not blocking, but worth addressing given this PR makes the resolver the hot path.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
cpunion
force-pushed
the
codex/fix-size-report-2679-20261003
branch
from
October 3, 2026 05:29
adad928 to
5c7c7aa
Compare
cpunion
changed the title
Collaborator
Author
|
@fennoai review The size parser, attribution and output now live in the independently tested |
Contributor
Review completeReviewed the latest revision ( All three findings from the prior review are resolved — resolver negative-cache, unbounded The new work (the |
zhouguangyuan0718
approved these changes
Oct 4, 2026
zhouguangyuan0718
left a comment
zhouguangyuan0718
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the final-artifact size accounting and the latest Emscripten EH/cache changes. Focused size-report and build-integration tests passed, including the added EH/cache tests. Checked real Wasm, ELF, and PE artifacts against their file/section sizes. No blocking findings in the reviewed changes. The latest CI run is still in progress.
Contributor
There was a problem hiding this comment.
Follow-up review — sizereport extraction, PE support, Wasm EH
Reviewed the latest revision (783ec4f...dc9a06c2). The three findings from the previous review are all resolved:
- Resolver
O(symbols x packages)— fixed:matchPackage/matchModulenow cache misses (map[...] = ""+ comma-ok lookup), so each distinct symbol scans packages at most once. - Unbounded
io.ReadAll— fixed:Collectrejects non-regular files andreadWasmSizeBytespresizes the buffer to the stat size with a singleio.ReadFull, plus a grown/shrunken-file guard. - Double
CollectArtifacts— fixed:reportBuildOutputscollects one artifact snapshot and shares it between the size report and the artifact listing (covered byTestReportBuildOutputsSharesArtifacts).
The new work is solid and well-tested:
- The
internal/sizereportextraction is clean — stdlib-only, exportedPackage/Artifact/Report, tool invocation (llvm-readelf) kept in the build layer behindErrUnsupportedFormat. - The new PE/COFF reader correctly excludes raw-file alignment padding, models virtual zero-fill as BSS via a shadow-section index, bounds-checks
SectionNumber, and usesuint32-widened arithmetic that cannot overflow. Malformed PE/ELF/Wasm inputs all fail closed (no panics, no unbounded allocation). applyEmscriptenEHFeaturecorrectly replaces (not duplicates) thetarget-featuresattribute, skips declarations, is idempotent, and only materializes cost on Emscripten J32/J64 builds.EMCC_CFLAGScache-key isolation is scoped tojs/wasmand test-verified.- Documentation (
doc/size-report.md,test/simd/README.md) matches the implemented behavior, including the JSON schema, theram = data + zero-filldefinition, and all three validation commands.
Security and documentation passes found nothing. Only two optional micro-nits below; nothing blocking.
cpunion
force-pushed
the
codex/fix-size-report-2679-20261003
branch
from
October 4, 2026 06:47
dc9a06c to
34acf06
Compare
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
2 participants
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.
llgo build -sizemeasures final Wasm, ELF and PE artifacts directly, preserves Go method names, and avoids duplicate ELF symbol ranges. PE reports exclude raw-file alignment padding and distinguish stored data from virtual zero-fill. Mach-O retains its native tool fallback.Move parsing, aggregation and text/JSON output into the standard-library-only
internal/sizereportpackage. The build layer selects final artifacts after post-link processing and supplies package metadata. Requested-report failures propagate to the caller.Part of #2679.
Validation: standalone tests with
CGO_ENABLED=0(97.7% statement coverage), vet, build integration tests, PE32/PE32+ and malformed-artifact cases, real Windows executables, and native/WASI final-artifact smoke tests. PE regressions cover section-relative symbols and zero-fill at a nonzero virtual address. Real-artifact acceptance lives undertest/sizereport.