fix(web-client): cache the precompile prover preprocessed table in the ST wasm build - #319
Open
WiktorStarczewski wants to merge 6 commits into
Open
WiktorStarczewski wants to merge 6 commits into
WiktorStarczewski wants to merge 6 commits into
Conversation
F-001 P1 the rc.3 heading was renamed rather than superseded, reattributing
five shipped entries to the unreleased rc.4 and erasing rc.3
F-002 P1 the new bench script's `/index.js` import failed the knip CI job
F-003 P1 unvalidated bench flags exited 0 having measured nothing, or crashed
in the summary after completing every proof
F-004 P1 any variant but exactly "ecdsa" silently benchmarked the Falcon
control while keeping the caller's label
F-005 P2 release_wasm paths filter still missed manifests that change the
shipped WASM without moving Cargo.lock
F-006 P2 bench server leaked stream errors, lost post-startup server errors,
and let teardown mask the benchmark's own failure
F-007 P2 manifest comment understated the std flip: it reaches nine more
crates and links parking_lot into the no-atomics ST build
F-009 P2 dropped the unmeasured "bundle size is unchanged" claim
F-012 P3 page errors were logged but still produced a recorded result
F-018 P2 add `make check-wasm-features`: nothing failed if the std feature
silently came back off, since the change's only effect is latency.
Asserts the ST wasm graph resolves exactly one
miden-precompiles-prover and that it carries `std`. Verified to fail
against the pre-fix tree, not just to pass against this one.
F-020 P1 the changelog published "~0.3-0.4s (~6-8%)" from a cold-vs-warm
measurement that cannot separate the cache from first-prove warm-up.
A controlled before/after build shows warm ECDSA proofs at 5009ms vs
4989ms and 4836ms vs 4988ms across two runs — inside run-to-run
variance — and the Falcon control, which raises no precompile claim,
shows the same cold-warm delta. Replaced with a device-dependent
statement and the measured memory cost.
F-014 P2 document the retained cost: ~50 MiB per proving hash function, one
slot in practice, with no eviction API
F-019 P2 wire the benchmark into a `bench-precompile-cache` Make target; it
was referenced from nothing
F-016 P3 close keep-alive sockets so bench teardown cannot stall
…en the feature guard
F-023 P2 manifest and CHANGELOG named Blake3_256 as the cached slot for the
common case; the zero-config submitNewTransaction path proves with
None and falls through to miden-client's LocalTransactionProver::
default(), which is ProvingOptions::new(Poseidon2). Blake3 is only
the explicit newLocalProver() path, so an app doing both retains
two caches (~100 MiB), not one
F-024 P2 check-wasm-features called plain `cargo`, which resolves to the
rust-toolchain.toml pin and would make CI download a second
toolchain the clippy-wasm job does not otherwise install
F-025 P2 per-iteration context.close() could throw and replace the error the
iteration was already unwinding with
F-026 P2 a misspelled flag was silently ignored, so the bench reported the
default config as if it were what was asked for
F-027 P2 bench header still presented same-instance cold-vs-warm as the
cache measurement, contradicting the Makefile and CHANGELOG
F-028 P2 CHANGELOG's "per proving hash function" read as up to five caches
F-029 P3 workspace stanza comment claimed to flip `std`; it does not
F-030 P3 `|| exit 1` was dead after a command substitution
F-031 P3 miden-prover/std vs miden-processor/std attribution was imprecise
and omitted that MT already ships both
F-032 P3 dropped a speculative "matters most on slower hardware" claim
F-034 P3 guard now checks the real ST feature set, and the bench fails fast
on a missing dist instead of after a ten-second browser launch
F-037 P3 path-filter comment implied js-export-macro compiles into the WASM
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.
The ST wasm build resolved
miden-precompiles-proverwithoutstd, so every prove/verify that raises a precompile claim (e.g. an ECDSA-authenticated tx) rebuilt the preprocessed STARK bundle from scratch; a feature-only direct dep now flipsstdon, so the ST build caches it like the MT build already does. Closes #318.cargo treebefore/after diff empty);miden-processorstays withoutstd, so the threaded trace-build wasm trap stays uncompiled.TransactionProver.newLocalProver()), so ~100 MiB worst case. Disclosed in the CHANGELOG. Wasm linear memory never shrinks, so the pre-fix per-prove transient build already grew the heap by the same ~50 MiB and browsers never returned it; only an app exercising both prover paths can see a higher peak than before.make check-wasm-featuresguard (run inmake lintand the clippy-wasm CI job) asserts the ST graph resolves exactly onemiden-precompiles-proverand that it carriesstd— the fix's only observable effect is latency, so without this a dep bump could silently regress it.scripts/bench-precompile-cache.mjsreproduces the measurement (in-browser mock chain, no network); not wired into CI.release_wasmpaths filter now includes the manifests, so a features-only Cargo.toml edit can't skip the release verify job.Measurement (headless Chromium, Apple-silicon Mac, full-optimization ST builds, 3 pages × 3 proves)
Before, cold≈warm — every proof pays the rebuild (the 322ms is generic JIT/memory warmup; the Falcon control shows the same shape). After, warm proofs additionally skip the table build. Tracing spans agree: the precompile-session
prove_starkdrops 3064ms → ~2660ms on warm proofs after (−400ms) vs −200ms before, and thebuild_session/build_tracespans — present only when a claim is raised — confirm the ECDSA tx exercises the precompile path while Falcon runs never emit them.Repro:
node crates/web-client/scripts/bench-precompile-cache.mjs <dist-st-dir> [--spans].Binary verification (`strings -a` counts, raw release wasm)
OnceLockcached_preprocessed!hash configparking_lotstdchainpreprocessed_cacheexecute_and_build_trace_syncstdstays off (acceptance criterion 4)ace_codegenThe issue's
ace_codegenproxy stays 0:mod aceispub(crate)and nothing on the wasm path calls it, so the linker DCEs it even with the feature on. The OnceLock/parking_lot markers plus the warm-proof drop are the std-is-on evidence instead (the criterion's alternate wording).Safety: why `miden-precompiles-prover/std` and not `miden-prover/std`
miden-prover/stdimpliesmiden-processor/std, which compilesexecute_and_build_trace_sync(std::thread::scope) — the known wasm trap.miden-precompiles-proverhas no processor/prover dependency, so itsstdcannot arm it. Everything it enables transitively (miden-ace-codegen,miden-air/std,miden-core/std→parking_lot, …) is already live in the shipped MT artifact. Cargo.lock diff is the single dep edge; no version moved.Full chromium suite green against the fixed dist (266 passed). The one failure,
no_wasm_reentry_via_tojson, fails identically on an unmodifiednextbuild locally and is green onnext's CI — pre-existing local-env quirk, unrelated.Upstream option 2 (cache in the
no_stdbranch too, viamiden-utils-sync::OnceLockCompat) remains open and complementary — this PR doesn't block it.Reviewers: the measurement table and the retained-memory trade-off are the parts worth your time — the win is real but modest (~0.3–0.4s per repeat proof at zero size cost); if you judge that not worth a dependency edge, closing per the issue's "negligible → say so" clause is a legitimate call.