wasmi majit tier: integrate patched wasmi + TLS elimination (#352)#442
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughThis PR makes two unrelated changes: it tightens the JIT's expanded-tail heap-read decision to avoid aliasing when identity-ref state fields are present, and adds a new pyre/wasm_check.py script that benchmarks pyre's wasm output across wasmtime and wasmi engines, checking correctness and reporting timing/speedup. ChangesExpanded-tail aliasing guard
Cross-engine wasm benchmark/correctness script
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant wasm_check
participant pyre_wasm_runner
participant Engines as wasmtime/wasmi engines
User->>wasm_check: run wasm_check.py [filters] [--reps] [--timeout]
wasm_check->>wasm_check: find_runner()
wasm_check->>wasm_check: collect_benches(filters)
loop for each benchmark
loop for each engine
wasm_check->>pyre_wasm_runner: run_timed(bench, engine env, timeout)
pyre_wasm_runner->>Engines: execute benchmark
Engines-->>pyre_wasm_runner: stdout, cpu_time, returncode
pyre_wasm_runner-->>wasm_check: best result per engine
end
wasm_check->>wasm_check: compare stdout across engines, mark correctness
wasm_check->>User: print result row (timings, speedup, status)
end
wasm_check->>User: exit(1 if any incorrect else 0)
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45fa6bc749
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # in-tree patched wasmi (nested workspace) with the `majit-jit` meta-tracing | ||
| # tier so the runner can execute the pyre module under the accelerated | ||
| # interpreter; the tier is on by default and toggled off with `WASMI_NO_MAJIT=1`. | ||
| wasmi = { path = "../../wasmi/crates/wasmi", features = ["majit-jit"] } |
There was a problem hiding this comment.
This manifest now path-depends on ../../wasmi/crates/wasmi, but the reviewed commit does not add a wasmi/ directory or a .gitmodules entry, so a normal checkout has no package at that path. Any workflow that builds the wasm runner, including pyre/check.py --backend wasm and cargo build -p pyre-wasm-runner, will fail during dependency resolution before compiling; please vendor/add the nested workspace or point at a dependency that exists in the repo.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit a631c67). 1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 655419cec4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| WASM_BENCH_CONFIGS = [ | ||
| ("wasmtime", "wasmtime", None), | ||
| ("wasmi-stock", "wasmi", "1"), | ||
| ("wasmi+majit", "wasmi", None), |
There was a problem hiding this comment.
Fresh evidence in the current tree is that this wasmi+majit row does not enable a different engine: pyre/pyre-wasm-runner/Cargo.toml still depends on crates.io wasmi = "1.1", and a repo-wide search finds no majit-jit feature/path dependency or wasmi/ workspace. As a result, --wasm-bench reports a majit speedup by comparing two invocations of the same stock wasmi binary (WASMI_NO_MAJIT is ignored by that dependency), which can corrupt the performance data this flag was added to collect; wire the patched dependency/feature before advertising this row.
Useful? React with 👍 / 👎.
| else: | ||
| spd = "-" | ||
| correct = green("yes") if ok else red("NO") | ||
| any_wrong = any_wrong or not ok |
There was a problem hiding this comment.
When any engine returns nonzero or times out but at least one other engine succeeds, the correctness loop skips the failed output and this line keeps any_wrong driven only by output mismatches, so --wasm-bench can print CRASH(...)/TIMEOUT yet still exit 0. That makes the benchmark pass in exactly the failure modes it should surface; include nonzero return codes in the failure condition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe6ecd8e97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ENGINES = [ | ||
| ("wasmtime", "wasmtime", None), | ||
| ("wasmi-stock", "wasmi", "1"), | ||
| ("wasmi+majit", "wasmi", None), |
There was a problem hiding this comment.
Fresh evidence is that the new standalone script reintroduces the wasmi+majit row, but it still launches the same runner with PYRE_WASM_ENGINE=wasmi and only removes WASMI_NO_MAJIT; I checked pyre/pyre-wasm-runner/Cargo.toml:23 and a repo-wide search for majit-jit/WASMI_NO_MAJIT, and the runner depends on stock crates.io wasmi = "1.1", which has no majit tier to toggle. As a result, the reported majit spd compares two stock wasmi runs and can publish meaningless performance data until this row is wired to a patched dependency/feature.
Useful? React with 👍 / 👎.
| else: | ||
| spd = "-" | ||
| correct = green("yes") if ok else red("NO") | ||
| any_wrong = any_wrong or not ok |
There was a problem hiding this comment.
Fresh evidence is that this failure mode now exists in pyre/wasm_check.py: the correctness loop skips engines with nonzero status, and this line only records stdout mismatches among successful engines. If wasmi+majit crashes or times out while wasmtime succeeds, the table prints CRASH(...)/TIMEOUT but any_wrong stays false and the script exits 0, so benchmark automation can pass exactly the engine failures this tool is meant to catch.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@majit/majit-macros/src/jit_interp/jitcode_lower/lower_value.rs`:
- Around line 1466-1509: The fallback in lower_native_int_binop_call is masking
an invariant violation: once the call is recognized as a native int binop alias
and both operands have been lowered, returning None causes lower_call_value to
lower lhs and rhs again, duplicating any side effects. Tighten this path by
treating the non-int operand case as unreachable for this recognized alias and
fail fast instead of falling back, keeping the behavior localized to
lower_native_int_binop_call and its interaction with lower_call_value.
In `@majit/majit-metainterp/src/lib.rs`:
- Around line 169-270: The boolean env-var helpers are duplicated across many
near-identical functions, so refactor them into a shared macro while preserving
the current caching behavior. Introduce a small `macro_rules!` near the existing
`closedbg_enabled`, `bh_debug_enabled`, `nbody_debug_enabled`,
`mptrace_enabled`, `pcseq_enabled`, `tldbg_enabled`, `heapdbg_enabled`,
`diag_enabled`, `log_jtet_enabled`, `smallir_enabled`, `log_opt_enabled`,
`bridge_debug_enabled`, and `no_unroll_enabled` symbols, and use it to generate
each `OnceLock<bool>` helper from just the function name and env-var string.
Keep `original_boxes_enabled`, `stall_window`, and `step_limit` as-is unless
they can also be cleanly expressed through the same pattern.
In `@majit/majit-translate/src/annotator/bookkeeper.rs`:
- Around line 2170-2188: Add unit tests for the new enum-base bare-leaf redirect
in bookkeeper’s enum handling: exercise reg.is_enum_base with strip_generic_args
and canonical_struct_name to confirm two generic instantiations of the same enum
converge on the shared bare leaf/base at a phi merge, and also verify a
qualified non-generic enum path is not incorrectly redirected. Use the relevant
bookkeeper logic around seen, strip_generic_args, and canonical_struct_name so
the tests lock in the intended base selection behavior.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch.rs`:
- Around line 1284-1291: The new FRAME_SHAPE_DECLINE_SEEN BTreeSet and the
covered HashSet in get_list_of_active_boxes introduce Rust-native dedup
containers that don’t obviously match the RPython/PyPy shape. Review these
locations in jitcode_dispatch.rs and either replace them with the closest
original data-structure pattern from the ported code or add a brief comment
citing the RPython source and explaining why the container is the minimal
borrow-checker/dedup deviation. Keep the change localized to
FRAME_SHAPE_DECLINE_SEEN and get_list_of_active_boxes/covered.
In `@pyre/wasm_check.py`:
- Around line 87-98: `collect_benches` only scans the top-level benchmark
directory, so the new `synth` benchmarks are missed. Update `collect_benches` in
`wasm_check.py` to search recursively from `BENCH_DIR` (or explicitly include
the `synth` subdirectory) while preserving the existing tmp_ exclusion and
filter-by-stem behavior, so the new benchmark files are returned along with the
existing ones.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c97272da-6101-4c37-847f-9c2f00e874c6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (88)
.github/workflows/pyre-ci.ymlCargo.tomlmajit/examples/spcount/Cargo.tomlmajit/examples/spcount/src/main.rsmajit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend-dynasm/src/aarch64/assembler.rsmajit/majit-backend-dynasm/src/runner.rsmajit/majit-backend-dynasm/src/x86/assembler.rsmajit/majit-backend-wasm/src/codegen.rsmajit/majit-backend-wasm/src/lib.rsmajit/majit-backend/src/model.rsmajit/majit-gc/src/collector.rsmajit/majit-gc/src/lib.rsmajit/majit-ir/src/eval_breaker.rsmajit/majit-ir/src/lib.rsmajit/majit-ir/src/resoperation.rsmajit/majit-macros/src/jit_interp/codegen_state.rsmajit/majit-macros/src/jit_interp/codegen_trace.rsmajit/majit-macros/src/jit_interp/jitcode_lower/lower_stmt.rsmajit/majit-macros/src/jit_interp/jitcode_lower/lower_value.rsmajit/majit-macros/src/jit_interp/jitcode_lower/mod.rsmajit/majit-macros/src/jit_interp/mod.rsmajit/majit-metainterp/src/blackhole.rsmajit/majit-metainterp/src/jit_state.rsmajit/majit-metainterp/src/jitcode/assembler.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/lib.rsmajit/majit-metainterp/src/optimizeopt/info.rsmajit/majit-metainterp/src/optimizeopt/mod.rsmajit/majit-metainterp/src/optimizeopt/optimizer.rsmajit/majit-metainterp/src/optimizeopt/rewrite.rsmajit/majit-metainterp/src/optimizeopt/unroll.rsmajit/majit-metainterp/src/optimizeopt/virtualstate.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rsmajit/majit-metainterp/src/resume.rsmajit/majit-translate/src/annotator/bookkeeper.rsmajit/majit-translate/src/annotator/unaryop.rsmajit/majit-translate/src/front/mir.rsmajit/majit-translate/src/front/option_closure_select.rsmajit/majit-translate/src/front/option_map_or.rsmajit/majit-translate/src/lib.rsmajit/majit-translate/src/local_crates.rsmajit/majit-translate/src/translator/rtyper/cutover.rsmajit/majit-translate/src/translator/rtyper/flowspace_adapter.rsmajit/majit-translate/src/translator/rtyper/lltypesystem/rordereddict.rsmajit/majit-translate/src/translator/rtyper/pyre_call_registry.rsmajit/majit-translate/src/translator/rtyper/rclass.rsmajit/majit-translate/src/translator/rtyper/rlist.rsmajit/majit-translate/src/translator/rtyper/rmodel.rsmajit/majit-translate/src/translator/rtyper/rtyper.rsmajit/majit-translate/tests/test_aheui_census.rspyre/bench/synth/foriter_in_while.pypyre/bench/synth/kept_stack_deep_var_shortcircuit_mutate.pypyre/bench/synth/nested_foriter_call.pypyre/bench/synth/nested_foriter_poly.pypyre/bench/synth/nested_foriter_range.pypyre/pyre-interpreter/src/_pypy_generic_alias.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/jit_fnaddr.rspyre/pyre-interpreter/src/module/_csv/mod.rspyre/pyre-interpreter/src/module/_socket/interp_socket.rspyre/pyre-interpreter/src/module/faulthandler/handler.rspyre/pyre-interpreter/src/module/signal/interp_signal.rspyre/pyre-interpreter/src/module/syslog/syslog.rspyre/pyre-interpreter/src/objspace/descroperation.rspyre/pyre-interpreter/src/pycode.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/helpers.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit-trace/src/walker_frame_ops.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/codewriter.rspyre/pyre-object/src/celldict.rspyre/pyre-object/src/dictmultiobject.rspyre/pyre-object/src/identitydict.rspyre/pyre-object/src/intobject.rspyre/pyre-object/src/iterobject.rspyre/pyre-object/src/listobject.rspyre/pyre-object/src/pyobject.rspyre/pyre-object/src/sliceobject.rspyre/pyre-object/src/typeobject.rspyre/wasm_check.py
| // ── Cached diagnostic env-var helpers ──────────────────────────────── | ||
| // | ||
| // Each env var is read once and cached via OnceLock so hot paths | ||
| // (back-edge, guard-failure, optimizer) never re-acquire the global | ||
| // env lock. | ||
|
|
||
| pub fn closedbg_enabled() -> bool { | ||
| static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| *FLAG.get_or_init(|| std::env::var_os("MAJIT_CLOSEDBG").is_some()) | ||
| } | ||
|
|
||
| pub fn bh_debug_enabled() -> bool { | ||
| static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| *FLAG.get_or_init(|| std::env::var_os("MAJIT_BH_DEBUG").is_some()) | ||
| } | ||
|
|
||
| pub fn nbody_debug_enabled() -> bool { | ||
| static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| *FLAG.get_or_init(|| std::env::var_os("PYRE_NBODY_DEBUG").is_some()) | ||
| } | ||
|
|
||
| pub fn mptrace_enabled() -> bool { | ||
| static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| *FLAG.get_or_init(|| std::env::var_os("MAJIT_MPTRACE").is_some()) | ||
| } | ||
|
|
||
| pub fn pcseq_enabled() -> bool { | ||
| static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| *FLAG.get_or_init(|| std::env::var_os("MAJIT_PCSEQ").is_some()) | ||
| } | ||
|
|
||
| pub fn tldbg_enabled() -> bool { | ||
| static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| *FLAG.get_or_init(|| std::env::var_os("MAJIT_TLDBG").is_some()) | ||
| } | ||
|
|
||
| pub fn heapdbg_enabled() -> bool { | ||
| static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| *FLAG.get_or_init(|| std::env::var_os("MAJIT_HEAPDBG").is_some()) | ||
| } | ||
|
|
||
| pub fn diag_enabled() -> bool { | ||
| static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| *FLAG.get_or_init(|| std::env::var_os("MAJIT_DIAG").is_some()) | ||
| } | ||
|
|
||
| pub fn log_jtet_enabled() -> bool { | ||
| static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| *FLAG.get_or_init(|| std::env::var_os("MAJIT_LOG_JTET").is_some()) | ||
| } | ||
|
|
||
| pub fn smallir_enabled() -> bool { | ||
| static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| *FLAG.get_or_init(|| std::env::var_os("MAJIT_SMALLIR").is_some()) | ||
| } | ||
|
|
||
| pub fn log_opt_enabled() -> bool { | ||
| static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| *FLAG.get_or_init(|| std::env::var_os("MAJIT_LOG_OPT").is_some()) | ||
| } | ||
|
|
||
| pub fn bridge_debug_enabled() -> bool { | ||
| static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| *FLAG.get_or_init(|| std::env::var_os("MAJIT_BRIDGE_DEBUG").is_some()) | ||
| } | ||
|
|
||
| pub fn no_unroll_enabled() -> bool { | ||
| static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| *FLAG.get_or_init(|| std::env::var_os("PYRE_NO_UNROLL").is_some()) | ||
| } | ||
|
|
||
| /// `PYRE_ORIGINAL_BOXES`: default true, only disabled by `0` or `false`. | ||
| pub fn original_boxes_enabled() -> bool { | ||
| static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| *FLAG.get_or_init(|| match std::env::var_os("PYRE_ORIGINAL_BOXES") { | ||
| Some(v) => { | ||
| let v = v.to_string_lossy(); | ||
| v != "0" && !v.eq_ignore_ascii_case("false") | ||
| } | ||
| None => true, | ||
| }) | ||
| } | ||
|
|
||
| pub fn stall_window() -> u64 { | ||
| static VAL: std::sync::LazyLock<u64> = std::sync::LazyLock::new(|| { | ||
| std::env::var("MAJIT_STALL_WINDOW") | ||
| .ok() | ||
| .and_then(|v| v.parse().ok()) | ||
| .unwrap_or(1_000_000) | ||
| }); | ||
| *VAL | ||
| } | ||
|
|
||
| pub fn step_limit() -> u64 { | ||
| static VAL: std::sync::LazyLock<u64> = std::sync::LazyLock::new(|| { | ||
| std::env::var("MAJIT_STEP_LIMIT") | ||
| .ok() | ||
| .and_then(|v| v.parse().ok()) | ||
| .unwrap_or(8_000_000) | ||
| }); | ||
| *VAL | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Thirteen near-identical OnceLock boolean helpers — consolidate with a macro.
closedbg_enabled, bh_debug_enabled, nbody_debug_enabled, mptrace_enabled, pcseq_enabled, tldbg_enabled, heapdbg_enabled, diag_enabled, log_jtet_enabled, smallir_enabled, log_opt_enabled, bridge_debug_enabled, and no_unroll_enabled all repeat the identical static FLAG: OnceLock<bool> = ...; *FLAG.get_or_init(|| std::env::var_os("...").is_some()) pattern, differing only in the function/env-var name. A small macro_rules! would remove the duplication and make adding future flags a one-liner.
♻️ Proposed macro-based consolidation
+macro_rules! cached_env_flag {
+ ($name:ident, $var:literal) => {
+ pub fn $name() -> bool {
+ static FLAG: std::sync::OnceLock<bool> = std::sync::OnceLock::new();
+ *FLAG.get_or_init(|| std::env::var_os($var).is_some())
+ }
+ };
+}
+
+cached_env_flag!(closedbg_enabled, "MAJIT_CLOSEDBG");
+cached_env_flag!(bh_debug_enabled, "MAJIT_BH_DEBUG");
+cached_env_flag!(nbody_debug_enabled, "PYRE_NBODY_DEBUG");
+cached_env_flag!(mptrace_enabled, "MAJIT_MPTRACE");
+cached_env_flag!(pcseq_enabled, "MAJIT_PCSEQ");
+cached_env_flag!(tldbg_enabled, "MAJIT_TLDBG");
+cached_env_flag!(heapdbg_enabled, "MAJIT_HEAPDBG");
+cached_env_flag!(diag_enabled, "MAJIT_DIAG");
+cached_env_flag!(log_jtet_enabled, "MAJIT_LOG_JTET");
+cached_env_flag!(smallir_enabled, "MAJIT_SMALLIR");
+cached_env_flag!(log_opt_enabled, "MAJIT_LOG_OPT");
+cached_env_flag!(bridge_debug_enabled, "MAJIT_BRIDGE_DEBUG");
+cached_env_flag!(no_unroll_enabled, "PYRE_NO_UNROLL");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/majit-metainterp/src/lib.rs` around lines 169 - 270, The boolean
env-var helpers are duplicated across many near-identical functions, so refactor
them into a shared macro while preserving the current caching behavior.
Introduce a small `macro_rules!` near the existing `closedbg_enabled`,
`bh_debug_enabled`, `nbody_debug_enabled`, `mptrace_enabled`, `pcseq_enabled`,
`tldbg_enabled`, `heapdbg_enabled`, `diag_enabled`, `log_jtet_enabled`,
`smallir_enabled`, `log_opt_enabled`, `bridge_debug_enabled`, and
`no_unroll_enabled` symbols, and use it to generate each `OnceLock<bool>` helper
from just the function name and env-var string. Keep `original_boxes_enabled`,
`stall_window`, and `step_limit` as-is unless they can also be cleanly expressed
through the same pattern.
| thread_local! { | ||
| /// Code objects already counted by | ||
| /// [`census_record_frame_shape_decline`], so a `CurrentFrameOnly` code | ||
| /// entered many times (a hot helper) records exactly one census entry | ||
| /// instead of one per call. Keyed by the stable `CodeObject` pointer. | ||
| static FRAME_SHAPE_DECLINE_SEEN: std::cell::RefCell<std::collections::BTreeSet<usize>> = | ||
| const { std::cell::RefCell::new(std::collections::BTreeSet::new()) }; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Verify the newly introduced Rust-native containers against the RPython shape.
This change introduces a BTreeSet<usize> (FRAME_SHAPE_DECLINE_SEEN, Line 1289) and a HashSet<usize> (covered, Line 10244, inside the get_list_of_active_boxes port). The repo convention is to avoid casually introducing HashMap/HashSet/BTreeMap-style containers when porting RPython/PyPy code, preferring the original structure shape, and to cite the RPython original when a container is used as a borrow-checker/dedup workaround.
Both here are pointer/index dedup helpers with no corresponding RPython container. Please confirm they are the minimal deviation (or cite the RPython original in a comment); the covered set in particular sits directly in ported get_list_of_active_boxes logic.
As per coding guidelines: "When porting RPython/PyPy code, do not casually introduce Rust-native containers like HashMap, HashSet, or BTreeMap; match the original RPython data structure shape instead."
Also applies to: 10244-10245
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch.rs` around lines 1284 - 1291, The
new FRAME_SHAPE_DECLINE_SEEN BTreeSet and the covered HashSet in
get_list_of_active_boxes introduce Rust-native dedup containers that don’t
obviously match the RPython/PyPy shape. Review these locations in
jitcode_dispatch.rs and either replace them with the closest original
data-structure pattern from the ported code or add a brief comment citing the
RPython source and explaining why the container is the minimal
borrow-checker/dedup deviation. Keep the change localized to
FRAME_SHAPE_DECLINE_SEEN and get_list_of_active_boxes/covered.
Source: Coding guidelines
| def collect_benches(filters: list[str]) -> list[Path]: | ||
| """Glob pyre/bench/*.py, optionally filtering by substring.""" | ||
| all_scripts = sorted(Path(BENCH_DIR).glob("*.py")) | ||
| # Exclude tmp_ files | ||
| all_scripts = [s for s in all_scripts if not s.name.startswith("tmp_")] | ||
| if not filters: | ||
| return all_scripts | ||
| matched = [] | ||
| for s in all_scripts: | ||
| if any(f in s.stem for f in filters): | ||
| matched.append(s) | ||
| return matched |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
collect_benches won't discover the new pyre/bench/synth/ benchmarks.
Path(BENCH_DIR).glob("*.py") is non-recursive, so the synth benchmarks added in this same PR (foriter_in_while.py, nested_foriter_call.py, etc.) are invisible to wasm_check.py. If these benchmarks are meant to be compared across engines, use a recursive glob or add the synth/ subdirectory.
Proposed fix: recursive glob
- all_scripts = sorted(Path(BENCH_DIR).glob("*.py"))
+ all_scripts = sorted(Path(BENCH_DIR).glob("**/*.py"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def collect_benches(filters: list[str]) -> list[Path]: | |
| """Glob pyre/bench/*.py, optionally filtering by substring.""" | |
| all_scripts = sorted(Path(BENCH_DIR).glob("*.py")) | |
| # Exclude tmp_ files | |
| all_scripts = [s for s in all_scripts if not s.name.startswith("tmp_")] | |
| if not filters: | |
| return all_scripts | |
| matched = [] | |
| for s in all_scripts: | |
| if any(f in s.stem for f in filters): | |
| matched.append(s) | |
| return matched | |
| def collect_benches(filters: list[str]) -> list[Path]: | |
| """Glob pyre/bench/*.py, optionally filtering by substring.""" | |
| all_scripts = sorted(Path(BENCH_DIR).glob("**/*.py")) | |
| # Exclude tmp_ files | |
| all_scripts = [s for s in all_scripts if not s.name.startswith("tmp_")] | |
| if not filters: | |
| return all_scripts | |
| matched = [] | |
| for s in all_scripts: | |
| if any(f in s.stem for f in filters): | |
| matched.append(s) | |
| return matched |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 97-97: Use a list comprehension to create a transformed list
(PERF401)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/wasm_check.py` around lines 87 - 98, `collect_benches` only scans the
top-level benchmark directory, so the new `synth` benchmarks are missed. Update
`collect_benches` in `wasm_check.py` to search recursively from `BENCH_DIR` (or
explicitly include the `synth` subdirectory) while preserving the existing tmp_
exclusion and filter-by-stem behavior, so the new benchmark files are returned
along with the existing ones.
When identity_ref_bank_index is set (state-field JIT), live_values includes scalar state fields that are not counted in num_reds. The comparison live_values.len() >= num_reds + total_vable can produce a false positive, causing initialize_virtualizable to reuse scalar inputarg slots as vable elements instead of minting fresh ones. Gate on identity_ref_bank_index.is_none() so that state-field JIT always takes the heap-read path via vable_ptr. Assisted-by: Claude
Switch wasmi dependency from crates.io 1.1 to in-tree patched wasmi (wasmi/ nested workspace) with the majit-jit meta-tracing feature. - Cargo.toml: exclude wasmi/ from workspace members - pyre-wasm-runner/Cargo.toml: path dependency with majit-jit feature - wasmi_host.rs: adapt to wasmi 2.x API (Nullable import, table-based trace dispatch replacing HashMap, trace_base tracking) Assisted-by: Claude
Add --wasm-bench / --wasm-bench-reps for comparing wasm execution across engines: wasmtime, wasmi stock, and wasmi+majit. Reports a speedup table for loop-heavy benchmarks (int_loop, float_loop, fib_loop, nested_loop, inline_helper, spectral_norm). Assisted-by: Claude
The patched wasmi with majit-jit requires the in-tree wasmi/ nested
workspace which is not available in CI. Revert to the crates.io
wasmi 1.1 dependency so CI builds pass.
For local development with the majit JIT tier, use .cargo/config.toml:
[patch.crates-io]
wasmi = { path = "wasmi/crates/wasmi" }
Assisted-by: Claude
Assisted-by: Claude
Move the --wasm-bench functionality from check.py to a standalone pyre/wasm_check.py that auto-discovers pyre/bench/*.py scripts and compares wasmtime vs wasmi-stock vs wasmi+majit. Revert check.py to its upstream state (no --wasm-bench flag). Usage: python pyre/wasm_check.py [--reps N] [BENCH_SUBSTRING ...] Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1954b2ee95
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| best, out, code = None, None, 0 | ||
| for _ in range(reps): | ||
| o, t, c, _e = run_timed([runner, str(script)], timeout_s=timeout, env=env) |
There was a problem hiding this comment.
When this script is run after the normal wasm build and a later web build, it benchmarks the runner's default target/.../pyre_wasm.wasm instead of the stable host snapshot. pyre/check.py:114-121 documents that pyre_wasm.wasm is shared by the web and wasm-host builds and can be clobbered, and pyre/check.py:286-289 avoids that by setting PYRE_WASM_MODULE to pyre_wasm.wasm-host.wasm; this standalone path never does so, so the comparison can fail or measure the wrong module depending on the last wasm build.
Useful? React with 👍 / 👎.
1 participant
Summary
Integrate our patched wasmi (in-tree
wasmi/workspace, branchtry-majit) with the majit meta-tracing JIT tier into the pyre wasm runner, resolving the blockers identified in #352.Changes
pyre-wasm-runner: wasmi integration
majit-jitfeaturewasmi_host.rsto wasmi 2.x API (table-based trace dispatch,Nullableimport)wasmi/nested workspace from the rootCargo.tomlcheck.py: benchmark tooling
--wasm-bench/--wasm-bench-repsflags for comparing wasm execution across engines (wasmtime, wasmi stock, wasmi+majit)majit:
has_expanded_tailfalse positive fixinitialize_virtualizableinpyjitpl.rs: state-field JIT's scalar state fields were misidentified as expanded vable tail, preventing additional state field additionswasmi tier status (try-majit branch)
#352 blockers resolved
MINI_RETURN_BAILmem_base/mem_len/mem_trap/mem_did_storeas register-promoted state fieldsWASMI_MAJIT_STATS=1Closes #352
— commented by Claude
Summary by CodeRabbit
New Features
Bug Fixes