morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 07:47:29 +00:00

[PR #4859] [CLOSED] perf(analysis-view): reuse one validated interpretation per sealed source (#4810) #6444

New issue

Closed

opened 2026-10-31 01:14:30 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:14:30 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4859
Author: @DeryFerd
Created: 10/17/2026
Status: ❌ Closed

Base: main ← Head: perf/4810-sealed-view-reuse

📝 Commits (1)

ab28982 perf(analysis-view): reuse one validated interpretation per sealed source

📊 Changes

3 files changed (+306 additions, -54 deletions)

View changed files

📝 src/domain/analysisView/analysisView.ts (+62 -39)
📝 src/domain/analysisView/analysisViewParent.ts (+95 -15)
➕ src/domain/analysisView/analysisViewSealedReuse.test.ts (+149 -0)

📄 Description

Summary

Fixes #4810. inspect_analysis_view repeated the complete schema and binding validation for a JavaScript application parent on every page, and re-parsed the binary layout on every page, even when the parent Evidence was sealed and could not change. One sealed source now keeps its first successful interpretation, so later pages reuse it instead of validating again. Warm summary pages on a sealed JavaScript parent went from 8.1 ms to 1.2 ms in my measurements.

Problem and expected behavior

The smallest trigger is any paged view over retained Evidence, on a source that was imported rather than produced in this process.

Producer Evidence carries realm-private proofs: parseOwnedJavaScriptApplicationAnalysisSteps checks isValidatedImmutableJavaScriptSemanticGraph, which consults a WeakSet populated only by the graph factory. An imported bundle gets freezeJsonSnapshot on its record and normalized_result, so isImmutableJsonSnapshot is true and AnalysisViewService passes sealedResult: true, but the nested graphs never enter that WeakSet. The owned fast path therefore declines, and the code falls back to javascriptApplicationAnalysisResultSchema.parse — a full schema pass plus checkApplicationBindingsSteps across every node and relation. inspect_binary_layout is simpler: binaryLayoutSchema.safeParse(parent.normalizedResult) runs unconditionally per page.

I confirmed that shape before changing anything. Through parseImmutableEvidenceBundle, a layout record reports isImmutableJsonSnapshot(record) === true and isImmutableJsonSnapshot(record.normalized_result) === true, while the graphs inside a JavaScript record report no producer proof. A synthetic 10,501-node / 6,000-unknown graph took about 2.3 s for one full parse attempt, which is the same order as the 1.05–1.46 s per-page cost in the report.

The violated invariant is in docs/tool-design.md#own-the-lifecycle and AGENTS.md §Performance: a sealed source cannot change, so re-establishing the same facts on every page spends CPU and time without adding certainty. The expected behavior is one complete validation per immutable source, reused for its later pages, with no change to any output.

Change and scope

src/domain/analysisView/analysisViewParent.ts gains reusableAnalysisViewSource and sealedAnalysisViewInterpretation. The first admits a source only when sealedResult === true and the value is the exact object registered by freezeJsonSnapshot; the flag alone is a caller-supplied hint and cannot admit anything. The second looks up a completed interpretation by source and by format owner, parses on a miss, and remembers the result only after the whole value validated.

The cache is WeakMap<source, Map<owner, WeakRef<interpretation>>>. The weak key means an entry is released with the Evidence that owns it. The WeakRef value means memory pressure can reclaim it and a reclaimed entry costs one re-parse rather than returning something stale. Separating by owner means two operations over the same source cannot read each other's interpretation.

analysisView.ts routes the two hot paths through it. The JavaScript path keeps parseOwnedJavaScriptApplicationAnalysisSteps as the parser, so in-realm producer Evidence still takes the existing proof-based shortcut; only the completed result is now retained. The layout path keeps its existing error projection unchanged.

Intentionally not included: caching failed validations, adding a public projection API, persisting anything, caching live or mutable state, or changing any default. A rejected parse is never remembered, and an abandoned generator cannot publish because the entry is written only after the parse returns.

Ordinary performance fix inside existing analysis-view ownership. No new provider, target execution, or protection-recovery capability, so a separate maintainer scope decision is not applicable.

Contract and boundary impact

Semantic owner and earliest changed stage: retained analysis-view parent interpretation in src/domain/analysisView/analysisViewParent.ts; the earliest changed stage is the parent parse that precedes every projection.
Contribution scope fit and linked maintainer decision: existing static retained-Evidence reads only; no target execution or mutation. Not applicable.
CLI and MCP/tool-catalog contract: none. inspect_analysis_view inputs, outputs, and the advertised schemas are unchanged.
Provider, bridge, target-format, or platform compatibility: none. The change is format-neutral and runs in the domain layer.
Evidence, artifact, provenance, or reconstruction contract: outputs are byte-identical. Evidence IDs, links, omissions, null versus scalar distinctions, errors, and coverage all keep their current values, because the cached value is exactly the object the schema returned on the first call.
Target selection, authority, upstream behavior, and cleanup details: no target is selected or executed. The cache holds only already-parsed JSON derived from Evidence the caller supplied; it creates no process, descriptor, or temporary resource, so there is nothing to release beyond the weak references.
Generated metadata, package, or installation impact: none.

Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible.

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

Tests added: src/domain/analysisView/analysisViewSealedReuse.test.ts, six cases. They observe the work directly by spying on the two schemas, so the assertions are about validation count rather than wall time.

A sealed JavaScript parent validates once and reuses for four pages.
A sealed layout parent validates once and reuses for four pages.
An unsealed parent validates on every page (control for the fast path not leaking).
A hand-frozen parent with sealedResult: true but no snapshot proof validates on every page (control for the hint being insufficient).
A sealed parent with invalid content fails on both attempts and never caches the rejection.
Two distinct sealed sources each validate once, so one source cannot read another's interpretation.

Base reproduction: with the two source files reverted, the first two cases fail with expected "Mock" to be called 1 times, but got 4 times — four pages, four validations, exactly the reported behavior. The four control cases pass in both states, which is what they are for. With the change, all six pass.

Measurements, on a sealed JavaScript parent built from the repository's own example analysis (kind: "summary", 20 warm pages, Node 24.21.0, Windows x64):

	cold	warm total	warm per page
Before	30.69 ms	162.47 ms	8.12 ms
After	18.27 ms	23.49 ms	1.17 ms

That is roughly 7× on warm pages. I did not measure CPU time, peak process-family memory, or retained-state size, and I did not reproduce the reporter's authentic 21 MB npm bundle or the SQLite ELF layout, so this is not a whole-task speedup claim and not the measurement set the issue's acceptance criteria ask for. The numbers above describe one synthetic sealed parent on this host.

For evidence-bearing changes:

Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

Host: Windows 10 x64, Node 24.21.0, npm 11.9.0.

npx vitest run --project domain src/domain/analysisView/ — 130 passed across 8 files.
npx vitest run src/application/analysisView/AnalysisViewService.test.ts — 34 passed.
npx vitest run tests/boundary/cli/analysisViewCli.test.ts --testTimeout=180000 — 13 passed.
npx vitest run tests/boundary/mcp/analysisViewMcp.test.ts --testTimeout=120000 — 18 passed.
Red-green check — reverting analysisView.ts and analysisViewParent.ts makes the two reuse cases fail with expected "Mock" to be called 1 times, but got 4 times; restoring them makes all six pass.
npm run check:fast — typecheck and lint passed, 2 of 2 tasks.
npx oxfmt --check src/domain/analysisView/*.ts — all files correctly formatted.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: none. No provider engine is involved.
Package or release metadata impact: none.
Security, privacy, process, or containment review: the cache admits only values whose snapshot proof was registered by this realm, so forged freeze hints and caller-supplied flags cannot cause reuse. It retains no credential, path, or environment data beyond the parsed JSON the caller already supplied, and it creates no process or descriptor.

Author readiness

Human owner: @DeryFerd

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 this PR ready for initial 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.

_{🔄 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/4859 **Author:** [@DeryFerd](https://github.com/DeryFerd) **Created:** 10/17/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `perf/4810-sealed-view-reuse` --- ### 📝 Commits (1) - [`ab28982`](https://github.com/morluto/rea/commit/ab2898250459da2c4f0fdb0e48e4ac49610d0f86) perf(analysis-view): reuse one validated interpretation per sealed source ### 📊 Changes **3 files changed** (+306 additions, -54 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/analysisView/analysisView.ts` (+62 -39) 📝 `src/domain/analysisView/analysisViewParent.ts` (+95 -15) ➕ `src/domain/analysisView/analysisViewSealedReuse.test.ts` (+149 -0) </details> ### 📄 Description ## Summary Fixes #4810. `inspect_analysis_view` repeated the complete schema and binding validation for a JavaScript application parent on every page, and re-parsed the binary layout on every page, even when the parent Evidence was sealed and could not change. One sealed source now keeps its first successful interpretation, so later pages reuse it instead of validating again. Warm summary pages on a sealed JavaScript parent went from 8.1 ms to 1.2 ms in my measurements. ## Problem and expected behavior The smallest trigger is any paged view over retained Evidence, on a source that was imported rather than produced in this process. Producer Evidence carries realm-private proofs: `parseOwnedJavaScriptApplicationAnalysisSteps` checks `isValidatedImmutableJavaScriptSemanticGraph`, which consults a `WeakSet` populated only by the graph factory. An imported bundle gets `freezeJsonSnapshot` on its record and `normalized_result`, so `isImmutableJsonSnapshot` is true and `AnalysisViewService` passes `sealedResult: true`, but the nested graphs never enter that `WeakSet`. The owned fast path therefore declines, and the code falls back to `javascriptApplicationAnalysisResultSchema.parse` — a full schema pass plus `checkApplicationBindingsSteps` across every node and relation. `inspect_binary_layout` is simpler: `binaryLayoutSchema.safeParse(parent.normalizedResult)` runs unconditionally per page. I confirmed that shape before changing anything. Through `parseImmutableEvidenceBundle`, a layout record reports `isImmutableJsonSnapshot(record) === true` and `isImmutableJsonSnapshot(record.normalized_result) === true`, while the graphs inside a JavaScript record report no producer proof. A synthetic 10,501-node / 6,000-unknown graph took about 2.3 s for one full parse attempt, which is the same order as the 1.05–1.46 s per-page cost in the report. The violated invariant is in `docs/tool-design.md#own-the-lifecycle` and `AGENTS.md` §Performance: a sealed source cannot change, so re-establishing the same facts on every page spends CPU and time without adding certainty. The expected behavior is one complete validation per immutable source, reused for its later pages, with no change to any output. ## Change and scope `src/domain/analysisView/analysisViewParent.ts` gains `reusableAnalysisViewSource` and `sealedAnalysisViewInterpretation`. The first admits a source only when `sealedResult === true` **and** the value is the exact object registered by `freezeJsonSnapshot`; the flag alone is a caller-supplied hint and cannot admit anything. The second looks up a completed interpretation by source and by format owner, parses on a miss, and remembers the result only after the whole value validated. The cache is `WeakMap<source, Map<owner, WeakRef<interpretation>>>`. The weak key means an entry is released with the Evidence that owns it. The `WeakRef` value means memory pressure can reclaim it and a reclaimed entry costs one re-parse rather than returning something stale. Separating by owner means two operations over the same source cannot read each other's interpretation. `analysisView.ts` routes the two hot paths through it. The JavaScript path keeps `parseOwnedJavaScriptApplicationAnalysisSteps` as the parser, so in-realm producer Evidence still takes the existing proof-based shortcut; only the completed result is now retained. The layout path keeps its existing error projection unchanged. Intentionally not included: caching failed validations, adding a public projection API, persisting anything, caching live or mutable state, or changing any default. A rejected parse is never remembered, and an abandoned generator cannot publish because the entry is written only after the parse returns. Ordinary performance fix inside existing analysis-view ownership. No new provider, target execution, or protection-recovery capability, so a separate maintainer scope decision is not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: retained analysis-view parent interpretation in `src/domain/analysisView/analysisViewParent.ts`; the earliest changed stage is the parent parse that precedes every projection. - Contribution scope fit and linked maintainer decision: existing static retained-Evidence reads only; no target execution or mutation. Not applicable. - CLI and MCP/tool-catalog contract: none. `inspect_analysis_view` inputs, outputs, and the advertised schemas are unchanged. - Provider, bridge, target-format, or platform compatibility: none. The change is format-neutral and runs in the domain layer. - Evidence, artifact, provenance, or reconstruction contract: outputs are byte-identical. Evidence IDs, links, omissions, `null` versus scalar distinctions, errors, and coverage all keep their current values, because the cached value is exactly the object the schema returned on the first call. - Target selection, authority, upstream behavior, and cleanup details: no target is selected or executed. The cache holds only already-parsed JSON derived from Evidence the caller supplied; it creates no process, descriptor, or temporary resource, so there is nothing to release beyond the weak references. - Generated metadata, package, or installation impact: none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [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 Tests added: `src/domain/analysisView/analysisViewSealedReuse.test.ts`, six cases. They observe the work directly by spying on the two schemas, so the assertions are about validation count rather than wall time. - A sealed JavaScript parent validates once and reuses for four pages. - A sealed layout parent validates once and reuses for four pages. - An unsealed parent validates on every page (control for the fast path not leaking). - A hand-frozen parent with `sealedResult: true` but no snapshot proof validates on every page (control for the hint being insufficient). - A sealed parent with invalid content fails on both attempts and never caches the rejection. - Two distinct sealed sources each validate once, so one source cannot read another's interpretation. Base reproduction: with the two source files reverted, the first two cases fail with `expected "Mock" to be called 1 times, but got 4 times` — four pages, four validations, exactly the reported behavior. The four control cases pass in both states, which is what they are for. With the change, all six pass. Measurements, on a sealed JavaScript parent built from the repository's own example analysis (`kind: "summary"`, 20 warm pages, Node 24.21.0, Windows x64): | | cold | warm total | warm per page | |---|---:|---:|---:| | Before | 30.69 ms | 162.47 ms | 8.12 ms | | After | 18.27 ms | 23.49 ms | 1.17 ms | That is roughly 7× on warm pages. I did not measure CPU time, peak process-family memory, or retained-state size, and I did not reproduce the reporter's authentic 21 MB npm bundle or the SQLite ELF layout, so this is not a whole-task speedup claim and not the measurement set the issue's acceptance criteria ask for. The numbers above describe one synthetic sealed parent on this host. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed Host: Windows 10 x64, Node 24.21.0, npm 11.9.0. - `npx vitest run --project domain src/domain/analysisView/` — 130 passed across 8 files. - `npx vitest run src/application/analysisView/AnalysisViewService.test.ts` — 34 passed. - `npx vitest run tests/boundary/cli/analysisViewCli.test.ts --testTimeout=180000` — 13 passed. - `npx vitest run tests/boundary/mcp/analysisViewMcp.test.ts --testTimeout=120000` — 18 passed. - Red-green check — reverting `analysisView.ts` and `analysisViewParent.ts` makes the two reuse cases fail with `expected "Mock" to be called 1 times, but got 4 times`; restoring them makes all six pass. - `npm run check:fast` — typecheck and lint passed, 2 of 2 tasks. - `npx oxfmt --check src/domain/analysisView/*.ts` — all files correctly formatted. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: none. No provider engine is involved. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the cache admits only values whose snapshot proof was registered by this realm, so forged freeze hints and caller-supplied flags cannot cause reuse. It retains no credential, path, or environment data beyond the parsed JSON the caller already supplied, and it creates no process or descriptor. ## Author readiness Human owner: @DeryFerd - [x] 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. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:14:30 +00:00

closed this issue
added the
pull-request
label

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#6444

No description provided.

Rows
Columns