perf(test): derive the census RTC frame once per model, not once per host (#4127) - #4158
perf(test): derive the census RTC frame once per model, not once per host (#4127)#4158louistrue wants to merge 2 commits into
Conversation
…host (#4127) The heavy watertightness census lane has never finished a scheduled run. Since #3933 every `process()` call in `triangulation_invariance` went through `census_rtc::router`, which built an entity index over the whole file, walked every entity again calling `has_geometry_by_name` (2,807,815 records for ISSUE_053) to collect geometry jobs, and only then sampled placements for the RTC offset. The sweep makes two of those calls per void host plus a third for a torn one, so the two heavy fixtures paid that O(file) work 1330 times (2 * 652 hosts + 26 torn). Both hoisted values are pure functions of the file, so they move out of the per-host loop into one `ModelFrame` per model: the entity index (handed to each host's decoder as the same `Arc`, which is the store `with_index` builds anyway) and the RTC offset. The router is deliberately NOT hoisted. It carries per-host caches and bookkeeping (mapped items, item dedup, geometry hashes, consumed-void hosts, CSG failure diagnostics), so sharing one across hosts would be a question about census rows rather than about speed. Every host still gets a fresh `GeometryRouter` and a fresh `EntityDecoder`; only the read-only index and the offset are shared. Nothing outside `rust/geometry/tests/` changes. ONE per-host O(file) walk survives, and the module doc now says so instead of reading as "nothing per-host is O(file)". `ModelFrame::router` calls `GeometryRouter::with_units`, whose `scan_unit_scale` reads entities from the start of the file until it finds `IFCPROJECT`. Usually that stops in the first few KB (ISSUE_053: byte 4246 of 177 MB). Measured, three gated fixtures where it does not, because the exporter emitted `IFCPROJECT` last: ara3d/duplex.ifc byte 2,380,634 of 2,380,763 ara3d/dental_clinic.ifc byte 9,496,389 of 13,003,205 various/01_BIMcollab_Example_ARC.ifc byte 6,252,152 of 18,230,149 (line 100819) The unit scale is not hoisted here either, and that is a decision rather than an oversight: the only way to reuse it is `GeometryRouter::with_scale_and_rtc`, which unlike `with_units` does not call `arm_content_dedup()`. In rust/geometry/src/router/mod.rs, `with_units` and `with_units_and_rtc` are the only two callers of `arm_content_dedup`. Swapping it in would silently disable item content-dedup for the census and could move rows, which is the one thing this change must not do. It needs its own argument about dedup. `ModelFrame` now borrows the content it was built from and hands it back out itself rather than taking a `&str` per call. The index holds absolute byte spans and `EntityDecoder::get_raw_bytes` slices them unchecked, so `ModelFrame::new(&a)` followed by `process(&frame, &b, ..)` compiled and would have decoded the wrong entities or panicked out of bounds. PROBED: a frame that outlives its content is now E0597 at compile time. The lifetime also drops seven redundant `&content` / `&ifc` arguments across the call sites. MEASURED, this machine, the workflow's exact invocation (`cargo test -p ifc-lite-geometry --features triangulation-alt --test triangulation_invariance -- --ignored --nocapture --test-threads=1 heavy_fixture`), debug/test profile, both heavy fixtures: before (0546d79): 751.21 s after: 6.8 to 7.0 s over three runs THE GOLDEN DID NOT MOVE. `watertightness_census_heavy.tsv` is untouched and both lanes still gate against it with `regressed : 0` and `coverage loss : 0`. Stronger than that: the per-fixture run rows this sweep writes to `target/watertightness_census.heavy.*.tsv` are BYTE-IDENTICAL over all 652 hosts (289 + 363, 26 torn on ISSUE_068), before the perf change, after it, and after the lifetime refactor: b54873d5ab75a8176ca79e4620ed58ce watertightness_census.heavy.ISSUE_053_20181220Holter_Tower_10.tsv 3d121119264ae66a8a525056d95b6908 watertightness_census.heavy.ISSUE_068_ARK_NUS_skolebygg.tsv The per-PR census lane (`--features triangulation-alt` without `--ignored`) is also green: 64 passed, 0 failed. Two corrections to #4127. The 5.5x figure understates the cost, because it compared against 4417f9a, which already rebuilt the entity index per call; against the current lane the sweep is over 100x faster. And the fix is not "hoist the router and entity index" as the issue suggests: hoisting the router would have changed what the census can see. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfAavu3wBCfAPxEsv9BJ4K
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_bd4dbf15-49d0-4afe-8e16-2c8fa1a21795) |
|
Warning Review limit reached
This review includes 2 billable files and costs up to $0.50. Or wait 7 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Claude review - no findings for
|
github-actions
Bot
added
the
llm-reviewed
Viewer benchmark✅ No threshold regressions detected. 01_Snowdon_Towers_Sample_Structural(1).ifcBaseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
AC20-FZK-Haus.ifcBaseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
Refresh the baseline from a CI run: dispatch the Benchmark workflow with |
![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: 55797ebef4
ℹ️ 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".
| let index = Arc::new(build_entity_index(content)); | ||
| let mut decoder = EntityDecoder::with_arc_index(content, Arc::clone(&index)); | ||
| let offset = detect_rtc_offset(content, &mut decoder); |
There was a problem hiding this comment.
This is a measured performance change—the commit reports the heavy census dropping from 751.21 s to 6.76 s—but the patch leaves scripts/perf/README.md unchanged. Record the verdict and lesson there in this PR so future optimization work does not repeat this investigation, as required for measured performance changes.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ff6d56ca-1869-43cc-8fdc-f32aa17308db) |
github-actions
Bot
removed
the
llm-reviewed
Claude review - no findings for
|
github-actions
Bot
added
the
llm-reviewed
1 participant
Part of #4127.
The heavy census lane has never completed. Both runs it has ever had were cancelled at
the 60-minute cap with no census row emitted. The cause is a performance regression in
the test harness, and this is the fix for it. The workflow-side reporting change is a
separate PR.
Measured
Exact workflow invocation,
cargo test -p ifc-lite-geometry --features triangulation-alt --test triangulation_invariance -- --ignored --nocapture --test-threads=1 heavy_fixture:Both fixtures. That is faster than the pre-regression baseline too, because #3933 was
not the only per-host cost.
Cause
sweep()callsprocess()twice per void host and a third time for a torn one, roughly1300 times across the two heavy fixtures. Since #3933 each call went through
census_rtc::router, which per call built an entity index over every record and thenwalked every record again calling
has_geometry_by_nameto collect the geometry jobs.On ISSUE_053 that is 2.8 M entities, twice, per call.
Fix
The frame is a pure function of the file, so derive it ONCE per model.
ModelFrameholdsthe
Arc<EntityIndex>and the RTC offset;sweep()builds one before the host loop.The router is deliberately NOT hoisted. It carries per-host state (
mapped_item_cache,item_dedup_cache,geometry_hash_cache,content_sig_memo,voids_consumed_hosts,plus the CSG-failure and host-opening diagnostics), and sharing it would let information
carry from host N to host N+1. Every host still gets a fresh
GeometryRouter::with_unitsand a fresh
EntityDecoder; only the read-only index and the offset are shared.That is what answers the golden question by construction rather than by hoping.
The golden did not move
ISSUE_068_KNOWN_TORN_HOSTS.regressed : 0,coverage loss : 0,added : 0,reclassified : 0,retessellated : 0,volume moved : 0.An independent reviewer reproduced this: their HEAD rows md5-match mine, and they
separately measured the pre-change code's ISSUE_053 rows as identical to the post-change
rows.
Why dropping the per-host offset pass is safe
resolve_scaled_placement,sample_element_translation,detect_rtc_offset_with_fallbackandget_placement_transform_from_elementare all&selfwith noborrow_mut; they mutate only the decoder's id-keyed memos. The subtlecase: the old pass warmed
placement_transform_cacheBEFOREset_rtc_offsetwas called.Had it baked the offset into a cached transform, the old harness would have been carrying
rtc-less transforms into meshing. It does not:
rtc_offsetis read at mesh-transformtime, so the cache is a pure function of placement id and a warm cache equals a cold one.
The offset is also independent of the alt-triangulator toggle (neither sampler
tessellates), which matters because the frame is now built outside the base/alt toggling.
ModelFramebinds its index to its contentModelFrame<'a>STORES the content;decoder()androuter()take no contentparameter. That, not the lifetime, is what makes a wrong pairing unrepresentable: two
distinct
Strings can both yield&'a strfor one'a, so a re-added parameter wouldtype-check against a different string, and the index holds absolute byte spans that
EntityDecoderslices unchecked. Demonstrated by compiling the bad shape, not argued.What is still per host, stated because the first draft did not
with_unitscallsscan_unit_scale, which walks to the firstIFCPROJECT. On the twoheavy fixtures that is byte 4,246 and 5,070, so it costs nothing on the lane this fixes.
But some exporters emit
IFCPROJECTlast, and three corpus fixtures underMAX_FIXTURE_BYTESdo:duplex.ifcat 2,380,634 of 2,380,763;dental_clinic.ifcat9,496,389 of 13,003,205;
01_BIMcollab_Example_ARC.ifcat 6,252,152 of 18,230,149.Hoisting the unit scale is declined here on purpose:
with_scale_and_rtcdoes not callarm_content_dedup()the waywith_unitsdoes, so a naive swap silently disarms itemcontent-dedup for the census, which is exactly the class of change this PR must not make.
Verification
Test harness only. No kernel change, no changeset (no
packages/*).🤖 Generated with Claude Code
https://claude.ai/code/session_01NfAavu3wBCfAPxEsv9BJ4K