Repository navigation
perf(host): export only the requested plugin package - #6661
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed the complete four-file change, the existing application commit/read path and SQL schema, read_package, and the manager's previous source_at checks.
The change has a concrete cost-saving mechanism: exportPackage previously loaded and hashed every package and rebuilt the revision's plan for each requested export. It now checks membership of the requested package in the exact deployment revision and reads only that package. The manager retains both state-domain and deployment checks; storage validates the source and package references, RootStore still requires an open owner, and read_package still validates paths, BLOB payloads and the reconstructed package digest. Exporting retained committed revisions continues to work without granting membership from another revision or deployment.
I found no confirmed P0–P2 issue in this change. I executed the exact new membership SQL against the real initial schema in a synthetic SQLite database: original/current revision, absent revision, another package and another deployment cases passed. Separately, I read the original hosted test logs from CI 42164401964: the expanded immutable-revision test passed in job 127726921911, and the expanded corruption test passed in job 127726921874 (their complete shards passed 188/188 and 187/187). Their actual checkout is 17e7203962e9c8ff10c76feeea1880f197e6a8cc, the merge of this head with rust-preview@72162299; my merge-tree produces the same tree.
The overall CI run failed: native7 repeats the #6655 cancelled-instruction-ack fixture problem, and CLI1 times out waiting for the model/source-search UI at keyboard.rs:312. I read both logs; I have not attributed the UI wait to this export change, so it is not an additional P2 finding here. The merge is clean. No protocol or database migration changes.
I did not run the complete Rust suite locally, a native desktop startup, a real Host package stream, or an independent performance benchmark. The release/debug timing improvements in the PR description are the author's measurements, not mine.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
management.exportPackage loaded the whole committed source for every export: all installed packages read from SQLite, every digest recomputed and the plan validated again, under the manager's lock. The workbench exports one package per UI plugin while it starts, so each import paid for every package of the installation (25 packages, about 9 MB by default). Read only the requested package of the revision instead. A foreign source is still rejected, a package outside the revision is still reported as not installed, and the exported bytes are still checked against their digest. In the release desktop fixture the startup plugin load chain drops from about 560 ms to 260 ms, and the workspace picker appears about 170 ms sooner. Generated-by: Claude Code
0848b6c to
776b1b7
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
I completed the follow-up review of the final eight-file change at 776b1b7, rather than extending the earlier four-file review to this larger scope without checking it.
The final version also moves package reconstruction/digest validation and full-source plan validation off the SQLite worker, reads package payloads in one query, applies the exact-package loader to readPackage and package-settings queries, reserves export resources before loading, and caches the immutable deployment-manager contract. I checked that manager domain/deployment validation, exact revision membership, path/BLOB validation, reconstructed digest checks, and RootStore's open-owner check remain. The transaction captures the immutable package bytes before the blocking worker validates them. The new sibling-corruption regression covers why a requested intact package no longer depends on validating an unrelated package.
I found no confirmed P0–P2 issue in these changes. I reran the exact production membership and payload SQL against this head's real initial schema in a synthetic SQLite database: six revision/deployment/package membership cases passed, the requested bytes remained independent of a corrupt sibling, and non-BLOB values were exposed as NULL for rejection. This checks SQL selection and type handling; it is not a Rust SQLx, digest-validation, cancellation or performance test. I read the complete final diff and compared the eight affected paths against the previously reviewed 0848 version, separately from incoming base changes. The actual merge of this head into 95df8f6 produces the exact tree merged as 01115825.
The final-head CI and merged-branch CI are still queued/running, so the new Rust tests have not been independently accepted here. The earlier hosted immutable-revision/corruption tests belonged to 0848 and do not establish this final head's result. I did not run the complete Rust suite, a real Host package export, or an independent desktop benchmark. No protocol or database schema changes. No approval or merge performed.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
The workbench imports each UI plugin by exporting its package through
management.exportPackage. The Host served every export by loading the whole committed source. That means reading every installed package from SQLite, recomputing every package digest and validating the plan again, while holding the manager's lock. A default installation has 25 packages (about 9 MB). Each plugin import therefore paid for all of them, and the five imports at desktop startup ran one after another behind that lock.exportPackagenow reads only the requested package of that revision, through the newRootStore::load_application_package_at. The other packages of the revision are no longer read. Behaviour is otherwise the same:Measured with a temporary probe (not part of this PR) in the desktop real-host fixture, from
Desktop::openuntil the composer's workspace picker is shown. The plugin load chain is the time from the UI catalogue arriving until the last startup plugin is active.Debug builds hash without optimisation, so the real-host tests gain about 1.5 s every time they open a desktop.
AI use
Tool(s) and scope: Claude Code measured the startup timeline, found the whole-source load in
exportPackage, and wrote the change, the tests and this description. The commit carriesGenerated-by: Claude Code.