mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
perf(tui): defer startup skills refresh (#18370)
# Summary This removes startup `skills/list` from the critical path to first input. In release measurements, median startup-to-input time improved from `307.5 ms` to `191.0 ms` across 30 measured runs with 5 warmups. # Background Startup currently waits for a forced `skills/list` app-server request before scheduling the first usable TUI frame. That makes skill metadata freshness part of the process-launch-to-input path, even though the prompt can safely accept normal input before skill metadata has finished loading. I measured startup from process launch until the TUI reports that the user can type. The measurement harness watched the startup measurement record, killed Codex after a successful sample, and enforced a timeout so repeated runs would not leave TUI processes behind. The debug runs had enough outliers that I used median as the primary signal and ran a baseline self-compare to understand the noise floor. # Why skills/list The `skills/list` cut was the best practical optimization because it improved startup without changing the important readiness contract: when the prompt is shown, it is still backed by an active session. Only enrichment data arrives later. | Candidate | Result | Decision | | --- | --- | --- | | Defer startup `skills/list` | Debug median improved from `524.0 ms` to `348.0 ms`; release median improved from `307.5 ms` to `191.0 ms`. | Keep | | Defer fresh `thread/start` | Debug median improved from `494.0 ms` to `256.0 ms`, but the prompt could appear before an active thread was attached. | Reject as too risky for this PR | | Avoid forced skills config reload | Debug median moved from `509.0 ms` to `512.0 ms`. | Reject as neutral | | Skip fresh history metadata | Debug median moved from `496.5 ms` to `531.5 ms`. | Reject as regression/noise | | Defer app-server startup | Not implemented because it would only permit a loading frame unless the TUI gained a deliberate pre-server state. | Out of scope | # Implementation `App::refresh_startup_skills` now clones the app-server request handle, spawns a background task, and issues the same forced `skills/list` request after the first frame is scheduled. When the request completes, the task sends `AppEvent::SkillsListLoaded` back through the normal app event queue. The existing skills response handling still converts the app-server response, updates the chat widget, and emits invalid `SKILL.md` warnings. Explicit user-initiated skills refreshes still use the existing synchronous app command path, so callers that intentionally requested fresh skill state do not race ahead of their own refresh. # Tradeoffs The main tradeoff is a narrow theoretical race at startup: skill mention completion depends on a background `skills/list` response, so it could briefly show stale or empty metadata if opened before that response arrives. In manual testing, pressing `$` as soon as possible after launch still showed populated skill metadata, so this risk appears minimal in normal use. Plain input remains available immediately, and the UI updates through the existing skills response path once the refresh completes. This PR does not change how skills are discovered, cached, force-reloaded, displayed, enabled, or warned about. It only changes when the startup refresh is allowed to complete relative to the first usable TUI frame. # Verification - `cargo test -p codex-tui`
This commit is contained in:
committed by
GitHub
Unverified
parent
92cf90277d
commit
48f117d0a2
+47
-10
@@ -93,6 +93,7 @@ use codex_app_server_protocol::PluginUninstallResponse;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::ServerNotification;
|
||||
use codex_app_server_protocol::ServerRequest;
|
||||
use codex_app_server_protocol::SkillsListParams;
|
||||
use codex_app_server_protocol::SkillsListResponse;
|
||||
use codex_app_server_protocol::ThreadItem;
|
||||
use codex_app_server_protocol::ThreadLoadedListParams;
|
||||
@@ -2072,6 +2073,25 @@ impl App {
|
||||
});
|
||||
}
|
||||
|
||||
/// Starts the initial skills refresh without delaying the first interactive frame.
|
||||
///
|
||||
/// Startup only needs skill metadata to populate skill mentions and the skills UI; the prompt can be
|
||||
/// rendered before that metadata arrives. The result is routed through the normal app event queue so
|
||||
/// the same response handler updates the chat widget and emits invalid `SKILL.md` warnings once the
|
||||
/// app-server RPC finishes. User-initiated skills refreshes still use the blocking app command path so
|
||||
/// callers that explicitly asked for fresh skill state do not race ahead of their own refresh.
|
||||
fn refresh_startup_skills(&mut self, app_server: &AppServerSession) {
|
||||
let request_handle = app_server.request_handle();
|
||||
let app_event_tx = self.app_event_tx.clone();
|
||||
let cwd = self.config.cwd.to_path_buf();
|
||||
tokio::spawn(async move {
|
||||
let result = fetch_skills_list(request_handle, cwd)
|
||||
.await
|
||||
.map_err(|err| err.to_string());
|
||||
app_event_tx.send(AppEvent::SkillsListLoaded { result });
|
||||
});
|
||||
}
|
||||
|
||||
fn fetch_plugins_list(&mut self, app_server: &AppServerSession, cwd: PathBuf) {
|
||||
let request_handle = app_server.request_handle();
|
||||
let app_event_tx = self.app_event_tx.clone();
|
||||
@@ -4036,16 +4056,6 @@ impl App {
|
||||
app.enqueue_primary_thread_session(started.session, started.turns)
|
||||
.await?;
|
||||
}
|
||||
app.handle_skills_list_result(
|
||||
app_server
|
||||
.skills_list(codex_app_server_protocol::SkillsListParams {
|
||||
cwds: vec![app.config.cwd.to_path_buf()],
|
||||
force_reload: true,
|
||||
per_cwd_extra_user_roots: None,
|
||||
})
|
||||
.await,
|
||||
"failed to load skills on startup",
|
||||
);
|
||||
|
||||
// On startup, if Agent mode (workspace-write) or ReadOnly is active, warn about world-writable dirs on Windows.
|
||||
#[cfg(target_os = "windows")]
|
||||
@@ -4076,6 +4086,7 @@ impl App {
|
||||
tokio::pin!(tui_events);
|
||||
|
||||
tui.frame_requester().schedule_frame();
|
||||
app.refresh_startup_skills(&app_server);
|
||||
// Kick off a non-blocking rate-limit prefetch so the first `/status`
|
||||
// already has data, without delaying the initial frame render.
|
||||
if requires_openai_auth && has_chatgpt_account {
|
||||
@@ -4774,6 +4785,12 @@ impl App {
|
||||
AppEvent::McpInventoryLoaded { result } => {
|
||||
self.handle_mcp_inventory_result(result);
|
||||
}
|
||||
AppEvent::SkillsListLoaded { result } => {
|
||||
self.handle_skills_list_result(
|
||||
result.map_err(|err| color_eyre::eyre::eyre!(err)),
|
||||
"failed to load skills on startup",
|
||||
);
|
||||
}
|
||||
AppEvent::StartFileSearch(query) => {
|
||||
self.file_search.on_user_query(query);
|
||||
}
|
||||
@@ -6454,6 +6471,26 @@ async fn fetch_account_rate_limits(
|
||||
Ok(app_server_rate_limit_snapshots_to_core(response))
|
||||
}
|
||||
|
||||
async fn fetch_skills_list(
|
||||
request_handle: AppServerRequestHandle,
|
||||
cwd: PathBuf,
|
||||
) -> Result<SkillsListResponse> {
|
||||
let request_id = RequestId::String(format!("startup-skills-list-{}", Uuid::new_v4()));
|
||||
// Use the cloneable request handle so startup can issue this RPC from a background task without
|
||||
// extending a borrow of `AppServerSession` across the first frame render.
|
||||
request_handle
|
||||
.request_typed(ClientRequest::SkillsList {
|
||||
request_id,
|
||||
params: SkillsListParams {
|
||||
cwds: vec![cwd],
|
||||
force_reload: true,
|
||||
per_cwd_extra_user_roots: None,
|
||||
},
|
||||
})
|
||||
.await
|
||||
.wrap_err("skills/list failed in TUI")
|
||||
}
|
||||
|
||||
async fn fetch_plugins_list(
|
||||
request_handle: AppServerRequestHandle,
|
||||
cwd: PathBuf,
|
||||
|
||||
@@ -17,6 +17,7 @@ use codex_app_server_protocol::PluginListResponse;
|
||||
use codex_app_server_protocol::PluginReadParams;
|
||||
use codex_app_server_protocol::PluginReadResponse;
|
||||
use codex_app_server_protocol::PluginUninstallResponse;
|
||||
use codex_app_server_protocol::SkillsListResponse;
|
||||
use codex_file_search::FileMatch;
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::openai_models::ModelPreset;
|
||||
@@ -306,6 +307,15 @@ pub(crate) enum AppEvent {
|
||||
result: Result<Vec<McpServerStatus>, String>,
|
||||
},
|
||||
|
||||
/// Result of the startup skills refresh that runs after the first frame is scheduled.
|
||||
///
|
||||
/// This event is startup-only. Interactive skills refreshes are handled synchronously through the app
|
||||
/// command path because those callers expect the visible skill state to be current when their command
|
||||
/// completes.
|
||||
SkillsListLoaded {
|
||||
result: Result<SkillsListResponse, String>,
|
||||
},
|
||||
|
||||
InsertHistoryCell(Box<dyn HistoryCell>),
|
||||
|
||||
/// Apply rollback semantics to local transcript cells.
|
||||
|
||||
Reference in New Issue
Block a user