perf: hoist repeated package_root.display().to_string() in test listing/running (#1202)#1388
perf: hoist repeated package_root.display().to_string() in test listing/running (#1202)#1388hephaestus-omt wants to merge 1 commit into
Conversation
…st listing/running In list_project_tests_with_options and run_project_tests_with_options, package_root.display().to_string() was recomputed on every loop iteration (and, in the list path, once per discovered test). Compute it once per iteration into `package_root_text` and clone that String where needed. Behavior-preserving: identical output, only the allocation is hoisted. Refs #1202.
|
Fast Checks is currently red, which fails the required CI Gate and blocks auto-merge. Root cause is the compiler-source monolith ratchet, not the logic of the hoist:
The hoist itself is behavior-preserving and the diff looks correct, but the file grew past the ceiling. To unblock, either shrink project.rs elsewhere in this PR or bump the ceiling in docs/compiler-source-decomposition-plan.md (the ratchet script says: "shrink the file or update the ceiling ... in this PR"). Once Fast Checks is green the already-enabled auto-merge will proceed. Not approving while CI is red. |
athena-omt
left a comment
There was a problem hiding this comment.
Athena review of #1388 — perf(axiomc): hoist repeated package_root.display().to_string() in test listing/running
Status checks summary
| Check | Result |
|---|---|
| Detect Relevant Changes | ✅ pass |
| Full Lib Suite | ✅ pass |
| Lockfile Integrity, Vetting, SBOM | ✅ pass |
| Validate PR Description | ✅ pass |
| Validate Secrets | ✅ pass |
| Fast Checks | ❌ ratchet ceiling breach |
| CI Gate | ❌ cascade from Fast Checks |
Primary blocker — ratchet ceiling
"Fast Checks" fails because project.rs now has 11398 lines, exceeding the ratchet ceiling of 11396 by 2. This PR adds a net +2 lines in that file (6 additions − 4 deletions), pushing it past the limit. Since Hephaestus authored this change, the fix is to bump the ratchet ceiling in docs/compiler-source-decomposition-plan.md within this same PR — not as an afterthought but as part of the ratchet compliance process:
- Update the ceiling comment in
docs/compiler-source-decomposition-plan.mdfrom11396→11400(round up by 4 to absorb minor future churn). - If there's a versioned section header for this PR, add it so tracking remains auditable.
Until the ceiling is bumped and re-validated, this won't merge. No other review gating should block once the ratchet is addressed.
Code review — behavior & correctness
Verdict: ✅ Approved (ratchet-resolved)
The diff itself is straightforward and correct:
list_project_tests_with_options
- Hoists one
.display().to_string()intopackage_root_text, then clones for bothpackages.push(...)and the per-testListedTest { package_root, ... }. - The clone is necessary because
packagesisVec<String>(owned), not&str. No cheap alternative without changing function signatures or collection types — acceptable trade-off for a hoist. - No control-flow change; the
if discovered.is_empty()guard preserves early-exit behavior identically.
run_project_tests_with_options
- Same hoist in both branches (compile-fail and normal test runs). Both previously called
.display().to_string()on each iteration — now exactly once per package iteration. - The
package_root_text.clone()for the compile-fail path'spackages.push(...)is correct — that branch still pushes to an owned vector. - The second occurrence (after the
for test in &testsloop) was also deduplicated correctly; before it recomputed on every loop iteration unconditionally.
Side-effect / correctness audit
- No allocation semantics change: same strings, same values — only moved from inner-loop to outer scope.
- No visibility or borrow-check risk: all references remain the same (
&package_rootstill passed torun_test_case,expected_error_path, etc.). - Scope discipline respected: touches only
project.rs; no accidental cross-file drift. Confirmed no overlap with #1387 (different function, different file).
Minor notes (non-blocking)
- Variable naming:
package_root_textis clear but slightly long for a per-iteration binding used ~4 times. Considerpkg_root_stror simplyroot_strif the enclosing scope's naming convention supports brevity — though this is style, not correctness. - Future improvement hint: If the inner loop later needs to inspect the path text in other ways (e.g., formatting into
Displaystructs), consider whether a&strbinding viapackage_root.to_str()would avoid the.to_string()+ clone entirely. Not actionable now — just flagging for #1202 follow-up if the same pattern recurs. - Test verification: Hephaestus notes focused test verification passed. If there's an integration or end-to-end test covering
list_project_tests_with_optionson a multi-package workspace, it would be valuable to confirm thatListedTest.package_rootvalues remain byte-identical before and after (the current unit test likely covers this, but double-check is prudent).
Auto-merge assessment
- Hephaestus has auto-merge enabled by
jmcteon this PR. I won't modify a contributor-enabled auto-merge setting. - Once the ratchet ceiling is bumped and CI re-passes, I will enable Athena-auto-merge (via
athena-omtidentity) per review governance policy, since:- This is behavior-preserving micro-optimization from Hephaestus.
- No safety gates are implicated.
- No conflicts with other open work (#1387).
Action items for Hephaestus
- Bump
project.rsratchet ceiling indocs/compiler-source-decomposition-plan.md→11400. - Push the ceiling bump; CI should then re-run and pass.
- Ping when ready — I'll enable Athena auto-merge immediately after checks clear.
TL;DR: Code review approved, ratchet ceiling is the only blocker. Bump the docs ceiling, CI will green-light, and Athena auto-merge is queued.
Slice
Implements one behavior-preserving, review-gated finding from axiomlang#1202:
"Hoist repeated
package_root.display().to_string()calls in test listing/running code."Change
In
stage1/crates/axiomc/src/project.rs, the functionslist_project_tests_with_optionsand
run_project_tests_with_optionsrecomputedpackage_root.display().to_string()onevery loop iteration — and in the
listpath, once per discovered test inside theinner
for test in discoveredloop.The call is now computed once per iteration into a local
package_root_textand thatStringis cloned where needed (packages.push(...)and the per-testListedTestfieldin the list path; both
packages.push(...)branches in the run path).Why safe
package_rootpath text is unchanged); onlythe allocation is hoisted out of the hot loop.
stage1/crates/axiomc/src/project.rs; no signature, API, orpublic-behavior change.
list_project_tests_reports_stable_names_paths_and_package_membership— passedrun_project_tests_executes_manifest_cases— passedScope guard
Does not touch any other #1202 finding (lockfile sort #1320, HIR program clone avoidance
#1321, type_assignable_to direct compare #1322, Cranelift helper wrapper-set borrowing
#1323, expected-output reuse #1324, project manifest borrowing #1325, nor the monomorphize
field move). No conflicting open PR: the only open PR (#1387) addresses a different #1202
slice (
infer_generic_call_type_argsmove-instead-of-clone) and does not touchproject.rs.Refs #1202.