[PR #3285] [MERGED] perf(artifacts): run the asset catalog and Interface Builder bundle walks together #5699

New issue

Closed

opened 2026-10-31 01:08:44 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:08:44 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3285
Author: @kaoru0822-kitauji
Created: 10/14/2026
Status: ✅ Merged
Merged: 10/14/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: perf/asset-catalog-concurrent-ib

📝 Commits (2)

e088f93 perf(artifacts): run the asset catalog and Interface Builder bundle walks together
726190d test(artifacts): hold asset catalog and Interface Builder work open to pin overlap

📊 Changes

2 files changed (+166 additions, -40 deletions)

View changed files

📝 src/artifacts/apple/AppleAssetCatalogAnalysis.test.ts (+89 -1)
📝 src/artifacts/apple/AppleAssetCatalogAnalysis.ts (+77 -39)

📄 Description

Summary

inspect_asset_catalog walks the app bundle twice, one after the other. It first finds and inspects Assets.car files, then runs the full Interface Builder analysis, whose own bundle walk supplies resource keys for the asset-key join. On large Apple bundles each walk takes about 1.7 s. The two analyses are independent and read-only, so this PR starts the Interface Builder analysis first and lets both run together. Output is identical; Keynote drops from 4.20 s to 2.89 s.

No linked issue: this is a hotspot I measured in a sweep of static tools over /Applications bundles (Keynote inspect-asset-catalog: 4.1 s).

Problem and expected behavior

rea inspect-asset-catalog /Applications/Keynote.app on main 307af044b: 4.2 s wall, 2.1 s of it system time. A CPU profile shows 2.7 s idle (I/O) on the main thread, #readDirectory/assertContainedDirectory from two bundle walks, and one assetutil --info child (0.15 s). Phases measured with the dist modules: catalog walk 1,690 ms (37k entries, 1 Assets.car), then analyzeInterfaceBuilderBundle 1,782 ms.

Change and scope

src/artifacts/apple/AppleAssetCatalogAnalysis.ts only:

The catalog loop and its empty-inventory check move unchanged into collectAssetCatalogs.
analyzeAppleAssetCatalogs starts the Interface Builder analysis before collecting catalogs, on its own AbortController combined with the caller's signal (AbortSignal.any).
Failure order matches the sequential code:
- A catalog failure (walk, assetutil, metadata limit, or the non-file-only case) cancels the Interface Builder analysis, waits for it to settle, and is reported.
- Otherwise an Interface Builder failure is reported where it was before.
- Nothing is left running after return or throw.
A test seam analyzeInterfaceBuilder sits beside the existing runAssetUtil seam; production uses analyzeInterfaceBuilderBundle.
Not changed: either analysis itself, the shared DirectoryArtifactReader per-entry containment checks, or output.

Contract and boundary impact

Semantic owner and earliest changed stage: asset catalog workflow orchestration.
Contribution scope fit and linked maintainer decision: existing read-only static inspection; not applicable.
CLI and MCP/tool-catalog contract: unchanged.
Provider, bridge, target-format, or platform compatibility: unchanged (assetutil on macOS).
Evidence, artifact, provenance, or reconstruction contract: normalized_result byte-identical on every app tested.
Target selection, authority, upstream behavior, and cleanup details: same reads; the IB analysis is cancelled and settled on catalog failure; caller cancellation reaches both.
Generated metadata, package, or installation impact: none.
Read existing files or captured data.
Launch or attach to a process.
Inject instrumentation or run scripts/code.
Change process memory, registers, files, or configuration.
Contact network services.
Recover protected code or work around protections.
Documentation or usability only; no target interaction.
Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

Real CLI, main 307af044b vs this branch, mean of 3 runs, macOS 26.7.1 arm64:

`rea inspect-asset-catalog`	wall (main → branch)	CPU user+sys	peak RSS	output bytes	`normalized_result`
Keynote.app	4.20 s → 2.89 s	4.86 → 5.14 s	583 → 571 MB	763,296 (same)	identical
Pages.app	4.36 s → 2.95 s	4.90 → 5.25 s	564 → 556 MB	747,830 (same)	identical
GarageBand.app	6.00 s → 4.06 s	6.97 → 7.21 s	602 → 616 MB	562,402 (same)	identical
Transmission.app	0.81 s → 0.74 s	1.26 → 1.23 s	508 → 501 MB	108,387 (same)	identical
XLD.app	1.75 s → 1.79 s	3.34 → 3.47 s	1212 → 1212 MB	27,897 (same)	identical

Keynote and GarageBand were re-measured interleaved (main/branch alternating, 3 each) to rule out load noise. Agent token cost is unchanged (same output); the win is time to an answer. CPU rises slightly because the walks overlap.

Tests (src/artifacts/apple/AppleAssetCatalogAnalysis.test.ts, new describe block; the review follow-up 726190dfa holds work open with controlled promises). Mutating the implementation to await the Interface Builder analysis before catalogs makes the overlap and cancellation tests fail; against main's implementation the cancellation and early-failure tests fail:

keeps the Interface Builder analysis running while catalogs are inspected: the Interface Builder stub stays pending until catalog inspection starts, so a sequential implementation times out
cancels the Interface Builder analysis and reports a catalog failure: checks the IB analysis observes cancellation and settles before the call rejects with the catalog error.
reports an early Interface Builder failure only after catalogs are inspected: catalog inspection is held open while the Interface Builder stub rejects; the call must not settle until the catalog finishes

Remaining proof gaps: none known. Peak RSS moves by about ±15 MB, since both analyses' working sets can now overlap.

Validation performed

npm run typecheck — pass
npm run lint — 0 errors
npm run jscpd — pass (3.20%)
npm run test:focused on 16 files covering asset catalogs and Interface Builder (module, boundary, acceptance) — 141 passed, 8 skipped
Real CLI comparison above.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: real macOS app bundles listed above with assetutil.
Package or release metadata impact: none.
Security, privacy, process, or containment review: same read-only walks and containment checks; the second analysis is cancelled and awaited on failure.

Author readiness

Human owner:

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare the current revision ready for maintainer review.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/3285 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/14/2026 **Status:** ✅ Merged **Merged:** 10/14/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `perf/asset-catalog-concurrent-ib` --- ### 📝 Commits (2) - [`e088f93`](https://github.com/morluto/rea/commit/e088f937cb9b9b0d890706486a08129841c8f183) perf(artifacts): run the asset catalog and Interface Builder bundle walks together - [`726190d`](https://github.com/morluto/rea/commit/726190dfacb036304711a0a102b6e132ce4103dc) test(artifacts): hold asset catalog and Interface Builder work open to pin overlap ### 📊 Changes **2 files changed** (+166 additions, -40 deletions) <details> <summary>View changed files</summary> 📝 `src/artifacts/apple/AppleAssetCatalogAnalysis.test.ts` (+89 -1) 📝 `src/artifacts/apple/AppleAssetCatalogAnalysis.ts` (+77 -39) </details> ### 📄 Description ## Summary `inspect_asset_catalog` walks the app bundle twice, one after the other. It first finds and inspects `Assets.car` files, then runs the full Interface Builder analysis, whose own bundle walk supplies resource keys for the asset-key join. On large Apple bundles each walk takes about 1.7 s. The two analyses are independent and read-only, so this PR starts the Interface Builder analysis first and lets both run together. Output is identical; Keynote drops from 4.20 s to 2.89 s. No linked issue: this is a hotspot I measured in a sweep of static tools over `/Applications` bundles (Keynote `inspect-asset-catalog`: 4.1 s). ## Problem and expected behavior `rea inspect-asset-catalog /Applications/Keynote.app` on main `307af044b`: 4.2 s wall, 2.1 s of it system time. A CPU profile shows 2.7 s idle (I/O) on the main thread, `#readDirectory`/`assertContainedDirectory` from two bundle walks, and one `assetutil --info` child (0.15 s). Phases measured with the dist modules: catalog walk 1,690 ms (37k entries, 1 `Assets.car`), then `analyzeInterfaceBuilderBundle` 1,782 ms. ## Change and scope `src/artifacts/apple/AppleAssetCatalogAnalysis.ts` only: - The catalog loop and its empty-inventory check move unchanged into `collectAssetCatalogs`. - `analyzeAppleAssetCatalogs` starts the Interface Builder analysis before collecting catalogs, on its own `AbortController` combined with the caller's signal (`AbortSignal.any`). - Failure order matches the sequential code: - A catalog failure (walk, `assetutil`, metadata limit, or the non-file-only case) cancels the Interface Builder analysis, waits for it to settle, and is reported. - Otherwise an Interface Builder failure is reported where it was before. - Nothing is left running after return or throw. - A test seam `analyzeInterfaceBuilder` sits beside the existing `runAssetUtil` seam; production uses `analyzeInterfaceBuilderBundle`. - Not changed: either analysis itself, the shared `DirectoryArtifactReader` per-entry containment checks, or output. ## Contract and boundary impact - Semantic owner and earliest changed stage: asset catalog workflow orchestration. - Contribution scope fit and linked maintainer decision: existing read-only static inspection; not applicable. - CLI and MCP/tool-catalog contract: unchanged. - Provider, bridge, target-format, or platform compatibility: unchanged (`assetutil` on macOS). - Evidence, artifact, provenance, or reconstruction contract: `normalized_result` byte-identical on every app tested. - Target selection, authority, upstream behavior, and cleanup details: same reads; the IB analysis is cancelled and settled on catalog failure; caller cancellation reaches both. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage **Real CLI, main `307af044b` vs this branch, mean of 3 runs, macOS 26.7.1 arm64:** | `rea inspect-asset-catalog` | wall (main → branch) | CPU user+sys | peak RSS | output bytes | `normalized_result` | | --- | ---: | ---: | ---: | ---: | --- | | Keynote.app | 4.20 s → **2.89 s** | 4.86 → 5.14 s | 583 → 571 MB | 763,296 (same) | identical | | Pages.app | 4.36 s → **2.95 s** | 4.90 → 5.25 s | 564 → 556 MB | 747,830 (same) | identical | | GarageBand.app | 6.00 s → **4.06 s** | 6.97 → 7.21 s | 602 → 616 MB | 562,402 (same) | identical | | Transmission.app | 0.81 s → 0.74 s | 1.26 → 1.23 s | 508 → 501 MB | 108,387 (same) | identical | | XLD.app | 1.75 s → 1.79 s | 3.34 → 3.47 s | 1212 → 1212 MB | 27,897 (same) | identical | Keynote and GarageBand were re-measured interleaved (main/branch alternating, 3 each) to rule out load noise. Agent token cost is unchanged (same output); the win is time to an answer. CPU rises slightly because the walks overlap. **Tests** (`src/artifacts/apple/AppleAssetCatalogAnalysis.test.ts`, new describe block; the review follow-up 726190dfa holds work open with controlled promises). Mutating the implementation to await the Interface Builder analysis before catalogs makes the overlap and cancellation tests fail; against main's implementation the cancellation and early-failure tests fail: - `keeps the Interface Builder analysis running while catalogs are inspected`: the Interface Builder stub stays pending until catalog inspection starts, so a sequential implementation times out - `cancels the Interface Builder analysis and reports a catalog failure`: checks the IB analysis observes cancellation and settles before the call rejects with the catalog error. - `reports an early Interface Builder failure only after catalogs are inspected`: catalog inspection is held open while the Interface Builder stub rejects; the call must not settle until the catalog finishes **Remaining proof gaps:** none known. Peak RSS moves by about ±15 MB, since both analyses' working sets can now overlap. ## Validation performed - `npm run typecheck` — pass - `npm run lint` — 0 errors - `npm run jscpd` — pass (3.20%) - `npm run test:focused` on 16 files covering asset catalogs and Interface Builder (module, boundary, acceptance) — 141 passed, 8 skipped - Real CLI comparison above. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: real macOS app bundles listed above with `assetutil`. - Package or release metadata impact: none. - Security, privacy, process, or containment review: same read-only walks and containment checks; the second analysis is cancelled and awaited on failure. ## Author readiness Human owner:  - [ ] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [ ] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [ ] I declare the current revision ready for maintainer review. ## Before merge - [ ] This PR addresses a concrete problem or an agreed enhancement. - [ ] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [ ] The PR has one focused outcome and the title follows `type(scope): outcome`. - [ ] Related issue is linked, or the reason for not linking one is stated above. - [ ] Tests cover changed observable behavior and meaningful failure paths. - [ ] Owning docs, contracts, and generated metadata are updated where needed. - [ ] User-visible CLI/MCP changes include representative output. - [ ] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:08:44 +00:00