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.
LLGo PR #2909 Review — Final WebAssembly size optimization
Reviewed the full diff: new -wasm-opt/-wasm-strip flags, the Binaryen-backed finalize pipeline (wasm_final.go), the artifact manifest + staged publish with rollback (wasm_manifest.go), the Wasm metadata strip/parse helpers (wasmdebug/metadata.go), and the wiring, docs, CI, and tests.
Overall this is high-quality, well-structured work. The staging-directory design keeps the real output untouched until every step (optimize → validate → PCLN → debug packaging → strip → manifest → publish) succeeds, the rollback logic is correct, and the test suite is unusually thorough — covering missing/wrong-version tool, link/optimizer failures, strip-only encoding preservation, non-file obstructions, and publish rollback. I verified the parsing path (readULEB32/readName/parse/TargetFeatures): ULEB shift/overflow bounds and uint64 length checks are correct, with no out-of-bounds or over-allocation on malformed modules. Documentation in doc/wasm-size.md matches the implementation on every claim I checked (defaults, skip-pass list, EXPORT_ALL=0, manifest fields, publish semantics, links).
The findings below are maintainability/quality suggestions, not defects — nothing blocking.
Minor notes not inlined:
-wasm-opt/-wasm-stripvalid-value sets are enforced in two places (cmd/internal/flags/wasm.goandvalidateWasmFinalOptions); adding a third mode means editing both. A shared source would reduce drift. Relatedly, "off" has two representations (""when the flag is omitted vs"off"when passed explicitly); the manifest normalizes this, so it's harmless today but a small latent trap.wasmArtifactgzips every staged artifact at level 9 (incl. potentially large.debug.wasm) purely to recordgzip_bytes. Reasonable as a feature cost, but it's the heaviest step in finalize for large modules — worth a conscious decision if manifests are produced on every build.- Security posture is clean under the local-build trust model: tool args are a fixed
[]string(no shell), feature names passed to--enable-*come from the hardcoded allowlist (never from parsed module bytes), and no secrets/env values are written to the manifest. The recorded tool SHA-256 is logged but not enforced against a pinned value — fine if the version check is considered sufficient.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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
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.
Adds opt-in
-wasm-opt=sizeand-wasm-strip=none|debug|allfor hosted Wasm executables. Final optimization runs after required lowering and before DWARF packaging, with explicit target features and preserved host exports. Outputs are staged with rollback and a manifest containing tool identities, artifact hashes, and raw/gzip sizes. Defaults and development-only optimization gates are unchanged.With
-wasm-opt=size -wasm-strip=debug, defaultfmtprintfdeployment sizes are:Includes JS, filesystem glue and the new manifest; no
-s, DCE or LTO. Baseline:057851875, excluding #2908. Full measurements and contracts include compression overhead and the Binaryen 132 DWARF pass workaround.Validation: full platform CI passed, including final-pipeline runtime/browser and DWARF coverage. Local checks also executed 18 size cases, W32/J32/J64 embedded/external DWARF source-line tests, J32/J64 browser reflection and worker FS/GC, and callback/ABI/public-export fixtures.
Part of #2480 and #2679. Stable frame IDs/external PCLN and further runtime/formatting reduction remain.