Repository navigation
Use FxHashMap for the VM's per-op caches - #5431
Conversation
The LoadType caches (interpreter and compiled code) and the static virtual-call cache are looked up on every LoadType and VirtualCall. Their keys are function addresses, constant indexes, call sites and program types, never program input, so the DoS-resistant SipHash buys nothing there. impl_cache.rs already uses rustc_hash.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Layer / File(s) | Summary |
|---|---|
Cache types and initialization baml_language/crates/bex_vm/src/vm.rs, baml_language/crates/bex_vm/src/vm/compiled_rt/state.rs |
The VM’s static type and virtual-call caches and the compiled runtime’s static type cache now use FxHashMap. Their test and runtime initialization uses FxHashMap::default(). Cache lookup and dispatch behavior are unchanged. |
Priority:
Estimated code review effort: 2 (Simple) | ~7 minutes
Change: Refactor
Possibly related PRs
- BoundaryML/baml#5423: Changes the same VM caches to weak references and adds garbage-collection handling.
- BoundaryML/baml#5264: Introduces the compiled-runtime caches that this change updates.
Suggested reviewers: harivansh-afk
Merge Risk: 
243ab
-
The cache representation change is mergeable; normal build and test validation remain appropriate.
Security architecture risk: 
243ab
-
The change does not expand permissions or public interfaces, and ordinary runtime payloads do not become cache keys. No concrete security regression was established. Exposure to adversarial program or type definitions remains deployment-dependent.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The direct changed surface is cache lookup behavior within a VM instance. The comparison establishes no additional service, tenant, credential, or infrastructure authority.
Security Findings and Attack Paths
- inferred — No direct runtime-payload-to-cache-key attack path was established. Program and type identities remain relevant inputs, so this conclusion does not establish collision resistance when adversaries control complete program definitions.
Trust Boundaries and Controls
- observed — Static virtual-cache insertion remains restricted to static implementations. Runtime package and registered rules continue through the existing dynamic resolver rather than becoming newly eligible for static caching.
Resilience and Maintainability Implications
- observed — The compiled cache still allocates before publishing an entry and preserves rooted values across suspension and GC. Source tests cover repeated yield, collection, and resume; those tests were inspected, not executed. Neither revision exposes a recoverable allocation-failure branch in this path.
-
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: replacing standard hash maps with FxHashMap for the VM's per-operation caches. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 1 files. (1 skipped: 1 too large.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption.
A rabbit checks the cache at dawn,
Where FxHashMap now hops along.
The keys stay put, the calls stay true,
The defaults take their places too.
Then off the rabbit bounds, content.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Binary size checks failed❌ 2 violations · ✅ 1 passed
Details & how to fixViolations:
Add/update baselines:
[artifacts.bridge_wasm]
file_bytes = 27531416
gzip_bytes = 8086041
[artifacts.packed-program]
file_bytes = 41387112
gzip_bytes = 16862811Generated by |
`join` was written in BAML. A rest pattern copied the array, then every element went through a generic `string.from` call and a `+` that built a rope node. It is now a native that appends into one buffer. A string element is appended as is. Any other element is rendered exactly as `string.from` renders it, through the override-honoring walker, one element at a time. So `ToString` overrides still apply at every depth and run in element order, as in the BAML loop. The separator semantics are unchanged: an empty array joins to `""`, one element to its own rendering. New tests pin those cases plus override classes, structural classes, a structural class holding an override, enums, maps, nested lists, and a garbage collection inside an override. The 12-hole `join` below joins the speedtest suite as `string::join-request-line-10k`. This is the top of the stack (BoundaryML#5431 → BoundaryML#5432 → this PR), so the table also compares with canary before any of the three. | workload | canary | PR 2 | this PR | vs PR 2 | vs canary | |---|---:|---:|---:|---:|---:| | 12-hole request line, `join` | 513.14 | 432.20 | 22.87 | 18.9× faster | 22.4× faster | | `[a, "/", b].join("")` | 80.55 | 70.48 | 7.70 | 9.2× faster | 10.5× faster | | 12-hole request line, template | 117.77 | 30.27 | 31.02 | +2% | 3.8× faster | | `` `${a}/${b}` `` | 20.06 | 4.17 | 4.33 | +4% | 4.6× faster | | `interfaces::to-string-interpolation-10k` | 51.39 | 32.89 | 33.34 | +1% | −35% | | repro `` `${a}${b}` `` (ns/op) | 1,563 | 100 | 98 | −1% | 15.9× faster | | repro `` `/v1/items/${a}/tags/${b}` `` (ns/op) | 1,703 | 104 | 104 | −1% | 16.5× faster | | repro `` `n=${k}` `` (ns/op) | 764 | 244 | 230 | −6% | 3.3× faster | | repro `a.to_string() + b.to_string()` (ns/op) | 826 | 454 | 432 | −5% | −48% | | repro loop `` out = `${out}${c}` `` (ns/op) | 2,508 | 540 | 556 | +3% | 4.5× faster | | repro `a + b` (ns/op, control) | 114 | 114 | 113 | −1% | −1% | | loop, no string built (control) | 2.43 | 2.46 | 2.64 | +7% | +9% | | `a + "/" + b` (control) | 4.07 | 4.09 | 4.37 | +7% | +7% | | `programs::richards-50` (control) | 510.13 | 496.96 | 515.00 | +4% | +1% | | `compute::generic-apply-inferred-1m` (control) | 527.90 | 492.42 | 530.13 | +8% | 0% | | `compute::fib32-recursive` (control) | 744.02 | 739.12 | 740.95 | 0% | 0% | | `compute::pure-call-1m` (control) | 115.37 | 115.58 | 116.12 | 0% | +1% | | `string::trim-padded-literal-100k` (control) | 33.89 | 33.93 | 35.77 | +5% | +6% | The controls that move run no `join` and no template. Against PR 2 they execute the same number of instructions (`perf stat`: `richards-50` 18.3266 vs 18.3265 billion, `generic-apply` 18.43 vs 18.43 billion). Only front-end stall cycles grow (`richards-50` 415M → 685M). That is code layout in the fat-LTO binary, not extra work. The same controls also swing ±3% between canary, PR 1 and PR 2, which don't touch them either. ms per job (median of 5) unless marked ns/op (median of 10). AMD Ryzen 9 5950X, Linux 7.0, release builds (fat LTO, rustc 1.98.0), `speedtest` VM row, telemetry off, pinned to CPU 7, canary and the three stacked PRs interleaved in 5 rounds. Rows without a `::` name are temporary workloads that build one string per iteration, 10,000 iterations per job. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **New Features** * Array joining now honors custom string representations for values, including nested values, while preserving element order and separators. * Joining supports empty and single-element arrays, primitive values, enums, classes, unions, and generic calls. * **Bug Fixes** * Improved consistency when custom string representations run during garbage collection. * **Tests** * Expanded coverage for joining behavior and added a performance workload for large batches of request-line strings. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
The VM looks up three caches on every
LoadTypeandVirtualCall: theLoadTypecache of the interpreter, the one of compiled code, and the static virtual-call cache. They used stdHashMap, which hashes with SipHash. They now useFxHashMap, likeimpl_cache.rsalready does.SipHash protects a map from keys an attacker picks. These keys are function addresses, constant indexes, call sites, type tags and program types. Program input never becomes a key, so Fx is safe here. The VM's other SipHash maps are not per-op caches: the structural
==and hashing drivers use per-call scratch sets of heap pointers, and the rest are keyed by names and used outside the op loop. They stay as they are.interfaces::polymorphic-dispatch-100kinterfaces::match-dispatch-100kinterfaces::default-method-100kinterfaces::to-string-interpolation-10kcompute::generic-apply-inferred-1mprograms::richards-50interp::long-plus(12-hole line,+andto_string())interp::long(12-hole line, template)interp::string(`${a}/${b}`)interp::int(`id=${k}`)string::concat-loop-10kstring::split-short-literal-100kcompute::fib32-recursivecompute::pure-call-1mclasses::method-call-100kms per job, median. AMD Ryzen 9 5950X, Linux 7.0, release builds (fat LTO, rustc 1.98.0),
speedtestVM row, telemetry off, pinned to CPU 7, 5 interleaved rounds.interp::*are temporary workloads from the interpolation investigation, each building one string per iteration, 10,000 iterations per job.Summary by CodeRabbit