Repository navigation
feat: Using binaryen to optimize wasm sizes - #314
Conversation
There was a problem hiding this comment.
The main functional addition is scripts/wasm-opt.sh plus wiring it into build.sh/setup.sh/CI. The wasm-opt.sh tool logic (resolution, install, hashing, stamp skip) looks sound, but the way it's invoked unconditionally at the end of build.sh conflicts with its own hard-fail-on-missing-directory behavior for non-release builds.
There was a problem hiding this comment.
Solid, well-documented addition of a wasm-opt post-processing step. The script itself is careful (pinned tool version, deterministic MVP-feature flags, explicit vs default target handling). Two things worth a second look: the opt-stamp cache only keys off the wasm file's own hash, so changing WASM_OPT_FLAGS or bumping the pinned wasm-opt version won't force re-optimization of files that cargo didn't rebuild — stale bytes could silently ship or skew benchmark-gas.sh numbers. Also, the new 'Ensure wasm-opt' step added to the UI/docker job in test.yml sits right after an npm-only setup with no visible Rust/cargo caching in this hunk — if that job has no cargo toolchain and no cached wasm-opt binary, the auto-install path will fail the job.
|
@TimothyBanks Can you take a look at the two bot comments (here and here) and maybe have claude rectify those? |
sappenin
left a comment
There was a problem hiding this comment.
See my main comment about addressing the PR bot's suggestions. Otherwise LGTM
Mutable tag and branch refs can be silently repointed by the action owner, as happened in the trivy-action and kics-github-action compromises. Pin all 30 `uses:` refs across test.yml, docs.yml, and check-pr-description.yml to 40-character commit SHAs, keeping the original tag as a trailing comment so the intended version stays readable and Dependabot can bump both together. The two XRPLF/actions reusable workflows were already SHA-pinned and are unchanged. `taiki-e/install-action@cargo-deny` and `@cargo-llvm-cov` were the most exposed: those are per-tool aliases the maintainer repoints on every release, not version tags. .github/dependabot.yml already declares the `github-actions` ecosystem, so the pins stay maintained with no config change. Each SHA was resolved from the tag it replaces and independently re-verified against the live tag. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: David Fuelling <sappenin@gmail.com>
|
@sappenin Should be addressed with my last commit. |
There was a problem hiding this comment.
This adds a wasm-opt.sh script wired into build.sh/setup.sh/CI to shrink release WASM artifacts with Binaryen. The script is well-structured (tool resolution → install fallback → optimize with a stamp-based skip cache), and the CI/action-pinning changes are standard SHA-pinning. The one real issue is that the stamp file only tracks the wasm binary's content hash, not the flags or wasm-opt version used to produce it, so changing WASM_OPT_FLAGS (or the pinned crate version) between runs without an intervening cargo build will silently skip re-optimization.
There was a problem hiding this comment.
This MR wires binaryen's wasm-opt into the build pipeline (build.sh, setup.sh, CI workflows) via a new scripts/wasm-opt.sh, plus pins GitHub Actions to SHAs. The new script itself (idempotency via hash+flags stamp, mandatory-tool vs no-op-when-nothing-to-optimize, MVP-feature flag rationale) is well thought out and matches the accompanying documentation. The main gap is that tool resolution never verifies the found binary's version against the pinned WASM_OPT_CRATE_VERSION, which combined with CI's cache key (based only on Cargo.lock, not on the wasm-opt pin) means a future pin bump can silently keep using a stale cached binary in CI.
Two review findings on the wasm-opt integration. Verify a cargo-managed wasm-opt against the pin. Tool resolution accepted whatever binary it found. CI caches ~/.cargo/bin under a key derived from Cargo.lock, which bumping WASM_OPT_CRATE_VERSION in this script does not change, so a future pin bump would silently keep using the stale cached binary and the pin would be a lie. A wasm-opt at $CARGO_HOME/bin is now version-checked against the pin, reinstalled on mismatch, and re-checked afterwards so a bad install fails loudly instead of proceeding with the wrong optimizer. An explicit $WASM_OPT and a system Binaryen stay exempt, as documented. Remove the stamp cache. The stamp only hashed the wasm bytes, so editing WASM_OPT_FLAGS or moving to a different wasm-opt silently skipped re-optimization. Rather than extend the stamp to cover both, drop it: writing in place breaks the hardlink cargo makes from deps/, so cargo re-uplifts the unoptimized artifact on every build and the cache could never legitimately hit. It was buying ~0.7s that never materialized while owning a real correctness bug. Each build.sh run is now exactly one pass over pristine cargo output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
This is a well-engineered change: the new scripts/wasm-opt.sh is carefully written with sensible fallback/pin-verification logic, and the CI/doc updates (action SHA-pinning, wasm-opt wiring into build.sh/docs.sh/setup.sh, and the AGENTS.md rationale) are internally consistent with the script's actual behavior. I didn't find correctness, security, or resource-leak bugs in the added code. The only note is a minor, purely cosmetic log-message issue in build.sh.
There was a problem hiding this comment.
@TimothyBanks I don't think the e2e tests are failing in main (but I can't seem to find a recent build to confirm that). The e2e-tests are passing locally for me in main, FWIW.
Can you look at that one before merging?
There was a problem hiding this comment.
This change wires up Binaryen's wasm-opt into the build pipeline (build.sh, setup.sh, CI workflows) via a new scripts/wasm-opt.sh, plus pins GitHub Actions to commit SHAs and documents the new behavior extensively in AGENTS.md/README. The new script is carefully engineered — it guards against div-by-zero in the size-delta calc, correctly exempts user-set/native wasm-opt from the version-pin check via a -ef identity test, handles the no-release-WASM-found case distinctly from an explicitly-named-but-missing target, and the mandatory-tool / in-place-write tradeoffs are well justified in AGENTS.md. I did not find any high-confidence correctness or security bugs introduced by this diff; the CI action SHA pins can't be verified against upstream tags without network access, so I'm not flagging them per the project's own guidance to avoid unverifiable configuration claims.
There was a problem hiding this comment.
This PR adds scripts/wasm-opt.sh (a Binaryen wasm-opt wrapper that shrinks release WASM artifacts in place), wires it into build.sh/setup.sh/CI, and pins third-party GitHub Actions to full commit SHAs. The new shell script is unusually well-engineered: it handles tool resolution order (explicit WASM_OPT > PATH > cargo bin), version-pins the cargo-installed binary while exempting user-supplied/system binaries, guards against div-by-zero in the size-delta calculation, cleans up temp files on failure, and distinguishes 'no release WASM to optimize' (exit 0) from 'explicitly named target missing' (exit 1). I traced the control flow (resolve_wasm_opt, the pipefail/head interaction in binaryen_version_of, the -e semantics around the if-blocks in install_pinned_wasm_opt) and didn't find a case that produces wrong behavior. The AGENTS.md documentation additions are detailed and internally consistent with the script and CI wiring (e.g., --ensure-tool calls in setup.sh/docs.yml/test.yml, SKIP_WASM_OPT/WASM_OPT_NO_INSTALL env vars, cache-staleness handling for version bumps). The GitHub Actions SHA-pinning changes look like good practice; I can't independently verify each 40-char SHA corresponds to the stated tag, so I'm not flagging those without evidence of a mismatch. No correctness, security, or resource-leak issues found in the added code that meet the bar for confident flagging.
|
@sappenin Fixed |
High Level Overview of Change
Hooks up binaryen wasm-opt during the build process to optimize wasm binary sizes.
From e2e tests:
Context of Change
Type of Change